Fix mustCallSuper for mixins; fixes #36492
This re-lands most of https://dart-review.googlesource.com/c/sdk/+/98443.
This change does not include the "inherited interfaces" bit of that CL,
which some developers have objected to; we'll discuss.
Bug: https://github.com/dart-lang/sdk/issues/36492
Change-Id: Ibaa8ab87c749eae6359b739204c6f286b6818aa5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99727
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analyzer/lib/dart/element/element.dart b/pkg/analyzer/lib/dart/element/element.dart
index 20b425d..4a83f50 100644
--- a/pkg/analyzer/lib/dart/element/element.dart
+++ b/pkg/analyzer/lib/dart/element/element.dart
@@ -521,9 +521,12 @@
/// Return `true` if this element has an annotation of the form `@JS(..)`.
bool get hasJS;
- /// Return `true` if this element has an annotation of the form '@literal'.
+ /// Return `true` if this element has an annotation of the form `@literal`.
bool get hasLiteral;
+ /// Return `true` if this element has an annotation of the form `@mustCallSuper`.
+ bool get hasMustCallSuper;
+
/// Return `true` if this element has an annotation of the form
/// `@optionalTypeArgs`.
bool get hasOptionalTypeArgs;
@@ -534,10 +537,10 @@
/// Return `true` if this element has an annotation of the form `@protected`.
bool get hasProtected;
- /// Return `true` if this element has an annotation of the form '@required'.
+ /// Return `true` if this element has an annotation of the form `@required`.
bool get hasRequired;
- /// Return `true` if this element has an annotation of the form '@sealed'.
+ /// Return `true` if this element has an annotation of the form `@sealed`.
bool get hasSealed;
/// Return `true` if this element has an annotation of the form
diff --git a/pkg/analyzer/lib/src/dart/element/element.dart b/pkg/analyzer/lib/src/dart/element/element.dart
index 588d1a6..1c3c7e2 100644
--- a/pkg/analyzer/lib/src/dart/element/element.dart
+++ b/pkg/analyzer/lib/src/dart/element/element.dart
@@ -3376,6 +3376,18 @@
}
@override
+ bool get hasMustCallSuper {
+ var metadata = this.metadata;
+ for (var i = 0; i < metadata.length; i++) {
+ var annotation = metadata[i];
+ if (annotation.isMustCallSuper) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @override
bool get hasOptionalTypeArgs {
var metadata = this.metadata;
for (var i = 0; i < metadata.length; i++) {
@@ -7516,6 +7528,9 @@
bool get hasLiteral => false;
@override
+ bool get hasMustCallSuper => false;
+
+ @override
bool get hasOptionalTypeArgs => false;
@override
diff --git a/pkg/analyzer/lib/src/dart/element/handle.dart b/pkg/analyzer/lib/src/dart/element/handle.dart
index 7a4a1e7..b92e450 100644
--- a/pkg/analyzer/lib/src/dart/element/handle.dart
+++ b/pkg/analyzer/lib/src/dart/element/handle.dart
@@ -395,6 +395,9 @@
bool get hasLiteral => actualElement.hasLiteral;
@override
+ bool get hasMustCallSuper => actualElement.hasMustCallSuper;
+
+ @override
bool get hasOptionalTypeArgs => actualElement.hasOptionalTypeArgs;
@override
diff --git a/pkg/analyzer/lib/src/dart/element/member.dart b/pkg/analyzer/lib/src/dart/element/member.dart
index 12fd38f..ea1a3aa 100644
--- a/pkg/analyzer/lib/src/dart/element/member.dart
+++ b/pkg/analyzer/lib/src/dart/element/member.dart
@@ -428,6 +428,9 @@
bool get hasLiteral => _baseElement.hasLiteral;
@override
+ bool get hasMustCallSuper => _baseElement.hasMustCallSuper;
+
+ @override
bool get hasOptionalTypeArgs => _baseElement.hasOptionalTypeArgs;
@override
diff --git a/pkg/analyzer/lib/src/dart/element/wrapped.dart b/pkg/analyzer/lib/src/dart/element/wrapped.dart
index 5aebd1d..5a9be7c 100644
--- a/pkg/analyzer/lib/src/dart/element/wrapped.dart
+++ b/pkg/analyzer/lib/src/dart/element/wrapped.dart
@@ -71,6 +71,9 @@
bool get hasLoadLibraryFunction => wrappedUnit.hasLoadLibraryFunction;
@override
+ bool get hasMustCallSuper => wrappedUnit.hasMustCallSuper;
+
+ @override
bool get hasOptionalTypeArgs => wrappedUnit.hasOptionalTypeArgs;
@override
@@ -264,6 +267,9 @@
bool get hasLiteral => wrappedImport.hasLiteral;
@override
+ bool get hasMustCallSuper => wrappedImport.hasMustCallSuper;
+
+ @override
bool get hasOptionalTypeArgs => wrappedImport.hasOptionalTypeArgs;
@override
@@ -472,6 +478,9 @@
bool get hasLoadLibraryFunction => wrappedLib.hasLoadLibraryFunction;
@override
+ bool get hasMustCallSuper => wrappedLib.hasMustCallSuper;
+
+ @override
bool get hasOptionalTypeArgs => wrappedLib.hasOptionalTypeArgs;
@override
diff --git a/pkg/analyzer/lib/src/generated/error_verifier.dart b/pkg/analyzer/lib/src/generated/error_verifier.dart
index 7f2f741..e0566d3 100644
--- a/pkg/analyzer/lib/src/generated/error_verifier.dart
+++ b/pkg/analyzer/lib/src/generated/error_verifier.dart
@@ -4358,7 +4358,7 @@
return;
}
MethodElement element = _findOverriddenMemberThatMustCallSuper(node);
- if (element != null) {
+ if (element != null && _hasConcreteSuperMethod(node)) {
_InvocationCollector collector = new _InvocationCollector();
node.accept(collector);
if (!collector.superCalls.contains(element.name)) {
@@ -5901,20 +5901,40 @@
return result;
}
+ // Find a method which is overridden by [node] and which is annotated with
+ // `@mustCallSuper`.
+ //
+ // As per the definition of `mustCallSuper` [1], every method which overrides
+ // a method annotated with `@mustCallSuper` is implicitly annotated with
+ // `@mustCallSuper`.
+ //
+ // [1] https://pub.dartlang.org/documentation/meta/latest/meta/mustCallSuper-constant.html
MethodElement _findOverriddenMemberThatMustCallSuper(MethodDeclaration node) {
- ExecutableElement overriddenMember =
- _getOverriddenMember(node.declaredElement);
- List<ExecutableElement> seen = <ExecutableElement>[];
- while (
- overriddenMember is MethodElement && !seen.contains(overriddenMember)) {
- for (ElementAnnotation annotation in overriddenMember.metadata) {
- if (annotation.isMustCallSuper) {
- return overriddenMember;
- }
+ Element member = node.declaredElement;
+ ClassElement classElement = member.enclosingElement;
+ String name = member.name;
+
+ // Walk up the type hierarchy from [classElement], ignoring direct interfaces.
+ Queue<ClassElement> superclasses =
+ Queue.of(classElement.mixins.map((i) => i.element))
+ ..addAll(classElement.superclassConstraints.map((i) => i.element))
+ ..add(classElement.supertype?.element);
+ Set<ClassElement> visitedClasses = new Set<ClassElement>();
+ while (superclasses.isNotEmpty) {
+ ClassElement ancestor = superclasses.removeFirst();
+ if (ancestor == null || !visitedClasses.add(ancestor)) {
+ continue;
}
- seen.add(overriddenMember);
- // Keep looking up the chain.
- overriddenMember = _getOverriddenMember(overriddenMember);
+ ExecutableElement member = ancestor.getMethod(name) ??
+ ancestor.getGetter(name) ??
+ ancestor.getSetter(name);
+ if (member is MethodElement && member.hasMustCallSuper) {
+ return member;
+ }
+ superclasses
+ ..addAll(ancestor.mixins.map((i) => i.element))
+ ..addAll(ancestor.superclassConstraints.map((i) => i.element))
+ ..add(ancestor.supertype?.element);
}
return null;
}
@@ -6021,23 +6041,6 @@
return buffer.toString();
}
- ExecutableElement _getOverriddenMember(Element member) {
- ClassElement classElement = member.enclosingElement;
- String name = member.name;
- ClassElement superclass = classElement.supertype?.element;
- Set<ClassElement> visitedClasses = new Set<ClassElement>();
- while (superclass != null && visitedClasses.add(superclass)) {
- ExecutableElement member = superclass.getMethod(name) ??
- superclass.getGetter(name) ??
- superclass.getSetter(name);
- if (member != null) {
- return member;
- }
- superclass = superclass.supertype?.element;
- }
- return null;
- }
-
/**
* Return the type of the first and only parameter of the given [setter].
*/
@@ -6051,6 +6054,19 @@
return setterParameters[0].type;
}
+ // Returns whether [node] overrides a concrete method.
+ bool _hasConcreteSuperMethod(MethodDeclaration node) {
+ ClassElement classElement = node.declaredElement.enclosingElement;
+ String name = node.declaredElement.name;
+
+ Queue<ClassElement> superclasses =
+ Queue.of(classElement.mixins.map((i) => i.element))
+ ..addAll(classElement.superclassConstraints.map((i) => i.element))
+ ..add(classElement.supertype?.element);
+ return superclasses.any(
+ (parent) => parent.lookUpConcreteMethod(name, parent.library) != null);
+ }
+
/**
* Return `true` if the given [constructor] redirects to itself, directly or
* indirectly.
diff --git a/pkg/analyzer/test/src/diagnostics/must_call_super_test.dart b/pkg/analyzer/test/src/diagnostics/must_call_super_test.dart
index 588df81..7230ed0 100644
--- a/pkg/analyzer/test/src/diagnostics/must_call_super_test.dart
+++ b/pkg/analyzer/test/src/diagnostics/must_call_super_test.dart
@@ -52,6 +52,20 @@
''', [HintCode.MUST_CALL_SUPER]);
}
+ test_fromExtendingClass_abstractImplementation() async {
+ await assertNoErrorsInCode(r'''
+import 'package:meta/meta.dart';
+abstract class A {
+ @mustCallSuper
+ void a();
+}
+class B extends A {
+ @override
+ void a() {}
+}
+''');
+ }
+
test_fromInterface() async {
await assertNoErrorsInCode(r'''
import 'package:meta/meta.dart';
@@ -66,6 +80,20 @@
''');
}
+ test_fromMixin() async {
+ await assertErrorCodesInCode(r'''
+import 'package:meta/meta.dart';
+class Mixin {
+ @mustCallSuper
+ void a() {}
+}
+class C with Mixin {
+ @override
+ void a() {}
+}
+''', [HintCode.MUST_CALL_SUPER]);
+ }
+
test_indirectlyInherited() async {
await assertErrorCodesInCode(r'''
import 'package:meta/meta.dart';
@@ -86,6 +114,35 @@
''', [HintCode.MUST_CALL_SUPER]);
}
+ test_indirectlyInheritedFromMixin() async {
+ await assertErrorCodesInCode(r'''
+import 'package:meta/meta.dart';
+class Mixin {
+ @mustCallSuper
+ void b() {}
+}
+class C extends Object with Mixin {}
+class D extends C {
+ @override
+ void b() {}
+}
+''', [HintCode.MUST_CALL_SUPER]);
+ }
+
+ test_indirectlyInheritedFromMixinConstraint() async {
+ await assertErrorCodesInCode(r'''
+import 'package:meta/meta.dart';
+class A {
+ @mustCallSuper
+ void a() {}
+}
+mixin C on A {
+ @override
+ void a() {}
+}
+''', [HintCode.MUST_CALL_SUPER]);
+ }
+
test_overriddenWithFuture() async {
// https://github.com/flutter/flutter/issues/11646
await assertNoErrorsInCode(r'''
