Start to refactor ElementMatcher

This is the first of multiple smaller CLs to refactor `ElementMatcher`.

I was starting to fix a bug and realized that the current implementation
allows the number of components to be out of sync with the element kind.
The primary goal is to prevent that by eventually merging the methods
`_componentsForNode` and `_kindsForNode` into a single method
(`buildMatchersForNode`).

In the process I realized that we can sometimes match either a top-level
declaraation or an instance member from a superclass, and that means
that we need multiple matchers (otherwise I think the number of
components will continue to be out-of-sync with the element kinds).

This CL also includes the failing test that started the whole
investigation into refactoring this class.

Change-Id: I0c3a56f29f0f6c6d0cad6ac80145b26201931518
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/228722
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: bwilkerson

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

@@ -33,15 +33,17 @@ class DataDriven extends MultiCorrectionProducer {
         importedUris.add(Uri.parse(uri));
       }
     }
-    var matcher = ElementMatcher.forNode(node);
-    if (matcher == null) {
-      // The node doesn't represent an element that can be transformed.
+    var matchers = ElementMatcher.matchersForNode(node);
+    if (matchers.isEmpty) {
+      // The node doesn't represent any element that can be transformed.
       return;
     }
     for (var set in _availableTransformSetsForLibrary(library)) {
-      for (var transform
-          in set.transformsFor(matcher, applyingBulkFixes: applyingBulkFixes)) {
-        yield DataDrivenFix(transform);
+      for (var matcher in matchers) {
+        for (var transform in set.transformsFor(matcher,
+            applyingBulkFixes: applyingBulkFixes)) {
+          yield DataDrivenFix(transform);
+        }
       }
     }
   }

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

@@ -26,13 +26,14 @@ class ElementMatcher {
   final List<ElementKind> validKinds;
 
   /// Initialize a newly created matcher representing a reference to an element
-  /// with the given [name] in a library that imports the [importedUris].
+  /// whose name matches the given [components] and element [kinds] in a library
+  /// that imports the [importedUris].
   ElementMatcher(
       {required this.importedUris,
       required this.components,
-      List<ElementKind>? kinds})
+      required List<ElementKind> kinds})
       : assert(components.isNotEmpty),
