linter: prefer_ctors: dont report for subtypes

Fixes https://github.com/dart-lang/linter/issues/3821

Change-Id: I1cd5de00a42fc0d7a109a6bffb22f4af6be1854e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364967
Auto-Submit: Samuel Rawlins <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>

Author: srawlins

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

@@ -9,7 +9,8 @@ import 'package:analyzer/dart/element/type.dart';
 import '../analyzer.dart';
 
 const _desc =
-    r'Prefer defining constructors instead of static methods to create instances.';
+    r'Prefer defining constructors instead of static methods to create '
+    'instances.';
 
 const _details = r'''
 **PREFER** defining constructors instead of static methods to create instances.
@@ -41,11 +42,6 @@ class Point {
 ```
 ''';
 
-// TODO(pq): temporary; remove after renamed class is in the SDK
-// ignore: non_constant_identifier_names
-LintRule PreferConstructorsInsteadOfStaticMethods() =>
-    PreferConstructorsOverStaticMethods();
-
 bool _hasNewInvocation(DartType returnType, FunctionBody body) =>
     _BodyVisitor(returnType).containsInstanceCreation(body);
 
@@ -68,7 +64,7 @@ class PreferConstructorsOverStaticMethods extends LintRule {
   @override
   void registerNodeProcessors(
       NodeLintRegistry registry, LinterContext context) {
-    var visitor = _Visitor(this, context);
+    var visitor = _Visitor(this);
     registry.addMethodDeclaration(this, visitor);
   }
 }
@@ -86,6 +82,9 @@ class _BodyVisitor extends RecursiveAstVisitor {
 
   @override
   visitInstanceCreationExpression(InstanceCreationExpression node) {
+    // TODO(srawlins): This assignment overrides existing `found` values.
+    // For example, given `() { C(); D(); }`, if `C` was the return type being
+    // sought, then the `found` value is overridden when we visit `D()`.
     found = node.staticType == returnType;
     if (!found) {
       super.visitInstanceCreationExpression(node);
@@ -95,9 +94,8 @@ class _BodyVisitor extends RecursiveAstVisitor {
 
 class _Visitor extends SimpleAstVisitor<void> {
   final LintRule rule;
-  final LinterContext context;
 
-  _Visitor(this.rule, this.context);
+  _Visitor(this.rule);
 
   @override
   void visitMethodDeclaration(MethodDeclaration node) {
@@ -107,13 +105,10 @@ class _Visitor extends SimpleAstVisitor<void> {
     if (returnType is! InterfaceType) return;
 
     var interfaceType = node.parent.typeToCheckOrNull();
-    if (interfaceType != null) {
-      if (!context.typeSystem.isAssignableTo(returnType, interfaceType)) {
-        return;
-      }
-      if (_hasNewInvocation(returnType, node.body)) {
-        rule.reportLintForToken(node.name);
-      }
+    if (interfaceType != returnType) return;
+
+    if (_hasNewInvocation(returnType, node.body)) {
+      rule.reportLintForToken(node.name);
     }
   }
 }

pkg/linter/test/rules/prefer_constructors_over_static_methods_test.dart

@@ -62,7 +62,7 @@ class A {
   }
 
   test_staticMethod_expressionBody_extensionType() async {
-    // Since the check logic is shared one test should be sufficient to verify
+    // Since the check logic is shared, one test should be sufficient to verify
     // extension types are supported.
     await assertDiagnostics(r'''
 extension type E(int i) {
@@ -125,13 +125,19 @@ class A {
 ''');
   }
 
+  test_staticMethod_returnsSubtype() async {
+    await assertNoDiagnostics(r'''
+class A {
+  static B f() => B();
+}
+class B extends A {}
+''');
+  }
+
   test_staticMethod_returnsUnrelatedType() async {
     await assertNoDiagnostics(r'''
 class A {
-  A.named();
   static Object staticM() => Object();
-
-  /*static A? ok2() => 1==1 ? null : A.internal(); // OK*/
 }
 ''');
   }