Add support for extensions to unused import detection

bwilkerson · commit-bot@chromium.org · commit 7bde33139d00 · 2019-08-08T12:40:40.000Z
Change-Id: I211c769765293a35f371bd2b403b38ac5dcb3b14 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112280 Commit-Queue: Brian Wilkerson <brianwilkerson@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com>
diff --git a/pkg/analyzer/lib/src/error/imports_verifier.dart b/pkg/analyzer/lib/src/error/imports_verifier.dart
@@ -18,26 +18,56 @@ class GatherUsedImportedElementsVisitor extends RecursiveAstVisitor {
 
   GatherUsedImportedElementsVisitor(this.library);
 
+  @override
+  void visitBinaryExpression(BinaryExpression node) {
+    _recordIfExtensionMember(node.staticElement);
+    return super.visitBinaryExpression(node);
+  }
+
   @override
   void visitExportDirective(ExportDirective node) {
     _visitDirective(node);
   }
 
+  @override
+  void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) {
+    _recordIfExtensionMember(node.staticElement);
+    return super.visitFunctionExpressionInvocation(node);
+  }
+
   @override
   void visitImportDirective(ImportDirective node) {
     _visitDirective(node);
   }
 
+  @override
+  void visitIndexExpression(IndexExpression node) {
+    _recordIfExtensionMember(node.staticElement);
+    return super.visitIndexExpression(node);
+  }
+
   @override
   void visitLibraryDirective(LibraryDirective node) {
     _visitDirective(node);
   }
 
+  @override
+  void visitPrefixExpression(PrefixExpression node) {
+    _recordIfExtensionMember(node.staticElement);
+    return super.visitPrefixExpression(node);
+  }
+
   @override
   void visitSimpleIdentifier(SimpleIdentifier node) {
     _visitIdentifier(node, node.staticElement);
   }
 
+  void _recordIfExtensionMember(Element element) {
+    if (element != null && element.enclosingElement is ExtensionElement) {
+      _recordUsedExtension(element.enclosingElement);
+    }
+  }
+
   /// If the given [identifier] is prefixed with a [PrefixElement], fill the
   /// corresponding `UsedImportedElements.prefixMap` entry and return `true`.
   bool _recordPrefixMap(SimpleIdentifier identifier, Element element) {
@@ -61,6 +91,29 @@ class GatherUsedImportedElementsVisitor extends RecursiveAstVisitor {
     return false;
   }
 
+  void _recordUsedElement(Element element) {
+    // Ignore if an unknown library.
+    LibraryElement containingLibrary = element.library;
+    if (containingLibrary == null) {
+      return;
+    }
+    // Ignore if a local element.
+    if (library == containingLibrary) {
+      return;
+    }
+    // Remember the element.
+    usedElements.elements.add(element);
+  }
+
+  void _recordUsedExtension(ExtensionElement extension) {
+    // Ignore if a local element.
+    if (library == extension.library) {
+      return;
+    }
+    // Remember the element.
+    usedElements.usedExtensions.add(extension);
+  }
+
   /// Visit identifiers used by the given [directive].
   void _visitDirective(Directive directive) {
     directive.documentationComment?.accept(this);
@@ -71,44 +124,28 @@ class GatherUsedImportedElementsVisitor extends RecursiveAstVisitor {
     if (element == null) {
       return;
     }
-    // If the element is multiply defined then call this method recursively for
-    // each of the conflicting elements.
-    if (element is MultiplyDefinedElement) {
+    // Record `importPrefix.identifier` into 'prefixMap'.
+    if (_recordPrefixMap(identifier, element)) {
+      return;
+    }
+    Element enclosingElement = element.enclosingElement;
+    if (enclosingElement is CompilationUnitElement) {
+      _recordUsedElement(element);
+    } else if (enclosingElement is ExtensionElement) {
+      _recordUsedExtension(enclosingElement);
+      return;
+    } else if (element is PrefixElement) {
+      usedElements.prefixMap.putIfAbsent(element, () => <Element>[]);
+    } else if (element is MultiplyDefinedElement) {
+      // If the element is multiply defined then call this method recursively
+      // for each of the conflicting elements.
       List<Element> conflictingElements = element.conflictingElements;
       int length = conflictingElements.length;
       for (int i = 0; i < length; i++) {
         Element elt = conflictingElements[i];
         _visitIdentifier(identifier, elt);
       }
-      return;
-    }
-
-    // Record `importPrefix.identifier` into 'prefixMap'.
-    if (_recordPrefixMap(identifier, element)) {
-      return;
-    }
-
-    if (element is PrefixElement) {
-      usedElements.prefixMap.putIfAbsent(element, () => <Element>[]);
-      return;
-    } else if (element.enclosingElement is! CompilationUnitElement) {
-      // Identifiers that aren't a prefix element and whose enclosing element
-      // isn't a CompilationUnit are ignored- this covers the case the
-      // identifier is a relative-reference, a reference to an identifier not
-      // imported by this library.
-      return;
-    }
-    // Ignore if an unknown library.
-    LibraryElement containingLibrary = element.library;
-    if (containingLibrary == null) {
-      return;
     }
-    // Ignore if a local element.
-    if (library == containingLibrary) {
-      return;
-    }
-    // Remember the element.
-    usedElements.elements.add(element);
   }
 }
 
@@ -381,6 +418,22 @@ class ImportsVerifier {
         }
       }
     }
