Do not match on parameter names

One of the bugs I recently found caused the transform for a getter to be
applied against a parameter. This CL fixes that particular issue by
correctly computing the element name to be the name of the enclosing
invocation rather than the parameter.

It also introduces an `ElementMatcher` to hold the data currently being
used to match elements with the intent that more information will be
added in the future to prevent similar sorts of mismatches.

Change-Id: I14abbece334b3dc660d3854b817362c441efa11d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166600
Reviewed-by: Phil Quitslund <pquitslund@google.com>

pkg/analysis_server/lib/src/services/correction/dart/data_driven.dart

@@ -5,6 +5,7 @@
 import 'package:analysis_server/src/services/correction/dart/abstract_producer.dart';
 import 'package:analysis_server/src/services/correction/fix.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/element_descriptor.dart';
+import 'package:analysis_server/src/services/correction/fix/data_driven/element_matcher.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform_set.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform_set_manager.dart';
@@ -33,8 +34,9 @@ class DataDriven extends MultiCorrectionProducer {
         importedUris.add(Uri.parse(uri));
       }
     }
+    var matcher = ElementMatcher(importedUris: importedUris, name: name);
     for (var set in _availableTransformSetsForLibrary(library)) {
-      for (var transform in set.transformsFor(name, importedUris)) {
+      for (var transform in set.transformsFor(matcher)) {
         yield DataDrivenFix(transform);
       }
     }
@@ -42,27 +44,40 @@ class DataDriven extends MultiCorrectionProducer {
 
   /// Return the name of the element that was changed.
   String get _name {
+    String nameFromParent(AstNode node) {
+      var parent = node.parent;
+      if (parent is MethodInvocation) {
+        return parent.methodName.name;
+      } else if (parent is InstanceCreationExpression) {
+        var constructorName = parent.constructorName;
+        if (constructorName.name != null) {
+          return constructorName.name.name;
+        }
+        return constructorName.type.name.name;
+      } else if (parent is ExtensionOverride) {
+        return parent.extensionName.name;
+      }
+      return null;
+    }
+
     var node = this.node;
     if (node is SimpleIdentifier) {
+      var parent = node.parent;
+      if (parent is Label && parent.parent is NamedExpression) {
+        // The parent of the named expression is an argument list. Because we
+        // don't represent parameters as elements, the element we need to match
+        // against is the invocation containing those arguments.
+        return nameFromParent(parent.parent.parent);
+      }
       return node.name;
     } else if (node is ConstructorName) {
       return node.name.name;
     } else if (node is NamedType) {
       return node.name.name;
     } else if (node is TypeArgumentList) {
-      var parent = node.parent;
-      if (parent is MethodInvocation) {
-        return parent.methodName.name;
-      } else if (parent is ExtensionOverride) {
-        return parent.extensionName.name;
-      }
+      return nameFromParent(node);
     } else if (node is ArgumentList) {
-      var parent = node.parent;
-      if (parent is MethodInvocation) {
-        return parent.methodName.name;
-      } else if (parent is ExtensionOverride) {
-        return parent.extensionName.name;
-      }
+      return nameFromParent(node);
     }
     return null;
   }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/element_descriptor.dart

@@ -27,23 +27,4 @@ class ElementDescriptor {
 
   /// Return `true` if the described element is a constructor.
   bool get isConstructor => _kind == 'constructor';
-
-  /// Return `true` if this descriptor matches an element with the given [name]
-  /// in a library that imports the [importedUris].
-  bool matches(String name, List<Uri> importedUris) {
-    var lastComponent = components.last;
-    if (lastComponent.isEmpty) {
-      if (components[components.length - 2] != name) {
-        return false;
-      }
-    } else if (lastComponent != name) {
-      return false;
-    }
-    for (var importedUri in importedUris) {
-      if (libraryUris.contains(importedUri)) {
-        return true;
-      }
-    }
-    return false;
-  }
 }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/element_matcher.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:analysis_server/src/services/correction/fix/data_driven/element_descriptor.dart';
+import 'package:meta/meta.dart';
+
+/// An object that can be used to determine whether an element is appropriate
+/// for a given reference.
+class ElementMatcher {
+  /// The URIs of the libraries that are imported in the library containing the
+  /// reference.
+  final List<Uri> importedUris;
+
+  /// The name of the element being referenced.
+  final String name;
+
+  /// Initialize a newly created matcher representing a reference to an element
+  /// with the given [name] in a library that imports the [importedUris].
+  ElementMatcher({@required this.importedUris, @required this.name});
+
+  /// Return `true` if this matcher matches the given [element].
+  bool matches(ElementDescriptor element) {
+    var components = element.components;
+    var lastComponent = components.last;
+    if (lastComponent.isEmpty) {
+      if (components[components.length - 2] != name) {
+        return false;
+      }
+    } else if (lastComponent != name) {
+      return false;
+    }
+    var libraryUris = element.libraryUris;
+    for (var importedUri in importedUris) {
+      if (libraryUris.contains(importedUri)) {
+        return true;
+      }
+    }
+    return false;
+  }
+}

pkg/analysis_server/lib/src/services/correction/fix/data_driven/transform.dart

@@ -4,6 +4,7 @@
 
 import 'package:analysis_server/src/services/correction/fix/data_driven/change.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/element_descriptor.dart';
+import 'package:analysis_server/src/services/correction/fix/data_driven/element_matcher.dart';
 import 'package:meta/meta.dart';
 
 /// A description of a set of changes to a single element of the API.
@@ -29,9 +30,8 @@ class Transform {
       @required this.changes});
 
   /// Return `true` if this transform can be applied to fix an issue related to
-  /// an element with the given [name] in a library that imports the
-  /// [importedUris].
-  bool appliesTo(String name, List<Uri> importedUris) {
-    return element.matches(name, importedUris);
+  /// an element that matches the given [matcher].
+  bool appliesTo(ElementMatcher matcher) {
+    return matcher.matches(element);
   }
 }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/transform_set.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analysis_server/src/services/correction/fix/data_driven/element_matcher.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform.dart';
 
 /// A set of transforms used to aid in the construction of fixes for issues
@@ -16,12 +17,11 @@ class TransformSet {
     _transforms.add(transform);
   }
 
-  /// Return a list of the transforms that apply for a reference to the given
-  /// [name] in a library that imports the [importedUris].
-  List<Transform> transformsFor(String name, List<Uri> importedUris) {
+  /// Return a list of the transforms that match the [matcher].
+  List<Transform> transformsFor(ElementMatcher matcher) {
     var result = <Transform>[];
     for (var transform in _transforms) {
-      if (transform.appliesTo(name, importedUris)) {
+      if (transform.appliesTo(matcher)) {
         result.add(transform);
       }
     }

pkg/analysis_server/test/src/services/correction/fix/data_driven/rename_test.dart

@@ -15,6 +15,7 @@ void main() {
     defineReflectiveTests(RenameConstructorTest);
     defineReflectiveTests(RenameExtensionTest);
     defineReflectiveTests(RenameFieldTest);
+    defineReflectiveTests(RenameGetterTest);
     defineReflectiveTests(RenameMethodTest);
     defineReflectiveTests(RenameMixinTest);
     defineReflectiveTests(RenameTopLevelFunctionTest);
@@ -740,6 +741,124 @@ void f() {
   }
 }
 
+@reflectiveTest
+class RenameGetterTest extends _AbstractRenameTest {
+  @override
+  String get _kind => 'getter';
+
+  Future<void> test_instance_nonReference_deprecated() async {
+    setPackageContent('''
+class C {
+  @deprecated
+  int get a => 0;
+  int get b => 1;
+}
+class D {
+  D({@deprecated int a; int c});
+}
+''');
+    setPackageData(_rename(['C', 'a'], 'b'));
+    await resolveTestUnit('''
+import '$importUri';
+
+D d = D(a: 2);
+''');
+    await assertNoFix();
+  }
+
+  Future<void> test_instance_reference_deprecated() async {
+    setPackageContent('''
+class C {
+  @deprecated
+  int get old => 0;
+  int get new => 1;
+}
+''');
+    setPackageData(_rename(['C', 'old'], 'new'));
+    await resolveTestUnit('''
+import '$importUri';
+
+void f(C c) {
+  c.old;
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f(C c) {
+  c.new;
+}
+''');
+  }
+
+  Future<void> test_instance_reference_removed() async {
+    setPackageContent('''
+class C {
+  int get new => 1;
+}
+''');
+    setPackageData(_rename(['C', 'old'], 'new'));
+    await resolveTestUnit('''
+import '$importUri';
+
+void f(C c) {
+  c.old;
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f(C c) {
+  c.new;
+}
+''');
+  }
+
+  Future<void> test_topLevel_reference_deprecated() async {
+    setPackageContent('''
+@deprecated
+int get old => 0;
+int get new => 1;
+''');
+    setPackageData(_rename(['old'], 'new'));
+    await resolveTestUnit('''
+import '$importUri';
+
+void f() {
+  old;
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  new;
+}
+''');
+  }
+
+  Future<void> test_topLevel_reference_removed() async {
+    setPackageContent('''
+int get new => 1;
+''');
+    setPackageData(_rename(['old'], 'new'));
+    await resolveTestUnit('''
+import '$importUri';
+
+void f() {
+  old;
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  new;
+}
+''', errorFilter: ignoreUnusedImport);
+  }
+}
+
 @reflectiveTest
 class RenameMethodTest extends _AbstractRenameTest {
   @override

pkg/analysis_server/test/src/services/correction/fix/data_driven/transform_set_parser_test.dart

@@ -4,9 +4,11 @@
 
 import 'package:analysis_server/src/services/correction/fix/data_driven/add_type_parameter.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/code_template.dart';
+import 'package:analysis_server/src/services/correction/fix/data_driven/element_matcher.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/modify_parameters.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/parameter_reference.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/rename.dart';
+import 'package:analysis_server/src/services/correction/fix/data_driven/transform.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform_set_error_code.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/value_generator.dart';
 import 'package:matcher/matcher.dart';
@@ -46,7 +48,7 @@ transforms:
             kind: 'argument'
             index: 1
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -82,7 +84,7 @@ transforms:
       name: 'p'
       style: optional_positional
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -118,7 +120,7 @@ transforms:
             kind: 'argument'
             index: 1
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -160,7 +162,7 @@ transforms:
             kind: 'argument'
             index: 1
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -205,7 +207,7 @@ transforms:
             kind: 'argument'
             index: 2
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -254,7 +256,7 @@ transforms:
             uris: ['dart:core']
             name: 'String'
 ''');
-    var transforms = result.transformsFor('A', uris);
+    var transforms = _transforms('A');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -290,7 +292,7 @@ transforms:
             kind: 'argument'
             name: 'p'
 ''');
-    var transforms = result.transformsFor('A', uris);
+    var transforms = _transforms('A');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -331,7 +333,7 @@ transforms:
             kind: 'argument'
             index: 2
 ''');
-    var transforms = result.transformsFor('A', uris);
+    var transforms = _transforms('A');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Add');
@@ -363,7 +365,7 @@ transforms:
     getter: 'g'
   changes: []
 ''');
-    var transforms = result.transformsFor('g', uris);
+    var transforms = _transforms('g');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename g');
@@ -382,7 +384,7 @@ transforms:
     inMixin: 'A'
   changes: []
 ''');
-    var transforms = result.transformsFor('g', uris);
+    var transforms = _transforms('g');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename g');
@@ -400,7 +402,7 @@ transforms:
     getter: 'g'
   changes: []
 ''');
-    var transforms = result.transformsFor('g', uris);
+    var transforms = _transforms('g');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename g');
@@ -419,7 +421,7 @@ transforms:
     inClass: 'A'
   changes: []
 ''');
-    var transforms = result.transformsFor('m', uris);
+    var transforms = _transforms('m');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename m');
@@ -437,7 +439,7 @@ transforms:
     variable: 'v'
   changes: []
 ''');
-    var transforms = result.transformsFor('v', uris);
+    var transforms = _transforms('v');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename v');
@@ -478,7 +480,7 @@ transforms:
     - kind: 'removeParameter'
       name: 'p'
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Remove');
@@ -504,7 +506,7 @@ transforms:
     - kind: 'removeParameter'
       index: 0
 ''');
-    var transforms = result.transformsFor('f', uris);
+    var transforms = _transforms('f');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Remove');
@@ -531,7 +533,7 @@ transforms:
     - kind: 'rename'
       newName: 'B'
 ''');
-    var transforms = result.transformsFor('A', uris);
+    var transforms = _transforms('A');
     expect(transforms, hasLength(1));
     var transform = transforms[0];
     expect(transform.title, 'Rename A');
@@ -539,4 +541,10 @@ transforms:
     var rename = transform.changes[0] as Rename;
     expect(rename.newName, 'B');
   }
+
+  ElementMatcher _matcher(String name) =>
+      ElementMatcher(importedUris: uris, name: name);
+
+  List<Transform> _transforms(String name) =>
+      result.transformsFor(_matcher(name));
 }