Issue 48987. Report DUPLICATE_IMPORT even when URIs are not syntactic…

…ally identical.

Bug: #48987
Change-Id: I88dfcd7ce4901933998ddd11724c3a47bec9a784
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/244183
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

pkg/analyzer/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## 4.2.0-dev
 * Update SDK constraints to `>=2.17.0 <3.0.0`.
+* Deprecated `ImportDirective.COMPARATOR`, use appropriate custom logic, if necessary.
 
 ## 4.1.0
 * Deprecated `ParameterElement.isNotOptional`, use `isRequired` instead.

pkg/analyzer/lib/dart/ast/ast.dart

@@ -2733,6 +2733,7 @@ abstract class ImplicitCallReference implements MethodReferenceExpression {
 ///
 /// Clients may not extend, implement or mix-in this class.
 abstract class ImportDirective implements NamespaceDirective {
+  @Deprecated('This kind of syntactic equality is rarely useful')
   static Comparator<ImportDirective> COMPARATOR =
       (ImportDirective import1, ImportDirective import2) {
     //

pkg/analyzer/lib/src/dart/ast/ast.dart

@@ -6383,6 +6383,58 @@ class ImportDirectiveImpl extends NamespaceDirectiveImpl
     _prefix?.accept(visitor);
     combinators.accept(visitor);
   }
+
+  /// Return `true` if the non-URI components of the two directives are
+  /// syntactically identical. URIs are checked outside to see if they resolve
+  /// to the same absolute URI, so to the same library, regardless of the used
+  /// syntax (absolute, relative, not normalized).
+  static bool areSyntacticallyIdenticalExceptUri(
+    ImportDirective node1,
+    ImportDirective node2,
+  ) {
+    if (node1.prefix?.name != node2.prefix?.name) {
+      return false;
+    }
+
+    bool areSameNames(
+      List<SimpleIdentifier> names1,
+      List<SimpleIdentifier> names2,
+    ) {
+      if (names1.length != names2.length) {
+        return false;
+      }
+      for (var i = 0; i < names1.length; i++) {
+        if (names1[i].name != names2[i].name) {
+          return false;
+        }
+      }
+      return true;
+    }
+
+    final combinators1 = node1.combinators;
+    final combinators2 = node2.combinators;
+    if (combinators1.length != combinators2.length) {
+      return false;
+    }
+    for (var i = 0; i < combinators1.length; i++) {
+      final combinator1 = combinators1[i];
+      final combinator2 = combinators2[i];
+      if (combinator1 is HideCombinator && combinator2 is HideCombinator) {
+        if (!areSameNames(combinator1.hiddenNames, combinator2.hiddenNames)) {
+          return false;
+        }
+      } else if (combinator1 is ShowCombinator &&
+          combinator2 is ShowCombinator) {
+        if (!areSameNames(combinator1.shownNames, combinator2.shownNames)) {
+          return false;
+        }
+      } else {
+        return false;
+      }
+    }
+
+    return true;
+  }
 }
 
 /// An index expression.

pkg/analyzer/lib/src/error/imports_verifier.dart

@@ -8,6 +8,7 @@ import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/error/listener.dart';
+import 'package:analyzer/src/dart/ast/ast.dart';
 import 'package:analyzer/src/dart/resolver/scope.dart';
 import 'package:analyzer/src/error/codes.dart';
 
@@ -254,6 +255,7 @@ class ImportsVerifier {
       HashMap<NamespaceDirective, List<SimpleIdentifier>>();
 
   void addImports(CompilationUnit node) {
+    final importsWithLibraries = <_ImportDirective>[];
     for (Directive directive in node.directives) {
       if (directive is ImportDirective) {
         var libraryElement = directive.uriElement;
@@ -262,6 +264,12 @@ class ImportsVerifier {
         }
         _allImports.add(directive);
         _unusedImports.add(directive);
+        importsWithLibraries.add(
+          _ImportDirective(
+            node: directive,
+            importedLibrary: libraryElement,
+          ),
+        );
         //
         // Initialize prefixElementMap
         //
@@ -286,23 +294,27 @@ class ImportsVerifier {
         _addDuplicateShownHiddenNames(directive);
       }
     }
-    if (_unusedImports.length > 1) {
+    if (importsWithLibraries.length > 1) {
       // order the list of unusedImports to find duplicates in faster than
       // O(n^2) time
-      List<ImportDirective> importDirectiveArray =
-          List<ImportDirective>.from(_unusedImports);
-      importDirectiveArray.sort(ImportDirective.COMPARATOR);
-      ImportDirective currentDirective = importDirectiveArray[0];
-      for (int i = 1; i < importDirectiveArray.length; i++) {
-        ImportDirective nextDirective = importDirectiveArray[i];
-        if (ImportDirective.COMPARATOR(currentDirective, nextDirective) == 0) {
+      importsWithLibraries.sort((import1, import2) {
+        return import1.libraryUriStr.compareTo(import2.libraryUriStr);
+      });
+      var currentDirective = importsWithLibraries[0];
+      for (var i = 1; i < importsWithLibraries.length; i++) {
+        final nextDirective = importsWithLibraries[i];
+        if (currentDirective.libraryUriStr == nextDirective.libraryUriStr &&
+            ImportDirectiveImpl.areSyntacticallyIdenticalExceptUri(
+              currentDirective.node,
+              nextDirective.node,
+            )) {
           // Add either the currentDirective or nextDirective depending on which
           // comes second, this guarantees that the first of the duplicates
           // won't be highlighted.
-          if (currentDirective.offset < nextDirective.offset) {
-            _duplicateImports.add(nextDirective);
+          if (currentDirective.node.offset < nextDirective.node.offset) {
+            _duplicateImports.add(nextDirective.node);
           } else {
-            _duplicateImports.add(currentDirective);
+            _duplicateImports.add(currentDirective.node);
           }
         }
         currentDirective = nextDirective;
@@ -603,6 +615,20 @@ class UsedImportedElements {
   final Set<ExtensionElement> usedExtensions = {};
 }
 
+/// [ImportDirective] with non-null imported [LibraryElement].
+class _ImportDirective {
+  final ImportDirective node;
+  final LibraryElement importedLibrary;
+
+  _ImportDirective({
+    required this.node,
+    required this.importedLibrary,
+  });
+
+  /// Returns the absolute URI of the imported library.
+  String get libraryUriStr => '${importedLibrary.source.uri}';
+}
+
 /// A class which verifies (and reports) whether any import directives are
 /// unnecessary.
 ///

pkg/analyzer/test/src/diagnostics/duplicate_import_test.dart

@@ -16,20 +16,48 @@ main() {
 
 @reflectiveTest
 class DuplicateImportTest extends PubPackageResolutionTest {
-  test_duplicateImport() async {
-    newFile('$testPackageLibPath/lib1.dart', r'''
-library lib1;
-class A {}''');
+  test_duplicateImport_absolute_absolute() async {
+    newFile('$testPackageLibPath/a.dart', r'''
+class A {}
+''');
 
-    newFile('$testPackageLibPath/lib2.dart', r'''
-library L;
-import 'lib1.dart';
-import 'lib1.dart';
-A a = A();''');
+    await assertErrorsInCode(r'''
+import 'package:test/a.dart';
+import 'package:test/a.dart';
 
-    await _resolveFile('$testPackageLibPath/lib1.dart');
-    await _resolveFile('$testPackageLibPath/lib2.dart', [
-      error(HintCode.DUPLICATE_IMPORT, 38, 11),
+final a = A();
+''', [
+      error(HintCode.DUPLICATE_IMPORT, 37, 21),
+    ]);
+  }
+
+  test_duplicateImport_relative_absolute() async {
+    newFile('$testPackageLibPath/a.dart', r'''
+class A {}
+''');
+
+    await assertErrorsInCode(r'''
+import 'a.dart';
+import 'package:test/a.dart';
+
+final a = A();
+''', [
+      error(HintCode.DUPLICATE_IMPORT, 24, 21),
+    ]);
+  }
+
+  test_duplicateImport_relative_relative() async {
+    newFile('$testPackageLibPath/a.dart', r'''
+class A {}
+''');
+
+    await assertErrorsInCode(r'''
+import 'a.dart';
+import 'a.dart';
+
+final a = A();
+''', [
+      error(HintCode.DUPLICATE_IMPORT, 24, 8),
     ]);
   }