Add an UNUSED_SHOWN_NAMES hint to the analyzer

pkg/analyzer/lib/src/generated/resolver.dart

Index: pkg/analyzer/lib/src/generated/resolver.dart
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index 0a3fdb82f353d6fb58e828030793f74915074cb5..fe7f196cc6bb8bdf8b593fbb4c78d30cd59b797f 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -4111,7 +4111,12 @@ class GatherUsedImportedElementsVisitor extends RecursiveAstVisitor {
     SimpleIdentifier prefixIdentifier = node.prefix;
     Element element = prefixIdentifier.staticElement;
     if (element is PrefixElement) {
-      usedElements.prefixes.add(element);
+      List<SimpleIdentifier> list = usedElements.prefixes[element];

[Brian Wilkerson, 2016/04/06 15:17:40]

usedElements.prefixes.putIfAbsent(element, () => <SimpleIdentifier>[]);

[srawlins, 2016/04/07 15:27:23]

Done.

+      if (list == null) {
+        list = <SimpleIdentifier>[];
+        usedElements.prefixes[element] = list;
+      }
+      list.add(node.identifier);
       return;
     }
     // Otherwise, pass the prefixed identifier element and name onto
@@ -4145,7 +4150,11 @@ class GatherUsedImportedElementsVisitor extends RecursiveAstVisitor {
       }
       return;
     } else if (element is PrefixElement) {
-      usedElements.prefixes.add(element);
+      List<SimpleIdentifier> list = usedElements.prefixes[element];
+      if (list == null) {
+        list = <SimpleIdentifier>[];
+        usedElements.prefixes[element] = list;
+      }
       return;
     } else if (element.enclosingElement is! CompilationUnitElement) {
       // Identifiers that aren't a prefix element and whose enclosing element
@@ -4375,6 +4384,7 @@ class HintGenerator {
             .removeUsedElements(_usedImportedElementsVisitor.usedElements);
         importsVerifier.generateDuplicateImportHints(definingUnitErrorReporter);
         importsVerifier.generateUnusedImportHints(definingUnitErrorReporter);
+        importsVerifier.generateUnusedShownNameHints(definingUnitErrorReporter);
       }
       _library.accept(new UnusedLocalElementsVerifier(
           _errorListener, _usedLocalElementsVisitor.usedElements));
@@ -4457,20 +4467,23 @@ class ImplicitLabelScope {
 }
 
 /**
- * Instances of the class `ImportsVerifier` visit all of the referenced libraries in the
- * source code verifying that all of the imports are used, otherwise a
- * [HintCode.UNUSED_IMPORT] is generated with
- * [generateUnusedImportHints].
+ * Instances of the class `ImportsVerifier` visit all of the referenced libraries in the source code
+ * verifying that all of the imports are used, otherwise a [HintCode.UNUSED_IMPORT] hint is
+ * generated with [generateUnusedImportHints].
+ *
+ * Additionally, [generateDuplicateImportHints] generates [HintCode.DUPLICATE_IMPORT] hints and
+ * [HintCode.UNUSED_SHOWN_NAME] hints.
  *
  * While this class does not yet have support for an "Organize Imports" action, this logic built up
  * in this class could be used for such an action in the future.
  */
 class ImportsVerifier {
   /**
-   * A list of [ImportDirective]s that the current library imports, as identifiers are visited
-   * by this visitor and an import has been identified as being used by the library, the
-   * [ImportDirective] is removed from this list. After all the sources in the library have
-   * been evaluated, this list represents the set of unused imports.
+   * A list of [ImportDirective]s that the current library imports, but does not use.
+   *
+   * As identifiers are visited by this visitor and an import has been identified as being used
+   * by the library, the [ImportDirective] is removed from this list. After all the sources in the
+   * library have been evaluated, this list represents the set of unused imports.
    *
    * See [ImportsVerifier.generateUnusedImportErrors].
    */
@@ -4483,15 +4496,14 @@ class ImportsVerifier {
   final List<ImportDirective> _duplicateImports = <ImportDirective>[];
 
   /**
-   * This is a map between the set of [LibraryElement]s that the current library imports, and
-   * a list of [ImportDirective]s that imports the library. In cases where the current library
-   * imports a library with a single directive (such as `import lib1.dart;`), the library
+   * This is a map between the set of [LibraryElement]s that the current library imports, and the
+   * list of [ImportDirective]s that import each [LibraryElement]. In cases where the current
+   * library imports a library with a single directive (such as `import lib1.dart;`), the library
    * element will map to a list of one [ImportDirective], which will then be removed from the
    * [unusedImports] list. In cases where the current library imports a library with multiple
-   * directives (such as `import lib1.dart; import lib1.dart show C;`), the
-   * [LibraryElement] will be mapped to a list of the import directives, and the namespace
-   * will need to be used to compute the correct [ImportDirective] being used, see
-   * [namespaceMap].
+   * directives (such as `import lib1.dart; import lib1.dart show C;`), the [LibraryElement] will
+   * be mapped to a list of the import directives, and the namespace will need to be used to
+   * compute the correct [ImportDirective] being used; see [_namespaceMap].
    */
   final HashMap<LibraryElement, List<ImportDirective>> _libraryMap =
       new HashMap<LibraryElement, List<ImportDirective>>();
@@ -4518,44 +4530,62 @@ class ImportsVerifier {
   final HashMap<PrefixElement, List<ImportDirective>> _prefixElementMap =
       new HashMap<PrefixElement, List<ImportDirective>>();
 
+  /**
+   * A map of identifiers that the current library's imports show, but that the library does not
+   * use.
+   *
+   * Each import directive maps to a list of the identifiers that are imported via the "show"
+   * keyword.
+   *
+   * As each identifier is visited by this visitor, it is identified as being used by the library,
+   * and the identifier is removed from this map (under the import that imported it). After all the
+   * sources in the library have been evaluated, each list in this map's values present the set of
+   * unused shown elements.
+   *
+   * See [ImportsVerifier.generateUnusedShownNameHints].
+   */
+  final HashMap<ImportDirective, List<SimpleIdentifier>> _unusedShownNamesMap =
+      new HashMap<ImportDirective, List<SimpleIdentifier>>();
+
   void addImports(CompilationUnit node) {
     for (Directive directive in node.directives) {
       if (directive is ImportDirective) {
         ImportDirective importDirective = directive;
         LibraryElement libraryElement = importDirective.uriElement;
-        if (libraryElement != null) {
-          _unusedImports.add(importDirective);
-          //
-          // Initialize prefixElementMap
-          //
-          if (importDirective.asKeyword != null) {
-            SimpleIdentifier prefixIdentifier = importDirective.prefix;
-            if (prefixIdentifier != null) {
-              Element element = prefixIdentifier.staticElement;
-              if (element is PrefixElement) {
-                PrefixElement prefixElementKey = element;
-                List<ImportDirective> list =
-                    _prefixElementMap[prefixElementKey];
-                if (list == null) {
-                  list = new List<ImportDirective>();
-                  _prefixElementMap[prefixElementKey] = list;
-                }
-                list.add(importDirective);
+        if (libraryElement == null) {
+          continue;

[Brian Wilkerson, 2016/04/06 15:17:40]

FWIW, I actually preferred the code the way it was, but that's just a personal
preference.

[srawlins, 2016/04/07 15:27:23]

Acknowledged.

+        }
+        _unusedImports.add(importDirective);
+        //
+        // Initialize prefixElementMap
+        //
+        if (importDirective.asKeyword != null) {
+          SimpleIdentifier prefixIdentifier = importDirective.prefix;
+          if (prefixIdentifier != null) {
+            Element element = prefixIdentifier.staticElement;
+            if (element is PrefixElement) {
+              PrefixElement prefixElementKey = element;
+              List<ImportDirective> list = _prefixElementMap[prefixElementKey];
+              if (list == null) {
+                list = new List<ImportDirective>();
+                _prefixElementMap[prefixElementKey] = list;
               }
-              // TODO (jwren) Can the element ever not be a PrefixElement?
+              list.add(importDirective);
             }
+            // TODO (jwren) Can the element ever not be a PrefixElement?
           }
-          //
-          // Initialize libraryMap: libraryElement -> importDirective
-          //
-          _putIntoLibraryMap(libraryElement, importDirective);
-          //
-          // For this new addition to the libraryMap, also recursively add any
-          // exports from the libraryElement.
-          //
-          _addAdditionalLibrariesForExports(
-              libraryElement, importDirective, new List<LibraryElement>());
         }
+        //
+        // Initialize libraryMap: libraryElement -> importDirective
+        //
+        _putIntoLibraryMap(libraryElement, importDirective);
+        //
+        // For this new addition to the libraryMap, also recursively add any
+        // exports from the libraryElement.
+        //
+        _addAdditionalLibrariesForExports(
+            libraryElement, importDirective, new List<LibraryElement>());
+        _addShownNames(importDirective);
       }
     }
     if (_unusedImports.length > 1) {
@@ -4598,12 +4628,12 @@ class ImportsVerifier {
   }
 
   /**
-   * After all of the compilation units have been visited by this visitor, this method can be called
-   * to report an [HintCode.UNUSED_IMPORT] hint for each of the import directives in the
-   * [unusedImports] list.
+   * Report an [HintCode.UNUSED_IMPORT] hint for each unused import.
    *
-   * @param errorReporter the error reporter to report the set of [HintCode.UNUSED_IMPORT]
-   *          hints to
+   * Only call this method after all of the compilation units have been visited by this visitor.
+   *
+   * @param errorReporter the error reporter used to report the set of [HintCode.UNUSED_IMPORT]
+   *          hints
    */
   void generateUnusedImportHints(ErrorReporter errorReporter) {
     for (ImportDirective unusedImport in _unusedImports) {
@@ -4621,32 +4651,58 @@ class ImportsVerifier {
   }
 
   /**
+   * Report an [HintCode.UNUSED_SHOWN_NAME] hint for each unused shown name.
+   *
+   * Only call this method after all of the compilation units have been visited by this visitor.
+   *
+   * @param errorReporter the error reporter used to report the set of [HintCode.UNUSED_SHOWN_NAME]
+   *          hints
+   */
+  void generateUnusedShownNameHints(ErrorReporter reporter) {
+    _unusedShownNamesMap.forEach((ImportDirective importDirective,
+        List<SimpleIdentifier> identifiers) {
+      if (_unusedImports.contains(importDirective)) {
+        // This import is actually wholly unused, not just one or more shown names from it.
+        // This is then an "unused import", rather than unused shown names.
+        return;
+      }
+      for (var identifier in identifiers) {

[Brian Wilkerson, 2016/04/06 15:17:40]

nit: we fully type annotate everything in the analyzer package (applies in
multiple places)

[srawlins, 2016/04/07 15:27:23]

Done.

+        reporter.reportErrorForNode(HintCode.UNUSED_SHOWN_NAME, identifier);
+      }
+    });
+  }
+
+  /**
    * Remove elements from [_unusedImports] using the given [usedElements].
    */
   void removeUsedElements(UsedImportedElements usedElements) {
-    // Stop if all the imports are known to be used.
-    if (_unusedImports.isEmpty) {
+    // Stop if all the imports and shown names are known to be used.
+    if (_unusedImports.isEmpty && _unusedShownNamesMap.isEmpty) {
       return;
     }
     // Process import prefixes.
-    for (PrefixElement prefix in usedElements.prefixes) {
+    usedElements.prefixes.forEach((PrefixElement prefix, List<SimpleIdentifier> elements) {
       List<ImportDirective> importDirectives = _prefixElementMap[prefix];
       if (importDirectives != null) {
         for (ImportDirective importDirective in importDirectives) {
           _unusedImports.remove(importDirective);
+          for (SimpleIdentifier element in elements) {
+            _removeFromUnusedShownNamesMap(element.staticElement, importDirective);
+          }
         }
       }
-    }
+    });
     // Process top-level elements.
     for (Element element in usedElements.elements) {
-      // Stop if all the imports are known to be used.
-      if (_unusedImports.isEmpty) {
+      // Stop if all the imports and shown names are known to be used.
+      if (_unusedImports.isEmpty && _unusedShownNamesMap.isEmpty) {
         return;
       }
-      // Prepare import directives for this library.
+      // Prepare import directives for this element's library.
       LibraryElement library = element.library;
       List<ImportDirective> importsLibrary = _libraryMap[library];
       if (importsLibrary == null) {
+        // element's library is not imported. Must be the current library.
         continue;
       }
       // If there is only one import directive for this library, then it must be
@@ -4655,6 +4711,7 @@ class ImportsVerifier {
       if (importsLibrary.length == 1) {
         ImportDirective usedImportDirective = importsLibrary[0];
         _unusedImports.remove(usedImportDirective);
+        _removeFromUnusedShownNamesMap(element, usedImportDirective);
         continue;
       }
       // Otherwise, find import directives using namespaces.
@@ -4663,12 +4720,33 @@ class ImportsVerifier {
         Namespace namespace = _computeNamespace(importDirective);
         if (namespace != null && namespace.get(name) != null) {
           _unusedImports.remove(importDirective);
+          _removeFromUnusedShownNamesMap(element, importDirective);
         }
       }
     }
   }
 
   /**
+   * Remove [element] from the list of names shown by [importDirective].
+   */
+  void _removeFromUnusedShownNamesMap(Element element,
+      ImportDirective importDirective) {
+    List<SimpleIdentifier> list = _unusedShownNamesMap[importDirective];

[scheglov, 2016/04/06 15:34:39]

Could you use a more specific name than "list"?

[srawlins, 2016/04/07 15:27:23]

Done.

+    if (list == null) {
+      return;
+    }
+    for (var identifier in list) {
+      if (identifier.staticElement == element) {
+        list.remove(identifier);
+        break;
+      }
+    }
+    if (list.isEmpty) {
+      _unusedShownNamesMap.remove(importDirective);
+    }
+  }
+
+  /**
    * Recursively add any exported library elements into the [libraryMap].
    */
   void _addAdditionalLibrariesForExports(LibraryElement library,
@@ -4685,9 +4763,10 @@ class ImportsVerifier {
   }
 
   /**
-   * Lookup and return the [Namespace] from the [namespaceMap], if the map does not
-   * have the computed namespace, compute it and cache it in the map. If the import directive is not
-   * resolved or is not resolvable, `null` is returned.
+   * Lookup and return the [Namespace] from the [_namespaceMap].
+   *
+   * If the map does not have the computed namespace, compute it and cache it in the map. If
+   * [importDirective] is not resolved or is not resolvable, `null` is returned.
    *
    * @param importDirective the import directive used to compute the returned namespace
    * @return the computed or looked up [Namespace]
@@ -4722,6 +4801,24 @@ class ImportsVerifier {
     }
     importList.add(importDirective);
   }
+
+  /**
+   * Add every shown name from [importDirective] into [_unusedShownNamesMap].
+   */
+  void _addShownNames(ImportDirective importDirective) {
+    if (importDirective.combinators == null) {
+      return;
+    }
+    List list = new List<SimpleIdentifier>();

[scheglov, 2016/04/06 15:34:39]

1. Add type arguments.
2. Use a better name.

[srawlins, 2016/04/07 15:27:23]

Done.

+    _unusedShownNamesMap[importDirective] = list;
+    for (Combinator combinator in importDirective.combinators) {
+      if (combinator is ShowCombinator) {
+        for (SimpleIdentifier name in combinator.shownNames) {
+          list.add(name);
+        }
+      }
+    }
+  }
 }
 
 /**
@@ -12568,7 +12665,8 @@ class UsedImportedElements {
   /**
    * The set of referenced [PrefixElement]s.
    */
-  final Set<PrefixElement> prefixes = new HashSet<PrefixElement>();
+  final Map<PrefixElement, List<SimpleIdentifier>> prefixes =

[scheglov, 2016/04/06 15:34:39]

1. Instances of the class UsedImportedElements are put into the analysis cache.
It is a BAD idea to put AST nodes into the cache. Use elements instead.

2. Update the name and comment.

[srawlins, 2016/04/07 15:27:23]

Done.

+      new HashMap<PrefixElement, List<SimpleIdentifier>>();
 
   /**
    * The set of referenced top-level [Element]s.