Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Commit b629752

Browse files
sigmundchsigmundch
committed
Get rid of redundant error messages.
[email protected] Review URL: https://chromereviews.googleplex.com/146637013
1 parent f67ba57 commit b629752

File tree

2 files changed

+371
-65
lines changed

2 files changed

+371
-65
lines changed

‎lib/src/checker/checker.dart

Lines changed: 118 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,14 @@ class _OverrideChecker {
4141

4242
// Check overrides from applying mixins
4343
for (int i = 0; i < mixins.length; i++) {
44+
var seen = new Set<String>();
4445
var current = mixins[i];
4546
var errorLocation = node.withClause.mixinTypes[i];
46-
_checkIndividualOverridesFromType(current, parent, errorLocation);
47-
for (int j = 0; j < i; j++) {
48-
_checkIndividualOverridesFromType(current, mixins[j], errorLocation);
47+
for (int j = i - 1; j >= 0; j--) {
48+
_checkIndividualOverridesFromType(
49+
current, mixins[j], errorLocation, seen);
4950
}
51+
_checkIndividualOverridesFromType(current, parent, errorLocation, seen);
5052
}
5153
}
5254

@@ -55,13 +57,30 @@ class _OverrideChecker {
5557
///
5658
/// A extends B with E, F
5759
///
58-
/// we check A against B, B super classes, E, and F.
60+
/// we check A against B, B super classes, E, and F.
61+
///
62+
/// Internally we avoid reporting errors twice and we visit classes bottom up
63+
/// to ensure we report the most immediate invalid override first. For
64+
/// example, in the following code we'll report that `Test` has an invalid
65+
/// override with respect to `Parent` (as opposed to an invalid override with
66+
/// respect to `Grandparent`):
67+
///
68+
/// class Grandparent {
69+
/// m(A a) {}
70+
/// }
71+
/// class Parent extends Grandparent {
72+
/// m(A a) {}
73+
/// }
74+
/// class Test extends Parent {
75+
/// m(B a) {} // invalid override
76+
/// }
5977
void _checkSuperOverrides(ClassDeclaration node) {
78+
var seen = new Set<String>();
6079
var current = node.element.type;
6180
do {
62-
_checkIndividualOverridesFromClass(node, current.superclass);
63-
current.mixins
64-
.forEach((m) => _checkIndividualOverridesFromClass(node, m));
81+
current.mixins.reversed
82+
.forEach((m) => _checkIndividualOverridesFromClass(node, m, seen));
83+
_checkIndividualOverridesFromClass(node, current.superclass, seen);
6584
current = current.superclass;
6685
} while (!current.isObject);
6786
}
@@ -82,6 +101,7 @@ class _OverrideChecker {
82101
/// F against H and I
83102
/// A against H and I
84103
void _checkAllInterfaceOverrides(ClassDeclaration node) {
104+
var seen = new Set<String>();
85105
// Helper function to collect all reachable interfaces.
86106
find(InterfaceType interfaceType, Set result) {
87107
if (interfaceType == null || interfaceType.isObject) return;
@@ -97,9 +117,8 @@ class _OverrideChecker {
97117
var localInterfaces = new Set();
98118
var type = node.element.type;
99119
type.interfaces.forEach((i) => find(i, localInterfaces));
100-
for (var interfaceType in localInterfaces) {
101-
_checkInterfaceOverrides(node, interfaceType, includeParents: true);
102-
}
120+
_checkInterfacesOverrides(node, localInterfaces, seen,
121+
includeParents: true);
103122

104123
// Check also how we override locally the interfaces from parent classes if
105124
// the parent class is abstract. Otherwise, these will be checked as
@@ -113,33 +132,55 @@ class _OverrideChecker {
113132
parent.interfaces.forEach((i) => find(i, superInterfaces));
114133
parent = parent.superclass;
115134
}
116-
for (var interfaceType in superInterfaces) {
117-
_checkInterfaceOverrides(node, interfaceType, includeParents: false);
118-
}
135+
_checkInterfacesOverrides(node, superInterfaces, seen,
136+
includeParents: false);
119137
}
120138

121-
/// Checks that [node] and its super classes (including mixins) correctly
122-
/// override [interfaceType].
123-
void _checkInterfaceOverrides(
124-
ClassDeclaration node, InterfaceType interfaceType,
125-
{bool includeParents}) {
126-
_checkIndividualOverridesFromClass(node, interfaceType);
127-
128-
var type = node.element.type;
129-
if (includeParents) {
130-
var parent = type.superclass;
131-
var parentErrorLocation = node.extendsClause;
132-
while (parent != null) {
139+
/// Checks that [cls] and its super classes (including mixins) correctly
140+
/// overrides each interface in [interfaces]. If [includeParents] is false,
141+
/// then mixins are still checked, but the base type and it's transitive
142+
/// supertypes are not.
143+
///
144+
/// [cls] can be either a [ClassDeclaration] or a [InterfaceType]. For
145+
/// [ClassDeclaration]s errors are reported on the member that contains the
146+
/// invalid override, for [InterfaceType]s we use [errorLocation] instead.
147+
void _checkInterfacesOverrides(
148+
cls, Iterable<InterfaceType> interfaces, Set<String> seen,
149+
{bool includeParents: true, AstNode errorLocation}) {
150+
var node = cls is ClassDeclaration ? cls : null;
151+
var type = cls is InterfaceType ? cls : node.element.type;
152+
153+
// Check direct overrides on [type]
154+
for (var interfaceType in interfaces) {
155+
if (node != null) {
156+
_checkIndividualOverridesFromClass(node, interfaceType, seen);
157+
} else {
133158
_checkIndividualOverridesFromType(
134-
parent, interfaceType, parentErrorLocation);
135-
parent = parent.superclass;
159+
type, interfaceType, errorLocation, seen);
136160
}
137161
}
138162

163+
// Check overrides from its mixins
139164
for (int i = 0; i < type.mixins.length; i++) {
140-
var current = type.mixins[i];
141-
var errorLocation = node.withClause.mixinTypes[i];
142-
_checkIndividualOverridesFromType(current, interfaceType, errorLocation);
165+
var loc =
166+
errorLocation != null ? errorLocation : node.withClause.mixinTypes[i];
167+
for (var interfaceType in interfaces) {
168+
// We copy [seen] so we can report separately if more than one mixin or
169+
// the base class have an invalid override.
170+
_checkIndividualOverridesFromType(
171+
type.mixins[i], interfaceType, loc, new Set.from(seen));
172+
}
173+
}
174+
175+
// Check overrides from its superclasses
176+
if (includeParents) {
177+
var parent = type.superclass;
178+
if (parent.isObject) return;
179+
var loc = errorLocation != null ? errorLocation : node.extendsClause;
180+
// No need to copy [seen] here because we made copies above when reporting
181+
// errors on mixins.
182+
_checkInterfacesOverrides(parent, interfaces, seen,
183+
includeParents: true, errorLocation: loc);
143184
}
144185
}
145186

@@ -148,11 +189,19 @@ class _OverrideChecker {
148189
///
149190
/// The [errorLocation] node indicates where errors are reported, see
150191
/// [_checkSingleOverride] for more details.
151-
_checkIndividualOverridesFromType(
152-
InterfaceType subType, InterfaceType baseType, AstNode errorLocation) {
192+
///
193+
/// The set [seen] is used to avoid reporting overrides more than once. It
194+
/// is used when invoking this function multiple times when checking several
195+
/// types in a class hierarchy. Errors are reported only the first time an
196+
/// invalid override involving a specific member is encountered.
197+
_checkIndividualOverridesFromType(InterfaceType subType,
198+
InterfaceType baseType, AstNode errorLocation, Set<String> seen) {
153199
void checkHelper(ExecutableElement e) {
154200
if (e.isStatic) return;
155-
_checkSingleOverride(e, baseType, null, errorLocation);
201+
if (seen.contains(e.name)) return;
202+
if (_checkSingleOverride(e, baseType, null, errorLocation)) {
203+
seen.add(e.name);
204+
}
156205
}
157206
subType.methods.forEach(checkHelper);
158207
subType.accessors.forEach(checkHelper);
@@ -164,23 +213,31 @@ class _OverrideChecker {
164213
/// The [errorLocation] node indicates where errors are reported, see
165214
/// [_checkSingleOverride] for more details.
166215
_checkIndividualOverridesFromClass(
167-
ClassDeclaration node, InterfaceType baseType) {
216+
ClassDeclaration node, InterfaceType baseType, Set<String> seen) {
168217
for (var member in node.members) {
169218
if (member is ConstructorDeclaration) continue;
170219
if (member is FieldDeclaration) {
171220
if (member.isStatic) continue;
172221
for (var variable in member.fields.variables) {
173-
_checkSingleOverride(
174-
variable.element.getter, baseType, variable, member);
175-
if (!variable.isFinal) {
176-
_checkSingleOverride(
177-
variable.element.setter, baseType, variable, member);
222+
var name = variable.element.name;
223+
if (seen.contains(name)) continue;
224+
var getter = variable.element.getter;
225+
var setter = variable.element.setter;
226+
bool found = _checkSingleOverride(getter, baseType, variable, member);
227+
if (!variable.isFinal &&
228+
_checkSingleOverride(setter, baseType, variable, member)) {
229+
found = true;
178230
}
231+
if (found) seen.add(name);
179232
}
180233
} else {
181234
assert(member is MethodDeclaration);
182235
if (member.isStatic) continue;
183-
_checkSingleOverride(member.element, baseType, member, member);
236+
var method = member.element;
237+
if (seen.contains(method.name)) continue;
238+
if (_checkSingleOverride(method, baseType, member, member)) {
239+
seen.add(method.name);
240+
}
184241
}
185242
}
186243
}
@@ -208,7 +265,8 @@ class _OverrideChecker {
208265
}
209266

210267
/// Checks that [element] correctly overrides its corresponding member in
211-
/// [type].
268+
/// [type]. Returns `true` if an override was found, that is, if [element] has
269+
/// a corresponding member in [type] that it overrides.
212270
///
213271
/// The [errorLocation] is a node where the error is reported. For example, a
214272
/// bad override of a method in a class with respect to its superclass is
@@ -232,39 +290,36 @@ class _OverrideChecker {
232290
/// When checking for overrides from a type and it's super types, [node] is
233291
/// the AST node that defines [element]. This is used to determine whether the
234292
/// type of the element could be inferred from the types in the super classes.
235-
void _checkSingleOverride(ExecutableElement element, InterfaceType type,
293+
bool _checkSingleOverride(ExecutableElement element, InterfaceType type,
236294
AstNode node, AstNode errorLocation) {
237295
assert(!element.isStatic);
238296

239297
FunctionType subType = _rules.elementType(element);
240298
// TODO(vsm): Test for generic
241299
FunctionType baseType = _getMemberType(type, element);
242300

243-
if (baseType != null) {
244-
//var result = _checkOverride(subType, baseType);
245-
//if (result != null) return result;
246-
if (!_rules.isAssignable(subType, baseType)) {
247-
// See whether non-assignable cases fit one of our common patterns:
248-
//
249-
// Common pattern 1: Inferable return type (on getters and methods)
250-
// class A {
251-
// int get foo => ...;
252-
// String toString() { ... }
253-
// }
254-
// class B extends A {
255-
// get foo => e; // no type specified.
256-
// toString() { ... } // no return type specified.
257-
// }
258-
if (_isInferableOverride(element, node, subType, baseType)) {
259-
_recordMessage(new InferableOverride(errorLocation, element, type,
260-
subType.returnType, baseType.returnType));
261-
return;
262-
}
301+
if (baseType == null) return false;
302+
if (!_rules.isAssignable(subType, baseType)) {
303+
// See whether non-assignable cases fit one of our common patterns:
304+
//
305+
// Common pattern 1: Inferable return type (on getters and methods)
306+
// class A {
307+
// int get foo => ...;
308+
// String toString() { ... }
309+
// }
310+
// class B extends A {
311+
// get foo => e; // no type specified.
312+
// toString() { ... } // no return type specified.
313+
// }
314+
if (_isInferableOverride(element, node, subType, baseType)) {
315+
_recordMessage(new InferableOverride(errorLocation, element, type,
316+
subType.returnType, baseType.returnType));
317+
} else {
263318
_recordMessage(new InvalidMethodOverride(
264319
errorLocation, element, type, subType, baseType));
265-
return;
266320
}
267321
}
322+
return true;
268323
}
269324

270325
bool _isInferableOverride(ExecutableElement element, AstNode node,

0 commit comments

Comments
 (0)