[element model] migrate overridden_fields

This one really deserved a re-write (as migrating as-is was really ugly). The implementation is much simpler and far more efficient. I'd definitely appreciate a close look though to make sure it's also *correct*. :) Bug: dart-lang/linter#5099 Change-Id: I7dcbcf9c98279c84b45921d674f2c3fe90b9d9bc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/392081 Commit-Queue: Phil Quitslund <[email protected]> Auto-Submit: Phil Quitslund <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
dart-lang · Oct 25, 2024 · 48e0dbe · 48e0dbe
1 parent e2cdc76
commit 48e0dbe
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 77 deletions.
diff --git a/pkg/linter/lib/src/rules/overridden_fields.dart b/pkg/linter/lib/src/rules/overridden_fields.dart
@@ -4,45 +4,13 @@
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
-import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/dart/element/type.dart';
-import 'package:collection/collection.dart' show IterableExtension;
+import 'package:analyzer/dart/element/element2.dart';
 
 import '../analyzer.dart';
 import '../extensions.dart';
 
 const _desc = r"Don't override fields.";
 
-Iterable<InterfaceType> _findAllSupertypesAndMixins(
-    InterfaceType? interface, List<InterfaceType> accumulator) {
-  if (interface == null ||
-      interface.isDartCoreObject ||
-      accumulator.contains(interface)) {
-    return accumulator;
-  }
-
-  accumulator.add(interface);
-  var superclass = interface.superclass;
-  var interfaces = <InterfaceType>[];
-  if (superclass != null) {
-    interfaces.add(superclass);
-  }
-  interfaces
-    ..addAll(interface.element.mixins)
-    ..addAll(_findAllSupertypesAndMixins(superclass, accumulator));
-  return interfaces.where((i) => i != interface);
-}
-
-Iterable<InterfaceType> _findAllSupertypesInMixin(MixinElement mixinElement) {
-  var supertypes = <InterfaceType>[];
-  var accumulator = <InterfaceType>[];
-  for (var type in mixinElement.superclassConstraints) {
-    supertypes.add(type);
-    supertypes.addAll(_findAllSupertypesAndMixins(type, accumulator));
-  }
-  return supertypes;
-}
-
 class OverriddenFields extends LintRule {
   OverriddenFields()
       : super(
@@ -56,66 +24,36 @@ class OverriddenFields extends LintRule {
   @override
   void registerNodeProcessors(
       NodeLintRegistry registry, LinterContext context) {
-    var visitor = _Visitor(this);
+    var visitor = _Visitor(this, context.inheritanceManager);
     registry.addFieldDeclaration(this, visitor);
   }
 }
 
 class _Visitor extends SimpleAstVisitor<void> {
   final LintRule rule;
+  final InheritanceManager3 inheritanceManager;
 
-  _Visitor(this.rule);
+  _Visitor(this.rule, this.inheritanceManager);
 
   @override
   void visitFieldDeclaration(FieldDeclaration node) {
     if (node.isAugmentation) return;
     if (node.isStatic) return;
 
     for (var variable in node.fields.variables) {
-      var declaredField = variable.declaredElement;
-      if (declaredField != null) {
-        var overriddenField = _getOverriddenMember(declaredField);
-        if (overriddenField != null && !overriddenField.isAbstract) {
-          rule.reportLintForToken(variable.name,
-              arguments: [overriddenField.enclosingElement3.displayName]);
-        }
-      }
-    }
-  }
-
-  PropertyAccessorElement? _getOverriddenMember(Element member) {
-    var memberName = member.name;
-    var library = member.library;
-    bool isOverriddenMember(PropertyAccessorElement a) {
-      if (memberName == null || a.isStatic) {
-        return false;
-      }
-      if (a.isSynthetic && a.name == memberName) {
-        // Ensure that private members are overriding a member of the same library.
-        if (Identifier.isPrivateName(memberName)) {
-          return library == a.library;
+      var parent = variable.declaredFragment?.element.enclosingElement2;
+      if (parent is InterfaceElement2) {
+        var overriddenMember = inheritanceManager.getMember4(
+            parent, Name(parent.library2.uri, variable.name.lexeme),
+            forSuper: true);
+        if (overriddenMember is GetterElement && overriddenMember.isSynthetic) {
+          var definingInterface = overriddenMember.enclosingElement2;
+          if (definingInterface != null) {
+            rule.reportLintForToken(variable.name,
+                arguments: [definingInterface.displayName]);
+          }
         }
-        return true;
       }
-      return false;
-    }
-
-    bool containsOverriddenMember(InterfaceType i) =>
-        i.accessors.any(isOverriddenMember);
-    var enclosingElement = member.enclosingElement3;
-    if (enclosingElement is! InterfaceElement) {
-      return null;
-    }
-    var classElement = enclosingElement;
-
-    Iterable<InterfaceType> interfaces;
-    if (classElement is MixinElement) {
-      interfaces = _findAllSupertypesInMixin(classElement);
-    } else {
-      interfaces =
-          _findAllSupertypesAndMixins(classElement.thisType, <InterfaceType>[]);
     }
-    var interface = interfaces.firstWhereOrNull(containsOverriddenMember);
-    return interface?.accessors.firstWhere(isOverriddenMember);
   }
 }
diff --git a/pkg/linter/test/rules/overridden_fields_test.dart b/pkg/linter/test/rules/overridden_fields_test.dart
@@ -70,6 +70,20 @@ augment class A {
     await assertNoDiagnosticsInFile(b.path);
   }
 
+  test_conflictingFieldAndMethod() async {
+    await assertDiagnostics(r'''
+class A {
+  int x() => 0;
+}
+
+class B extends A {
+  int x = 9;
+}
+''', [
+      error(CompileTimeErrorCode.CONFLICTING_FIELD_AND_METHOD, 55, 1),
+    ]);
+  }
+
   /// https://github.com/dart-lang/linter/issues/2874
   test_conflictingStaticAndInstance() async {
     await assertNoDiagnostics(r'''
@@ -115,6 +129,18 @@ class B extends A {
     ]);
   }
 
+  test_fieldOverridesGetter() async {
+    await assertNoDiagnostics(r'''
+class A {
+  int get a => 0;
+}
+class B extends A {
+  @override
+  int a = 1;
+}
+''');
+  }
+
   test_mixinInheritsFromNotObject() async {
     // See: https://github.com/dart-lang/linter/issues/2969
     // Preserves existing testing logic but has so many misuses of mixins that
@@ -189,6 +215,7 @@ class GC34 extends GC33 {
       lint(127, 5),
       lint(194, 9),
       error(CompileTimeErrorCode.MIXIN_INHERITS_FROM_NOT_OBJECT, 273, 4),
+      lint(301, 9),
       lint(343, 5),
       lint(418, 9),
       error(CompileTimeErrorCode.MIXIN_INHERITS_FROM_NOT_OBJECT, 472, 4),
@@ -203,6 +230,49 @@ class GC34 extends GC33 {
     ]);
   }
 
+  test_overridingAbstractField() async {
+    await assertNoDiagnostics(r'''
+abstract class A {
+  abstract int x;
+}
+
+class B extends A {
+  @override
+  int x = 1;
+}
+''');
+  }
+
+  test_privateFieldInSameLibrary() async {
+    await assertDiagnostics(r'''
+class A {
+  int _x = 0;
+}
+
+class B extends A {
+  int _x = 9;
+}
+''', [
+      error(WarningCode.UNUSED_FIELD, 16, 2),
+      lint(53, 2),
+      error(WarningCode.UNUSED_FIELD, 53, 2),
+    ]);
+  }
+
+  test_publicFieldInSameLibrary() async {
+    await assertDiagnostics(r'''
+class A {
+  int x = 0;
+}
+
+class B extends A {
+  int x = 9;
+}
+''', [
+      lint(52, 1),
+    ]);
+  }
+
   test_recursiveInterfaceInheritance() async {
     // Produces a recursive_interface_inheritance diagnostic.
     await assertDiagnostics(r'''