Add an UNUSED_SHOWN_NAMES hint to the analyzer

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

 // Copyright (c) 2014, 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.
 
 library analyzer.src.generated.resolver;
 
 import 'dart:collection';
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
@@ skipping 4093 matching lines @@
   }
 
   @override
   void visitPrefixedIdentifier(PrefixedIdentifier node) {
     // If the prefixed identifier references some A.B, where A is a library
     // prefix, then we can lookup the associated ImportDirective in
     // prefixElementMap and remove it from the unusedImports list.
     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
     // visitIdentifier.
     _visitIdentifier(element, prefixIdentifier.name);
   }
 
   @override
   void visitSimpleIdentifier(SimpleIdentifier node) {
     _visitIdentifier(node.staticElement, node.name);
@@ skipping 13 matching lines @@
     }
     // If the element is multiply defined then call this method recursively for
     // each of the conflicting elements.
     if (element is MultiplyDefinedElement) {
       MultiplyDefinedElement multiplyDefinedElement = element;
       for (Element elt in multiplyDefinedElement.conflictingElements) {
         _visitIdentifier(elt, name);
       }
       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
       // 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;
@@ skipping 209 matching lines @@
       CompilationUnit definingUnit = _compilationUnits[0];
       ErrorReporter definingUnitErrorReporter =
           new ErrorReporter(_errorListener, definingUnit.element.source);
       {
         ImportsVerifier importsVerifier = new ImportsVerifier();
         importsVerifier.addImports(definingUnit);
         importsVerifier
             .removeUsedElements(_usedImportedElementsVisitor.usedElements);
         importsVerifier.generateDuplicateImportHints(definingUnitErrorReporter);
         importsVerifier.generateUnusedImportHints(definingUnitErrorReporter);
+        importsVerifier.generateUnusedShownNameHints(definingUnitErrorReporter);
       }
       _library.accept(new UnusedLocalElementsVerifier(
           _errorListener, _usedLocalElementsVisitor.usedElements));
     });
   }
 
   void _generateForCompilationUnit(CompilationUnit unit, Source source) {
     ErrorReporter errorReporter = new ErrorReporter(_errorListener, source);
     unit.accept(_usedImportedElementsVisitor);
     // dead code analysis
@@ skipping 62 matching lines @@
   /**
    * Initialize a newly created scope to represent a switch statement or loop
    * nested within the current scope.  [statement] is the statement associated
    * with the newly created scope.
    */
   ImplicitLabelScope nest(Statement statement) =>
       new ImplicitLabelScope._(this, statement);
 }
 
 /**
- * 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].
    */
   final List<ImportDirective> _unusedImports = <ImportDirective>[];
 
   /**
    * After the list of [unusedImports] has been computed, this list is a proper subset of the
    * unused imports that are listed more than once.
    */
   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>>();
 
   /**
    * In cases where there is more than one import directive per library element, this mapping is
    * used to determine which of the multiple import directives are used by generating a
    * [Namespace] for each of the imports to do lookups in the same way that they are done from
    * the [ElementResolver].
    */
   final HashMap<ImportDirective, Namespace> _namespaceMap =
       new HashMap<ImportDirective, Namespace>();
 
   /**
    * This is a map between prefix elements and the import directives from which they are derived. In
    * cases where a type is referenced via a prefix element, the import directive can be marked as
    * used (removed from the unusedImports) by looking at the resolved `lib` in `lib.X`,
    * instead of looking at which library the `lib.X` resolves.
    *
    * TODO (jwren) Since multiple [ImportDirective]s can share the same [PrefixElement],
    * it is possible to have an unreported unused import in situations where two imports use the same
    * prefix and at least one import directive is used.
    */
   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) {
       // order the list of unusedImports to find duplicates in faster than
       // O(n^2) time
       List<ImportDirective> importDirectiveArray =
           new List<ImportDirective>.from(_unusedImports);
       importDirectiveArray.sort(ImportDirective.COMPARATOR);
       ImportDirective currentDirective = importDirectiveArray[0];
       for (int i = 1; i < importDirectiveArray.length; i++) {
@@ skipping 22 matching lines @@
    *          hints to
    */
   void generateDuplicateImportHints(ErrorReporter errorReporter) {
     for (ImportDirective duplicateImport in _duplicateImports) {
       errorReporter.reportErrorForNode(
           HintCode.DUPLICATE_IMPORT, duplicateImport.uri);
     }
   }
 
   /**
-   * 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) {
       // Check that the import isn't dart:core
       ImportElement importElement = unusedImport.element;
       if (importElement != null) {
         LibraryElement libraryElement = importElement.importedLibrary;
         if (libraryElement != null && libraryElement.isDartCore) {
           continue;
         }
       }
       errorReporter.reportErrorForNode(
           HintCode.UNUSED_IMPORT, unusedImport.uri);
     }
   }
 
   /**
+   * 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
       // the directive that this element is imported with, remove it from the
       // unusedImports list.
       if (importsLibrary.length == 1) {
         ImportDirective usedImportDirective = importsLibrary[0];
         _unusedImports.remove(usedImportDirective);
+        _removeFromUnusedShownNamesMap(element, usedImportDirective);
         continue;
       }
       // Otherwise, find import directives using namespaces.
       String name = element.displayName;
       for (ImportDirective importDirective in importsLibrary) {
         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,
       ImportDirective importDirective, List<LibraryElement> exportPath) {
     if (exportPath.contains(library)) {
       return;
     }
     exportPath.add(library);
     for (LibraryElement exportedLibraryElt in library.exportedLibraries) {
       _putIntoLibraryMap(exportedLibraryElt, importDirective);
       _addAdditionalLibrariesForExports(
           exportedLibraryElt, importDirective, exportPath);
     }
   }
 
   /**
-   * 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]
    */
   Namespace _computeNamespace(ImportDirective importDirective) {
     Namespace namespace = _namespaceMap[importDirective];
     if (namespace == null) {
       // If the namespace isn't in the namespaceMap, then compute and put it in
       // the map.
       ImportElement importElement = importDirective.element;
@@ skipping 14 matching lines @@
    */
   void _putIntoLibraryMap(
       LibraryElement libraryElement, ImportDirective importDirective) {
     List<ImportDirective> importList = _libraryMap[libraryElement];
     if (importList == null) {
       importList = new List<ImportDirective>();
       _libraryMap[libraryElement] = importList;
     }
     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);
+        }
+      }
+    }
+  }
 }
 
 /**
  * Maintains and manages contextual type information used for
  * inferring types.
  */
 class InferenceContext {
   // TODO(leafp): Consider replacing these node properties with a
   // hash table help in an instance of this class.
   static const String _typeProperty =
@@ skipping 7826 matching lines @@
 }
 
 /**
  * A container with information about used imports prefixes and used imported
  * elements.
  */
 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.
    */
   final Set<Element> elements = new HashSet<Element>();
 }
 
 /**
  * A container with sets of used [Element]s.
  * All these elements are defined in a single compilation unit or a library.
@@ skipping 395 matching lines @@
     nonFields.add(node);
     return null;
   }
 
   @override
   Object visitNode(AstNode node) => node.accept(TypeResolverVisitor_this);
 
   @override
   Object visitWithClause(WithClause node) => null;
 }