[analyzer] Adds new warning for multiple combinators

Fixes: #56879
Change-Id: Ifd53878e3603090efa7905b3cd021b8a63344c26
Tested: trybots
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/413520
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Auto-Submit: Felipe Morschel <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Paul Berry <[email protected]>

Author: FMorschel

pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

@@ -3656,6 +3656,10 @@ WarningCode.MIXIN_ON_SEALED_CLASS:
   notes: |-
     `sealed` annotation is likely to be replaced by the `sealed` keyword
     in the next stable release.
+WarningCode.MULTIPLE_COMBINATORS:
+  status: needsFix
+  notes: |-
+    Aggregate the combinators into a single one.
 WarningCode.MUST_BE_IMMUTABLE:
   status: noFix
 WarningCode.MUST_CALL_SUPER:

pkg/analysis_server/test/services/refactoring/legacy/rename_import_test.dart

@@ -55,6 +55,7 @@ void f(Random r) {}
   Future<void> test_createChange_add() async {
     await indexTestUnit('''
 import 'dart:async';
+// ignore: multiple_combinators
 import 'dart:math' show Random, min hide max;
 void f() {
   Future f;
@@ -69,6 +70,7 @@ void f() {
     // validate change
     return assertSuccessfulRefactoring('''
 import 'dart:async';
+// ignore: multiple_combinators
 import 'dart:math' as newName show Random, min hide max;
 void f() {
   Future f;

pkg/analysis_server/test/src/services/correction/fix/ambiguous_import_fix_test.dart

@@ -453,6 +453,7 @@ void foo() {}''');
     newFile(join(testPackageLibPath, 'lib2.dart'), '''
 void bar() {}''');
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'lib1.dart' hide baz hide foo;
 import 'lib2.dart';
 
@@ -461,6 +462,7 @@ void foo() {
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'lib1.dart' hide baz, bar hide foo, bar;
 import 'lib2.dart';
 
@@ -873,6 +875,7 @@ class O {}''');
     newFile(join(testPackageLibPath, 'lib2.dart'), '''
 class N {}''');
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'lib1.dart' show N show N, O, M;
 import 'lib2.dart' show N;
 
@@ -881,6 +884,7 @@ void f(N? n) {
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'lib1.dart' show O, M;
 import 'lib2.dart' show N;
 
@@ -896,6 +900,7 @@ class N {}''');
     newFile(join(testPackageLibPath, 'lib2.dart'), '''
 class N {}''');
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'lib1.dart' show N show N;
 import 'lib2.dart' show N;
 
@@ -904,6 +909,7 @@ void f(N? n) {
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'lib1.dart' hide N;
 import 'lib2.dart' show N;
 
@@ -920,6 +926,7 @@ class N {}''');
     newFile(join(testPackageLibPath, 'lib2.dart'), '''
 class N {}''');
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'lib1.dart' show N hide M hide M;
 import 'lib2.dart' show N;
 
@@ -928,6 +935,7 @@ void f(N? n) {
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'lib1.dart' hide M, N hide M, N;
 import 'lib2.dart' show N;
 

pkg/analysis_server/test/src/services/correction/fix/remove_name_from_combinator_test.dart

@@ -180,13 +180,15 @@ f(x) {
 
   Future<void> test_undefinedHiddenName_only_first() async {
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'dart:math' hide aaa hide cos, sin;
 
 void f() {
   print(min(0, 1));
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'dart:math' hide cos, sin;
 
 void f() {
@@ -197,13 +199,15 @@ void f() {
 
   Future<void> test_undefinedHiddenName_only_last() async {
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'dart:math' hide cos, sin hide aaa;
 
 void f() {
   print(min(0, 1));
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'dart:math' hide cos, sin;
 
 void f() {
@@ -214,13 +218,15 @@ void f() {
 
   Future<void> test_undefinedHiddenName_only_middle() async {
     await resolveTestCode('''
+// ignore: multiple_combinators
 import 'dart:math' hide cos hide aaa hide sin;
 
 void f() {
   print(min(0, 1));
 }
 ''');
     await assertHasFix('''
+// ignore: multiple_combinators
 import 'dart:math' hide cos hide sin;
 
 void f() {

pkg/analysis_server/test/src/utilities/selection_test.dart

@@ -556,6 +556,7 @@ enum E { a }
 int a, b, c, d;
 ''');
     var selection = await _computeSelection('''
+// ignore: multiple_combinators
 export 'a.dart' show a [!hide b show c!] hide d;
 ''');
     _assertSelection(selection, r'''
@@ -790,6 +791,7 @@ nodesInRange
 int a, b, c, d;
 ''');
     var selection = await _computeSelection('''
+// ignore: multiple_combinators
 import 'a.dart' show a [!hide b show c!] hide d;
 ''');
     _assertSelection(selection, r'''

pkg/analyzer/lib/src/error/codes.g.dart

@@ -7043,6 +7043,14 @@ class WarningCode extends ErrorCode {
     hasPublishedDocs: true,
   );
 
+  ///  No parameters.
+  static const WarningCode MULTIPLE_COMBINATORS = WarningCode(
+    'MULTIPLE_COMBINATORS',
+    "Using multiple 'hide' or 'show' combinators is never necessary and often "
+        "produces surprising results.",
+    correctionMessage: "Try using a single combinator.",
+  );
+
   ///  Generates a warning for classes that inherit from classes annotated with
   ///  `@immutable` but that are not immutable.
   ///

pkg/analyzer/lib/src/error/error_code_values.g.dart

@@ -1052,6 +1052,7 @@ const List<ErrorCode> errorCodeValues = [
   WarningCode.MISSING_REQUIRED_PARAM,
   WarningCode.MISSING_REQUIRED_PARAM_WITH_DETAILS,
   WarningCode.MIXIN_ON_SEALED_CLASS,
+  WarningCode.MULTIPLE_COMBINATORS,
   WarningCode.MUST_BE_IMMUTABLE,
   WarningCode.MUST_CALL_SUPER,
   WarningCode.NON_CONST_ARGUMENT_FOR_CONST_PARAMETER,

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

@@ -157,6 +157,9 @@ class EnclosingExecutableContext {
 /// and warnings not covered by the parser and resolver.
 class ErrorVerifier extends RecursiveAstVisitor<void>
     with ErrorDetectionHelpers {
+  /// The factory used to create diagnostic messages.
+  static final _diagnosticFactory = DiagnosticFactory();
+
   /// The error reporter by which errors will be reported.
   @override
   final ErrorReporter errorReporter;
@@ -732,6 +735,7 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
       _checkForAmbiguousExport(node, libraryExport, exportedLibrary);
       _checkForExportInternalLibrary(node, libraryExport);
     }
+    _reportForMultipleCombinators(node);
     super.visitExportDirective(node);
   }
 
@@ -1078,6 +1082,8 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
         _checkForDeferredImportOfExtensions(node, importElement);
       }
     }
+
+    _reportForMultipleCombinators(node);
     super.visitImportDirective(node);
   }
 
@@ -5203,7 +5209,7 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
         _hiddenElements!.contains(element)) {
       _hiddenElements!.contains(element);
       errorReporter.reportError(
-        DiagnosticFactory().referencedBeforeDeclaration(
+        _diagnosticFactory.referencedBeforeDeclaration(
           errorReporter.source,
           nameToken: nameToken,
           element2: element,
@@ -5689,8 +5695,8 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
       if (errorCode == StaticWarningCode.INVALID_NULL_AWARE_OPERATOR) {
         var previousOperator = previousShortCircuitingOperator(target);
         if (previousOperator != null) {
-          errorReporter.reportError(DiagnosticFactory()
-              .invalidNullAwareAfterShortCircuit(
+          errorReporter.reportError(
+              _diagnosticFactory.invalidNullAwareAfterShortCircuit(
                   errorReporter.source,
                   operator.offset,
                   endToken.end - operator.offset,
@@ -6481,6 +6487,19 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
     return null;
   }
 
+  void _reportForMultipleCombinators(NamespaceDirective node) {
+    var combinators = node.combinators;
+    if (combinators.length > 1) {
+      var offset = combinators.beginToken!.offset;
+      var length = combinators.endToken!.end - offset;
+      errorReporter.atOffset(
+        offset: offset,
+        length: length,
+        errorCode: WarningCode.MULTIPLE_COMBINATORS,
+      );
+    }
+  }
+
   void _withEnclosingExecutable(
     ExecutableElement2OrMember element,
     void Function() operation, {

pkg/analyzer/messages.yaml

@@ -27704,6 +27704,73 @@ WarningCode:
       }
       ```
       TODO(brianwilkerson): Highlight the identifier without the colon.
+  MULTIPLE_COMBINATORS:
+    problemMessage: "Using multiple 'hide' or 'show' combinators is never necessary and often produces surprising results."
+    correctionMessage: "Try using a single combinator."
+    hasPublishedDocs: false
+    comment: No parameters.
+    documentation: |-
+      #### Description
+
+      The analyzer produces this diagnostic when an import or export directive
+      contains more than one combinator.
+
+      #### Examples
+
+      The following code produces this diagnostic because the second `show`
+      combinator hides `List` and `int`:
+
+      ```dart
+      import 'dart:core' [!show Future, List, int show Future!];
+
+      var x = Future.value(1);
+      ```
+
+      The following code produces this diagnostic because the second `hide` combinator
+      is redundant:
+
+      ```dart
+      import 'dart:math' [!hide Random, max, min hide min!];
+
+      var x = pi;
+      ```
+
+      The following codes produce this diagnostic because the `hide` combinator
+      is redundant:
+
+      ```dart
+      import 'dart:math' [!show Random, max hide min!];
+
+      var x = max(0, 1);
+      var r = Random();
+      ```
+
+      The following code produces this diagnostic because the `show` combinator
+      already hides `Random` and `max`, so the `hide` combinator is redundant:
+
+      ```dart
+      import 'dart:math' [!hide Random, max show min!];
+
+      var x = min(0, 1);
+      ```
+
+      #### Common fixes
+
+      If you prefer to list the names that should be visible, then use a single `show` combinator:
+
+      ```dart
+      import 'dart:math' show min;
+
+      var x = min(0, 1);
+      ```
+
+      If you prefer to list the names that should be hidden, then use a single `hide` combinator:
+
+      ```dart
+      import 'dart:math' hide Random, max, min;
+
+      var x = pi;
+      ```
   UNUSED_LOCAL_VARIABLE:
     problemMessage: "The value of the local variable '{0}' isn't used."
     correctionMessage: Try removing the variable or using it.

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

@@ -135,7 +135,9 @@ M.A a = M.A();
 
     await assertErrorsInFile2(lib1, []);
     await assertErrorsInFile2(lib2, [
+      error(WarningCode.MULTIPLE_COMBINATORS, 35, 13),
       error(WarningCode.DUPLICATE_IMPORT, 57, 11),
+      error(WarningCode.MULTIPLE_COMBINATORS, 74, 13),
     ]);
   }