+    // Process extension elements.
+    for (ExtensionElement extensionElement in usedElements.usedExtensions) {
+      // Stop if all the imports and shown names are known to be used.
+      if (_unusedImports.isEmpty && _unusedShownNamesMap.isEmpty) {
+        return;
+      }
+      // Find import directives using namespaces.
+      String name = extensionElement.name;
+      for (ImportDirective importDirective in _allImports) {
+        Namespace namespace = _computeNamespace(importDirective);
+        if (namespace?.get(name) == extensionElement) {
+          _unusedImports.remove(importDirective);
+          _removeFromUnusedShownNamesMap(extensionElement, importDirective);
+        }
+      }
+    }
   }
 
   /// Add duplicate shown and hidden names from [directive] into
@@ -492,11 +545,12 @@ class ImportsVerifier {
 /// A container with information about used imports prefixes and used imported
 /// elements.
 class UsedImportedElements {
-  /// The map of referenced [PrefixElement]s and the [Element]s that they
-  /// prefix.
-  final Map<PrefixElement, List<Element>> prefixMap =
-      new HashMap<PrefixElement, List<Element>>();
+  /// The map of referenced prefix elements and the elements that they prefix.
+  final Map<PrefixElement, List<Element>> prefixMap = {};
+
+  /// The set of referenced top-level elements.
+  final Set<Element> elements = {};
 
-  /// The set of referenced top-level [Element]s.
-  final Set<Element> elements = new HashSet<Element>();
+  /// The set of extensions defining members that are referenced.
+  final Set<ExtensionElement> usedExtensions = {};
 }
diff --git a/pkg/analyzer/test/src/diagnostics/unused_import_test.dart b/pkg/analyzer/test/src/diagnostics/unused_import_test.dart
@@ -2,14 +2,17 @@
 // 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:analyzer/dart/analysis/features.dart';
 import 'package:analyzer/src/error/codes.dart';
+import 'package:analyzer/src/generated/engine.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import '../dart/resolution/driver_resolution.dart';
 
 main() {
   defineReflectiveSuite(() {
     defineReflectiveTests(UnusedImportTest);
+    defineReflectiveTests(UnusedImportWithExtensionMethodsTest);
   });
 }
 
@@ -224,3 +227,169 @@ import 'lib1.dart';
     ]);
   }
 }
+
+@reflectiveTest
+class UnusedImportWithExtensionMethodsTest extends UnusedImportTest {
+  @override
+  AnalysisOptionsImpl get analysisOptions => AnalysisOptionsImpl()
+    ..contextFeatures = new FeatureSet.forTesting(
+        sdkVersion: '2.3.0', additionalFeatures: [Feature.extension_methods]);
+
+  test_instance_call() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on int {
+  int call(int x) => 0;
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  7(9);
+}
+''');
+  }
+
+  test_instance_getter() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  String get empty => '';
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  ''.empty;
+}
+''');
+  }
+
+  test_instance_method() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  String empty() => '';
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  ''.empty();
+}
+''');
+  }
+
+  test_instance_operator_binary() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  String operator -(String s) => this;
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  'abc' - 'c';
+}
+''');
+  }
+
+  test_instance_operator_index() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on int {
+  int operator [](int i) => 0;
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  9[7];
+}
+''');
+  }
+
+  test_instance_operator_unary() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  void operator -() {}
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  -'abc';
+}
+''');
+  }
+
+  test_instance_setter() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  void set length(int i) {}
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  'abc'.length = 2;
+}
+''');
+  }
+
+  test_multipleExtensions() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  String a() => '';
+}
+''');
+    newFile('/test/lib/lib2.dart', content: r'''
+extension E on String {
+  String b() => '';
+}
+''');
+    await assertErrorsInCode('''
+import 'lib1.dart';
+import 'lib2.dart';
+
+f() {
+  ''.b();
+}
+''', [
+      error(HintCode.UNUSED_IMPORT, 7, 11),
+    ]);
+  }
+
+  test_override_getter() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  String get empty => '';
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  E('').empty;
+}
+''');
+  }
+
+  test_static_field() async {
+    newFile('/test/lib/lib1.dart', content: r'''
+extension E on String {
+  static const String empty = '';
+}
+''');
+    await assertNoErrorsInCode('''
+import 'lib1.dart';
+
+f() {
+  E.empty;
+}
+''');
+  }
+}
diff --git a/pkg/analyzer/test/src/diagnostics/unused_shown_name_test.dart b/pkg/analyzer/test/src/diagnostics/unused_shown_name_test.dart
