[DAS] Fixes rename field add this to fix shadowing

[email protected]

Fixes: #59954
Change-Id: I860e48989566a09ea09092ddd1771be61f89121b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/413901
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Auto-Submit: Felipe Morschel <[email protected]>

Author: FMorschel

pkg/analysis_server/lib/src/services/refactoring/legacy/rename_class_member.dart

@@ -171,6 +171,10 @@ class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
     }
   }
 
+  Future<void> _addThisEdit(SourceReference reference, String newName) async {
+    reference.addEdit(change, 'this.$newName');
+  }
+
   String _newPotentialId() {
     assert(includePotential);
     var id = potentialEditIds.length.toString();
@@ -180,6 +184,9 @@ class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
 
   Future<void> _updateReferences() async {
     var references = getSourceReferences(_validator.references);
+    var unshadowed = getSourceReferences(
+      _validator.unshadowed,
+    ).map((r) => r.element);
 
     for (var reference in references) {
       var element = reference.element;
@@ -194,6 +201,12 @@ class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
         continue;
       }
 
+      if (unshadowed.contains(reference.element) &&
+          reference.element is ExecutableElement2) {
+        await _addThisEdit(reference, newName);
+        continue;
+      }
+
       reference.addEdit(change, newName);
     }
   }
@@ -317,7 +330,7 @@ class _CreateClassMemberValidator extends _BaseClassMemberValidator {
 
 class _LocalElementsCollector extends GeneralizingAstVisitor<void> {
   final String name;
-  final List<LocalElement2> elements = [];
+  final List<Element2> elements = [];
 
   _LocalElementsCollector(this.name);
 
@@ -345,6 +358,17 @@ class _LocalElementsCollector extends GeneralizingAstVisitor<void> {
     super.visitSimpleFormalParameter(node);
   }
 
+  @override
+  void visitSimpleIdentifier(SimpleIdentifier node) {
+    var element = node.element;
+    if (element is GetterElement && element.name3 == name) {
+      if (element.variable3 case TopLevelVariableElement2 variable) {
+        elements.add(variable);
+      }
+    }
+    super.visitSimpleIdentifier(node);
+  }
+
   @override
   void visitVariableDeclaration(VariableDeclaration node) {
     if (node.name.lexeme == name) {
@@ -358,11 +382,19 @@ class _LocalElementsCollector extends GeneralizingAstVisitor<void> {
   }
 }
 
-class _MatchShadowedByLocal {
+class _MatchShadowedBy {
   final SearchMatch match;
-  final LocalElement2 localElement;
+  final Element2 element;
+
+  _MatchShadowedBy(this.match, this.element);
+}
+
+class _MatchShadowedByLocal extends _MatchShadowedBy {
+  @override
+  final LocalElement2 element;
 
-  _MatchShadowedByLocal(this.match, this.localElement);
+  _MatchShadowedByLocal(SearchMatch match, this.element)
+    : super(match, element);
 }
 
 /// Helper to check if the renamed [element] will cause any conflicts.
@@ -371,6 +403,7 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
 
   Set<Element2> elements = {};
   List<SearchMatch> references = [];
+  List<SearchMatch> unshadowed = [];
 
   _RenameClassMemberValidator(
     SearchEngine searchEngine,
@@ -404,15 +437,15 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
     }
     // usage of the renamed Element is shadowed by a local element
     {
-      var conflict = await _getShadowingLocalElement();
+      var conflict = await _getLocalShadowingElement();
       if (conflict != null) {
-        var localElement = conflict.localElement;
+        var element = conflict.element;
         result.addError(
           format(
             "Usage of renamed {0} will be shadowed by {1} '{2}'.",
             elementKind.displayName,
-            getElementKindName(localElement),
-            localElement.displayName,
+            getElementKindName(element),
+            element.displayName,
           ),
           newLocation_fromMatch(conflict.match),
         );
@@ -426,11 +459,11 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
     return result;
   }
 
-  Future<_MatchShadowedByLocal?> _getShadowingLocalElement() async {
-    var localElementMap = <LibraryFragment, List<LocalElement2>>{};
-    var visibleRangeMap = <LocalElement2, SourceRange>{};
+  Future<_MatchShadowedBy?> _getLocalShadowingElement() async {
+    var localElementMap = <LibraryFragment, List<Element2>>{};
+    var visibleRangeMap = <Element2, SourceRange>{};
 
-    Future<List<LocalElement2>> getLocalElements(Element2 element) async {
+    Future<List<Element2>> getLocalElements(Element2 element) async {
       var unitFragment = element.firstFragment.libraryFragment;
       if (unitFragment == null) {
         return const [];
@@ -464,11 +497,20 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
       }
       // Check local elements that might shadow the reference.
       var localElements = await getLocalElements(match.element);
+      if (element.kind == ElementKind.FIELD) {
+        for (var element in localElements.avoidableShadowsForField) {
+          unshadowed.addAll(await searchEngine.searchReferences(element));
+          localElements.remove(element);
+        }
+      }
       for (var localElement in localElements) {
         var elementRange = visibleRangeMap[localElement];
         if (elementRange != null &&
             elementRange.intersects(match.sourceRange)) {
-          return _MatchShadowedByLocal(match, localElement);
+          if (localElement is LocalElement2) {
+            return _MatchShadowedByLocal(match, localElement);
+          }
+          return _MatchShadowedBy(match, localElement);
         }
       }
     }
@@ -513,3 +555,20 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
     }
   }
 }
+
+extension on Element2 {
+  bool get canAvoidShadowForField {
+    return switch (kind) {
+      ElementKind.LOCAL_VARIABLE ||
+      ElementKind.TOP_LEVEL_VARIABLE ||
+      ElementKind.PARAMETER => true,
+      _ => false,
+    };
+  }
+}
+
+extension on List<Element2> {
+  List<Element2> get avoidableShadowsForField {
+    return where((e) => e.canAvoidShadowForField).toList();
+  }
+}

pkg/analysis_server/test/services/refactoring/legacy/rename_class_member_test.dart

@@ -1242,6 +1242,100 @@ class A<NewName> {
 ''');
   }
 
+  Future<void> test_shadowingLocalVariable_addsThis() async {
+    await indexTestUnit('''
+class A {
+  final int? _value;
+
+  const A(this._value);
+
+  A copyWith() {
+    var value = _get();
+    return A(value ?? _value);
+  }
+
+  int? _get() => null;
+}
+''');
+    createRenameRefactoringAtString('_value;');
+    // check status
+    refactoring.newName = 'value';
+    await assertSuccessfulRefactoring('''
+class A {
+  final int? value;
+
+  const A(this.value);
+
+  A copyWith() {
+    var value = _get();
+    return A(value ?? this.value);
+  }
+
+  int? _get() => null;
+}
+''');
+  }
+
+  Future<void> test_shadowingParameter_addsThis() async {
+    await indexTestUnit('''
+class A {
+  final int? _value;
+
+  const A(this._value);
+
+  A copyWith({int? value}) {
+    return A(value ?? _value);
+  }
+}
+''');
+    createRenameRefactoringAtString('_value;');
+    // check status
+    refactoring.newName = 'value';
+    await assertSuccessfulRefactoring('''
+class A {
+  final int? value;
+
+  const A(this.value);
+
+  A copyWith({int? value}) {
+    return A(value ?? this.value);
+  }
+}
+''');
+  }
+
+  Future<void> test_shadowingTopLevelVariable_addsThis() async {
+    await indexTestUnit('''
+int? value = 0;
+
+class A {
+  final int? _value;
+
+  const A(this._value);
+
+  A copyWith() {
+    return A(value ?? _value);
+  }
+}
+''');
+    createRenameRefactoringAtString('_value;');
+    // check status
+    refactoring.newName = 'value';
+    await assertSuccessfulRefactoring('''
+int? value = 0;
+
+class A {
+  final int? value;
+
+  const A(this.value);
+
+  A copyWith() {
+    return A(value ?? this.value);
+  }
+}
+''');
+  }
+
   Future<void> test_subclass_namedSuper_otherLibrary() async {
     await indexTestUnit('''
 class Base {