Report type parameters whose name is a type in scope

See #59517 for background and discussion.

Change-Id: I6d02d07cdd95e8b2b647cf3f7f87a9f27a1c48b3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381841
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Erik Ernst <[email protected]>
Reviewed-by: Paul Berry <[email protected]>

Author: eernstg

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

@@ -1985,7 +1985,11 @@ LintCode.avoid_type_to_string:
   notes: |-
     Theoretically we could replace some uses with `is` checks, but it's probably
     uncommon enough to not be worth the cost.
-LintCode.avoid_types_as_parameter_names:
+LintCode.avoid_types_as_parameter_names_type_parameter:
+  status: noFix
+  notes: |-
+    The type variable needs to be renamed, but that's a refactoring.
+LintCode.avoid_types_as_parameter_names_formal_parameter:
   status: hasFix
 LintCode.avoid_types_on_closure_parameters:
   status: hasFix

pkg/analysis_server/lib/src/services/correction/fix_internal.dart

@@ -311,7 +311,9 @@ final _builtInLintGenerators = <LintCode, List<ProducerGenerator>>{
     //  diagnostics and should also be available as an assist.
     ReplaceCascadeWithDot.new,
   ],
-  LinterLintCode.avoid_types_as_parameter_names: [ConvertToOnType.new],
+  LinterLintCode.avoid_types_as_parameter_names_formal_parameter: [
+    ConvertToOnType.new,
+  ],
   LinterLintCode.avoid_types_on_closure_parameters: [
     ReplaceWithIdentifier.new,
     RemoveTypeAnnotation.other,

pkg/analyzer/lib/src/fine/library_manifest.dart

@@ -708,9 +708,9 @@ class LibraryManifestBuilder {
 
   /// Returns either the existing item from [declaredItems], or builds a new one.
   Item _getOrBuildElementItem<
-    Element extends ElementImpl2,
+    E extends ElementImpl2,
     Item extends ManifestItem
-  >(Element element, Item Function() build) {
+  >(E element, Item Function() build) {
     // We assume that when matching elements against the structure of
     // the item, we put into [itemMap] only the type of the item that
     // corresponds the type of the element.

pkg/analyzer/test/generated/error_suppression_test.dart

@@ -234,7 +234,13 @@ int y = (0 as int);
 // ignore: type=wrong
 void f(arg1(int)) {} // AVOID_TYPES_AS_PARAMETER_NAMES
 ''',
-      [error(LinterLintCode.avoid_types_as_parameter_names, 34, 3)],
+      [
+        error(
+          LinterLintCode.avoid_types_as_parameter_names_formal_parameter,
+          34,
+          3,
+        ),
+      ],
     );
   }
 

pkg/linter/lib/src/lint_codes.g.dart

@@ -360,13 +360,25 @@ class LinterLintCode extends LintCode {
     hasPublishedDocs: true,
   );
 
-  static const LintCode avoid_types_as_parameter_names = LinterLintCode(
+  static const LintCode
+  avoid_types_as_parameter_names_formal_parameter = LinterLintCode(
     LintNames.avoid_types_as_parameter_names,
     "The parameter name '{0}' matches a visible type name.",
     correctionMessage:
         "Try adding a name for the parameter or changing the parameter name to "
         "not match an existing type.",
     hasPublishedDocs: true,
+    uniqueName: 'avoid_types_as_parameter_names_formal_parameter',
+  );
+
+  static const LintCode
+  avoid_types_as_parameter_names_type_parameter = LinterLintCode(
+    LintNames.avoid_types_as_parameter_names,
+    "The type parameter name '{0}' matches a visible type name.",
+    correctionMessage:
+        "Try changing the type parameter name to not match an existing type.",
+    hasPublishedDocs: true,
+    uniqueName: 'avoid_types_as_parameter_names_type_parameter',
   );
 
   static const LintCode avoid_types_on_closure_parameters = LinterLintCode(

pkg/linter/lib/src/rules/avoid_types_as_parameter_names.dart

@@ -19,7 +19,10 @@ class AvoidTypesAsParameterNames extends LintRule {
     : super(name: LintNames.avoid_types_as_parameter_names, description: _desc);
 
   @override
-  LintCode get lintCode => LinterLintCode.avoid_types_as_parameter_names;
+  List<LintCode> get lintCodes => [
+    LinterLintCode.avoid_types_as_parameter_names_type_parameter,
+    LinterLintCode.avoid_types_as_parameter_names_formal_parameter,
+  ];
 
   @override
   void registerNodeProcessors(
@@ -29,6 +32,7 @@ class AvoidTypesAsParameterNames extends LintRule {
     var visitor = _Visitor(this, context);
     registry.addFormalParameterList(this, visitor);
     registry.addCatchClause(this, visitor);
+    registry.addTypeParameterList(this, visitor);
   }
 }
 
@@ -42,7 +46,12 @@ class _Visitor extends SimpleAstVisitor<void> {
   void visitCatchClause(CatchClause node) {
     var parameter = node.exceptionParameter;
     if (parameter != null && _isTypeName(node, parameter.name)) {
-      rule.reportAtNode(parameter, arguments: [parameter.name.lexeme]);
+      rule.reportAtNode(
+        parameter,
+        arguments: [parameter.name.lexeme],
+        errorCode:
+            LinterLintCode.avoid_types_as_parameter_names_formal_parameter,
+      );
     }
   }
 
@@ -56,19 +65,76 @@ class _Visitor extends SimpleAstVisitor<void> {
           declaredElement.hasImplicitType &&
           name != null &&
           _isTypeName(node, name)) {
-        rule.reportAtToken(name, arguments: [name.lexeme]);
+        rule.reportAtToken(
+          name,
+          arguments: [name.lexeme],
+          errorCode:
+              LinterLintCode.avoid_types_as_parameter_names_formal_parameter,
+        );
       }
     }
   }
 
-  bool _isTypeName(AstNode scope, Token name) {
+  @override
+  void visitTypeParameterList(TypeParameterList node) {
+    for (var typeParameter in node.typeParameters) {
+      var declaredElement = typeParameter.declaredFragment?.element;
+      var name = typeParameter.name;
+      var scope = node.parent;
+      var isShadowedByTypeParameter = false;
+      // Step out into enclosing scope where this type parameter isn't
+      // itself in scope (the type parameter doesn't shadow itself).
+      while (scope != null) {
+        if (scope is ClassDeclaration ||
+            scope is FunctionDeclaration ||
+            (scope is FunctionExpression &&
+                scope.parent is! FunctionDeclaration) ||
+            scope is GenericFunctionType ||
+            scope is MethodDeclaration) {
+          if (scope is MethodDeclaration) {
+            isShadowedByTypeParameter = true;
+          }
+          scope = scope.parent;
+          break;
+        }
+        scope = scope.parent;
+      }
+      if (declaredElement != null &&
+          scope != null &&
+          _isTypeName(
+            scope,
+            name,
+            isShadowedByMethodTypeParameter: isShadowedByTypeParameter,
+          )) {
+        rule.reportAtToken(
+          name,
+          arguments: [name.lexeme],
+          errorCode:
+              LinterLintCode.avoid_types_as_parameter_names_type_parameter,
+        );
+      }
+    }
+  }
+
+  bool _isTypeName(
+    AstNode scope,
+    Token name, {
+    bool isShadowedByMethodTypeParameter = false,
+  }) {
     var result = resolveNameInScope(
       name.lexeme,
       scope,
       shouldResolveSetter: false,
     );
     if (result.isRequestedName) {
       var element = result.element;
+      if (isShadowedByMethodTypeParameter && element is TypeParameterElement) {
+        // A type parameter of a static method can only shadow another type
+        // parameter when the latter is a type parameter of the enclosing
+        // declaration (class, mixin, mixin class, enum, extension, or
+        // extension type). We do not lint this case.
+        return false;
+      }
       return element is ClassElement ||
           element is ExtensionTypeElement ||
           element is TypeAliasElement ||

pkg/linter/messages.yaml

@@ -2347,11 +2347,17 @@ LintCode:
         return getThingFromDatabase(key: myThing.thingTypeKey);
       }
       ```
-  avoid_types_as_parameter_names:
+  avoid_types_as_parameter_names_formal_parameter:
+    sharedName: avoid_types_as_parameter_names
     problemMessage: "The parameter name '{0}' matches a visible type name."
     correctionMessage: "Try adding a name for the parameter or changing the parameter name to not match an existing type."
+    hasPublishedDocs: true
+  avoid_types_as_parameter_names_type_parameter:
+    sharedName: avoid_types_as_parameter_names
+    problemMessage: "The type parameter name '{0}' matches a visible type name."
     state:
       stable: "2.0"
+    correctionMessage: "Try changing the type parameter name to not match an existing type."
     categories: [unintentional]
     hasPublishedDocs: true
     documentation: |-
@@ -2368,6 +2374,11 @@ LintCode:
       shadow the existing type, which can lead to bugs that are difficult to
       diagnose.
 
+      The analyzer also produces this diagnostic when the name of a type
+      parameter in a type parameter list is the same as a type whose name is
+      in scope. It is again recommended that the type parameter is renamed
+      such that the error-prone shadowing is avoided.
+
       #### Example
 
       The following code produces this diagnostic because the function `f` has a

pkg/linter/test/rules/avoid_types_as_parameter_names_test.dart

@@ -30,6 +30,22 @@ void f() {
     );
   }
 
+  test_classTypeParameter_instanceMethodTypeParameter() async {
+    await assertNoDiagnostics(r'''
+class A<X> {
+  X instanceMethod<X>(X x) => x;
+}
+''');
+  }
+
+  test_classTypeParameter_staticMethodTypeParameter() async {
+    await assertNoDiagnostics(r'''
+class A<X> {
+  static X staticMethod<X>(X x) => x;
+}
+''');
+  }
+
   test_extensionType() async {
     await assertDiagnostics(
       r'''
@@ -201,6 +217,37 @@ typedef T = int Function(int);
 ''');
   }
 
+  test_typeParameter_class() async {
+    await assertDiagnostics(
+      r'''
+class A<Object> {}
+''',
+      [lint(8, 6)],
+    );
+  }
+
+  test_typeParameter_function() async {
+    await assertDiagnostics(
+      r'''
+void f<int>() {}
+''',
+      [lint(7, 3)],
+    );
+  }
+
+  test_typeParameter_instanceMethod_class() async {
+    await assertDiagnostics(
+      r'''
+class A<X> {
+  C instanceMethod<C>(C c) => c;
+}
+
+class C {}
+''',
+      [lint(32, 1)],
+    );
+  }
+
   test_typeParameter_wildcard() async {
     await assertDiagnostics(
       r'''