-        validKinds = kinds ?? const [];
+        validKinds = kinds;
 
   /// Return `true` if this matcher matches the given [element].
   bool matches(ElementDescriptor element) {
@@ -93,25 +94,68 @@ class ElementMatcher {
     return false;
   }
 
-  /// Return an element matcher that will match the element that is, or should
-  /// be, associated with the given [node], or `null` if there is no appropriate
-  /// matcher for the node.
-  static ElementMatcher? forNode(AstNode? node) {
+  /// Return a list of element matchers that will match the element that is, or
+  /// should be, associated with the given [node]. The list will be empty if
+  /// there are no appropriate matchers for the [node].
+  static List<ElementMatcher> matchersForNode(AstNode? node) {
     if (node == null) {
-      return null;
+      return const <ElementMatcher>[];
     }
     var importedUris = _importElementsForNode(node);
     if (importedUris == null) {
+      return const <ElementMatcher>[];
+    }
+    var builder = _MatcherBuilder(importedUris);
+    builder.buildMatchersForNode(node);
+    return builder.matchers.toList();
+  }
+
+  /// Return the URIs of the imports in the library containing the [node], or
+  /// `null` if the imports can't be determined.
+  static List<Uri>? _importElementsForNode(AstNode node) {
+    var root = node.root;
+    if (root is! CompilationUnit) {
       return null;
     }
+    var importedUris = <Uri>[];
+    var library = root.declaredElement?.library;
+    if (library == null) {
+      return null;
+    }
+    for (var importElement in library.imports) {
+      // TODO(brianwilkerson) Filter based on combinators to help avoid making
+      //  invalid suggestions.
+      var uri = importElement.importedLibrary?.source.uri;
+      if (uri != null) {
+        // The [uri] is `null` if the literal string is not a valid URI.
+        importedUris.add(uri);
+      }
+    }
+    return importedUris;
+  }
+}
+
+/// A helper class used to build a list of element matchers.
+class _MatcherBuilder {
+  final List<ElementMatcher> matchers = [];
+
+  final List<Uri> importedUris;
+
+  _MatcherBuilder(this.importedUris);
+
+  void buildMatchersForNode(AstNode? node) {
     var components = _componentsForNode(node);
     if (components == null) {
-      return null;
+      return;
     }
-    return ElementMatcher(
-        importedUris: importedUris,
-        components: components,
-        kinds: _kindsForNode(node));
+    var kinds = _kindsForNode(node) ?? [];
+    _addMatcher(components: components, kinds: kinds);
+  }
+
+  void _addMatcher(
+      {required List<String> components, required List<ElementKind> kinds}) {
+    matchers.add(ElementMatcher(
+        importedUris: importedUris, components: components, kinds: kinds));
   }
 
   /// Return the components of the path of the element associated with the given
@@ -244,30 +288,6 @@ class ElementMatcher {
     return null;
   }
 
-  /// Return the URIs of the imports in the library containing the [node], or
-  /// `null` if the imports can't be determined.
-  static List<Uri>? _importElementsForNode(AstNode node) {
-    var root = node.root;
-    if (root is! CompilationUnit) {
-      return null;
-    }
-    var importedUris = <Uri>[];
-    var library = root.declaredElement?.library;
-    if (library == null) {
-      return null;
-    }
-    for (var importElement in library.imports) {
-      // TODO(brianwilkerson) Filter based on combinators to help avoid making
-      //  invalid suggestions.
-      var uri = importElement.importedLibrary?.source.uri;
-      if (uri != null) {
-        // The [uri] is `null` if the literal string is not a valid URI.
-        importedUris.add(uri);
-      }
-    }
-    return importedUris;
-  }
-
   /// Return `true` if the [node] is a prefix
   static bool _isPrefix(AstNode? node) {
     return node is SimpleIdentifier && node.staticElement is PrefixElement;
@@ -312,6 +332,7 @@ class ElementMatcher {
       }
       return const [
         ElementKind.classKind,
+//        ElementKind.constructorKind,
         ElementKind.enumKind,
         ElementKind.mixinKind,
         ElementKind.typedefKind

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

@@ -23,7 +23,9 @@ abstract class AbstractElementMatcherTest extends DataDrivenFixProcessorTest {
       List<ElementKind>? expectedKinds,
       List<String>? expectedUris}) {
     var node = findNode.any(search);
-    var matcher = ElementMatcher.forNode(node)!;
+    var matchers = ElementMatcher.matchersForNode(node);
+    expect(matchers, hasLength(1));
+    var matcher = matchers[0];
     if (expectedUris != null) {
       expect(matcher.importedUris,
           unorderedEquals(expectedUris.map((uri) => Uri.parse(uri))));

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

@@ -706,6 +706,117 @@ void f() {
 ''');
   }
 
+  @failingTest
+  Future<void> test_material_FlatButton_deprecated() async {
+    setPackageContent('''
+@deprecated
+class FlatButton {
+  factory FlatButton.icon({
+    Key? key,
+    required VoidCallback? onPressed,
+    VoidCallback? onLongPress,
+    ValueChanged<bool>? onHighlightChanged,
+    ButtonTextTheme? textTheme,
+    Color? highlightColor,
+    Brightness? colorBrightness,
+    Clip? clipBehavior,
+    FocusNode? focusNode,
+    bool autofocus = true,
+    required Widget icon,
+    required Widget label,
+  }) => FlatButton();
+  FlatButton();
+}
+class Key {}
+class UniqueKey extends Key {}
+typedef VoidCallback = void Function();
+typedef ValueChanged<T> = void Function(T value);
+class ButtonTextTheme {}
+class Color {}
+class Colors {
+  static Color blue = Color();
+}
+class Brightness {
+  static Brightness dark = Brightness();
+}
+class Clip {
+  static Clip hardEdge = Clip();
+}
+class FocusNode {}
+class Widget {}
+class Icon extends Widget {
+  Icon(String icon);
+}
+class Text extends Widget {
+  Text(String content);
+}
+class Icons {
+  static String ten_k_outlined = '';
+}
+''');
+    addPackageDataFile('''
+version: 1
+transforms:
+  # Changes made in https://github.com/flutter/flutter/pull/73352
+  - title: "Migrate to 'TextButton.icon'"
+    date: 2021-01-08
+    element:
+      uris: [ '$importUri' ]
+      constructor: 'icon'
+      inClass: 'FlatButton'
+    changes:
+      - kind: 'removeParameter'
+        name: 'onHighlightChanged'
+      - kind: 'removeParameter'
+        name: 'textTheme'
+      - kind: 'removeParameter'
+        name: 'highlightColor'
+      - kind: 'removeParameter'
+        name: 'colorBrightness'
+      - kind: 'replacedBy'
+        newElement:
+          uris: [ '$importUri' ]
+          constructor: 'icon'
+          inClass: 'TextButton'
+''');
+    await resolveTestCode('''
+import '$importUri';
+
+void f() {
+  FlatButton.icon(
+    key: UniqueKey(),
+    icon: Icon(Icons.ten_k_outlined),
+    label: Text('FlatButton'),
+    onPressed: (){},
+    onLongPress: (){},
+    clipBehavior: Clip.hardEdge,
+    focusNode: FocusNode(),
+    autofocus: true,
+    onHighlightChanged: (_) {},
+    textTheme: ButtonTextTheme(),
+    highlightColor: Colors.blue,
+    colorBrightness: Brightness.dark,
+  );
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  TextButton.icon(
+    key: UniqueKey(),
+    icon: Icon(Icons.ten_k_outlined),
+    label: Text('FlatButton'),
+    onPressed: (){},
+    onLongPress: (){},
+    clipBehavior: Clip.hardEdge,
+    focusNode: FocusNode(),
+    autofocus: true,
+  );
+}
+''');
+  }
+
   Future<void>
       test_material_InputDecoration_defaultConstructor_matchFirstCase_deprecated() async {
     setPackageContent('''

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

@@ -784,7 +784,7 @@ transforms:
       (transform.changesSelector as UnconditionalChangesSelector).changes;
 
   ElementMatcher _matcher(String name) =>
-      ElementMatcher(importedUris: uris, components: [name]);
+      ElementMatcher(importedUris: uris, components: [name], kinds: const []);
 
   List<Transform> _transforms(String name) =>
       result!.transformsFor(_matcher(name), applyingBulkFixes: false);