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

Commit a07cad7

Browse files
sigmundchsigmundch
committed
Remove InvalidFieldOverride and measure "Inferable overrides"
[email protected] Review URL: https://chromereviews.googleplex.com/136867013
1 parent f0587c0 commit a07cad7

File tree

6 files changed

+235
-53
lines changed

6 files changed

+235
-53
lines changed

‎lib/src/checker/checker.dart

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ class ProgramChecker extends RecursiveAstVisitor {
7070
node.visitChildren(this);
7171
}
7272

73-
// Check that member declarations soundly override any overridden declarations.
73+
// Check that member declarations soundly override any parent declarations.
7474
InvalidOverride findInvalidOverride(AstNode node, ExecutableElement element,
75-
InterfaceType type, [bool allowFieldOverride = null]) {
75+
InterfaceType type) {
7676
// FIXME: This can be done a lot more efficiently.
7777
assert(!element.isStatic);
7878

@@ -88,7 +88,6 @@ class ProgramChecker extends RecursiveAstVisitor {
8888
if (isGetter) {
8989
assert(!isSetter);
9090
// Look for getter or field.
91-
// FIXME: Verify that this handles fields.
9291
baseMethod = type.getGetter(memberName);
9392
} else if (isSetter) {
9493
baseMethod = type.getSetter(memberName);
@@ -110,39 +109,48 @@ class ProgramChecker extends RecursiveAstVisitor {
110109
// TODO(vsm): Test for generic
111110
FunctionType baseType = _rules.elementType(baseMethod);
112111
if (!_rules.isAssignable(subType, baseType)) {
113-
return new InvalidMethodOverride(
114-
node, element, type, subType, baseType);
115-
}
116-
117-
// Test that we're not overriding a field.
118-
if (allowFieldOverride == false) {
119-
for (FieldElement field in type.element.fields) {
120-
if (field.name == memberName) {
121-
// TODO(vsm): Is this the right test?
122-
bool syn = field.isSynthetic;
123-
if (!syn) {
124-
return new InvalidFieldOverride(node, element, type);
125-
}
112+
// See whether non-assignable cases fit one of our common patterns:
113+
//
114+
// Common pattern 1: Inferable return type (on getters and methods)
115+
// class A {
116+
// int get foo => ...;
117+
// String toString() { ... }
118+
// }
119+
// class B extends A {
120+
// get foo => e; // no type specified.
121+
// toString() { ... } // no return type specified.
122+
// }
123+
if (isGetter && element.isSynthetic) {
124+
if ((node as FieldDeclaration).fields.type == null) {
125+
return new InferableOverride(node, element, type,
126+
subType.returnType, baseType.returnType);
126127
}
128+
} else if (node is MethodDeclaration &&
129+
(node as MethodDeclaration).returnType == null &&
130+
_rules.isFunctionSubTypeOf(subType, baseType, ignoreReturn: true)) {
131+
// node is a MethodDeclaration whenever getters and setters are
132+
// declared explicitly. Setters declared from a field will have the
133+
// correct return type, so we don't need to check that separately.
134+
return new InferableOverride(node, element, type,
135+
subType.returnType, baseType.returnType);
127136
}
137+
return new InvalidMethodOverride(
138+
node, element, type, subType, baseType);
128139
}
129140
}
130141

131142
if (type.isObject) return null;
132143

133-
allowFieldOverride =
134-
allowFieldOverride == null ? false : allowFieldOverride;
135-
InvalidOverride base =
136-
findInvalidOverride(node, element, type.superclass, allowFieldOverride);
144+
InvalidOverride base = findInvalidOverride(node, element, type.superclass);
137145
if (base != null) return base;
138146

139147
for (final parent in type.interfaces) {
140-
base = findInvalidOverride(node, element, parent, true);
148+
base = findInvalidOverride(node, element, parent);
141149
if (base != null) return base;
142150
}
143151

144152
for (final parent in type.mixins) {
145-
base = findInvalidOverride(node, element, parent, true);
153+
base = findInvalidOverride(node, element, parent);
146154
if (base != null) return base;
147155
}
148156

‎lib/src/checker/rules.dart

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,18 @@ class RestrictedRules extends TypeRules {
9999
return true;
100100
}
101101

102-
bool isFunctionSubTypeOf(FunctionType f1, FunctionType f2) {
102+
/// Check that f1 is a subtype of f2. [ignoreReturn] is used in the DDC
103+
/// checker to determine whether f1 would be a subtype of f2 if the return
104+
/// type of f1 is set to match f2's return type.
105+
bool isFunctionSubTypeOf(FunctionType f1, FunctionType f2,
106+
{bool ignoreReturn: false}) {
103107
final r1s = f1.normalParameterTypes;
104108
final o1s = f1.optionalParameterTypes;
105109
final n1s = f1.namedParameterTypes;
106110
final r2s = f2.normalParameterTypes;
107111
final o2s = f2.optionalParameterTypes;
108112
final n2s = f2.namedParameterTypes;
109-
final ret1 = f1.returnType;
113+
final ret1 = ignoreReturn ? f2.returnType : f1.returnType;
110114
final ret2 = f2.returnType;
111115

112116
// A -> B <: C -> D if C <: A and

‎lib/src/info.dart

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -274,8 +274,12 @@ class InvalidRuntimeCheckError extends StaticError {
274274
abstract class InvalidOverride extends StaticError {
275275
final ExecutableElement element;
276276
final InterfaceType base;
277+
final DartType subType;
278+
final DartType baseType;
277279

278-
InvalidOverride(AstNode node, this.element, this.base) : super(node);
280+
InvalidOverride(
281+
AstNode node, this.element, this.base, this.subType, this.baseType)
282+
: super(node);
279283

280284
ClassDeclaration get parent =>
281285
element.enclosingElement.node as ClassDeclaration;
@@ -284,30 +288,29 @@ abstract class InvalidOverride extends StaticError {
284288
// Invalid override due to incompatible type. I.e., the overridden signature
285289
// is not compatible with the original.
286290
class InvalidMethodOverride extends InvalidOverride {
287-
final FunctionType methodType;
288-
final FunctionType baseType;
289-
290291
InvalidMethodOverride(AstNode node, ExecutableElement element,
291-
InterfaceType base, this.methodType, this.baseType)
292-
: super(node, element, base);
292+
InterfaceType base, FunctionType subType, FunctionType baseType)
293+
: super(node, element, base, subType, baseType);
293294

294295
String get message {
295296
return 'Invalid override for ${element.name} in ${parent.name} '
296-
'over $base: $methodType does not subtype $baseType';
297+
'over $base: $subType does not subtype $baseType';
297298
}
298299
}
299300

300-
// TODO(vsm): Do we still need this?
301-
// Under certain rules, we disallow overriding a field with a
302-
// field/getter/setter.
303-
class InvalidFieldOverride extends InvalidOverride {
304-
InvalidFieldOverride(
305-
AstNode node, ExecutableElement element, InterfaceType base)
306-
: super(node, element, base);
301+
// TODO(sigmund): delete, if we fix this, this should be part of the type
302+
// inference, not something we detect in the checker.
303+
// TODO(sigmund): split and track field, getter, setter, method separately
304+
class InferableOverride extends InvalidOverride {
305+
InferableOverride(AstNode node, ExecutableElement element,
306+
InterfaceType base, DartType subType, DartType baseType)
307+
: super(node, element, base, subType, baseType);
308+
309+
Level get level => Level.WARNING;
307310

308311
String get message {
309-
return 'Invalid field override for ${element.name} in '
310-
'${parent.name} over $base';
312+
return 'Invalid but inferrable override for ${element.name} in '
313+
'${parent.name} over $base: $subType does not subtype $baseType';
311314
}
312315
}
313316

‎lib/src/report.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ class _Table {
357357
List _currentRow;
358358

359359
/// Add a column with the given [name].
360-
void declareColumn(String name, {bool abbreviate}) {
360+
void declareColumn(String name, {bool abbreviate: false}) {
361361
assert (!_sealed);
362362
var headerName = name;
363363
if (abbreviate) {

‎lib/src/testing.dart

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -172,17 +172,20 @@ class _ErrorMarkerVisitor extends UnifyingAstVisitor {
172172
var comment = token.precedingComments;
173173
// Use error marker found in an immediately preceding comment,
174174
// and attach it to the outermost expression that starts at that token.
175-
if (comment != null &&
176-
comment.end == token.offset &&
177-
_realParent(node).beginToken != token) {
178-
var commentText = '$comment';
179-
expect(commentText.startsWith('/*'), isTrue);
180-
expect(commentText.startsWith('/**'), isFalse);
181-
expect(commentText.endsWith('*/'), isTrue);
182-
expect(commentText.endsWith('**/'), isFalse);
183-
var errors = commentText.substring(2, commentText.length - 2).split(',');
184-
var expectations = errors.map(_ErrorExpectation.parse);
185-
expectedErrors[node] = expectations.where((x) => x != null).toList();
175+
if (comment != null) {
176+
while (comment.next != null) { comment = comment.next; }
177+
if (comment.end == token.offset &&
178+
_realParent(node).beginToken != token) {
179+
var commentText = '$comment';
180+
var start = commentText.lastIndexOf('/*');
181+
var end = commentText.lastIndexOf('*/');
182+
if (start != -1 && end != -1) {
183+
expect(start, lessThan(end));
184+
var errors = commentText.substring(start + 2, end).split(',');
185+
var expectations = errors.map(_ErrorExpectation.parse);
186+
expectedErrors[node] = expectations.where((x) => x != null).toList();
187+
}
188+
}
186189
}
187190
return super.visitNode(node);
188191
}
@@ -230,6 +233,8 @@ class _ErrorExpectation {
230233
// eventually we could do more automated reporting here.
231234
return _parse(tokens[0]);
232235
}
236+
237+
String toString() => '$level $type';
233238
}
234239

235240
/// Uri resolver that can load test files from memory.
@@ -278,7 +283,11 @@ class _TestSource implements Source {
278283

279284
Uri resolveRelativeUri(Uri relativeUri) => uri.resolveUri(relativeUri);
280285

281-
SourceSpan spanFor(AstNode node) => _file.span(node.offset, node.end);
286+
SourceSpan spanFor(AstNode node) {
287+
final begin = node is AnnotatedNode ?
288+
node.firstTokenAfterCommentAndMetadata.offset : node.offset;
289+
return _file.span(begin, node.end);
290+
}
282291

283292
String toString() => '[$runtimeType: $uri]';
284293
}

‎test/checker/checker_test.dart

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,4 +1046,162 @@ main() {
10461046
'''
10471047
});
10481048
});
1049+
1050+
test('field/field override', () {
1051+
testChecker({
1052+
'/main.dart': '''
1053+
class A {}
1054+
class B extends A {}
1055+
class C extends B {}
1056+
1057+
class Base {
1058+
B f1;
1059+
B f2;
1060+
B f3;
1061+
B f4;
1062+
}
1063+
1064+
class Child extends Base {
1065+
/*severe:InvalidMethodOverride*/A f1; // invalid for getter
1066+
/*severe:InvalidMethodOverride*/C f2; // invalid for setter
1067+
/*warning:InferableOverride*/var f3;
1068+
/*severe:InvalidMethodOverride*/dynamic f4;
1069+
}
1070+
'''
1071+
});
1072+
});
1073+
1074+
test('getter/getter override', () {
1075+
testChecker({
1076+
'/main.dart': '''
1077+
class A {}
1078+
class B extends A {}
1079+
class C extends B {}
1080+
1081+
abstract class Base {
1082+
B get f1;
1083+
B get f2;
1084+
B get f3;
1085+
B get f4;
1086+
}
1087+
1088+
class Child extends Base {
1089+
/*severe:InvalidMethodOverride*/A get f1 => null;
1090+
C get f2 => null;
1091+
/*warning:InferableOverride*/get f3 => null;
1092+
/*severe:InvalidMethodOverride*/dynamic get f4 => null;
1093+
}
1094+
'''
1095+
});
1096+
});
1097+
1098+
test('field/getter override', () {
1099+
testChecker({
1100+
'/main.dart': '''
1101+
class A {}
1102+
class B extends A {}
1103+
class C extends B {}
1104+
1105+
abstract class Base {
1106+
B f1;
1107+
B f2;
1108+
B f3;
1109+
B f4;
1110+
}
1111+
1112+
class Child extends Base {
1113+
/*severe:InvalidMethodOverride*/A get f1 => null;
1114+
C get f2 => null;
1115+
/*warning:InferableOverride*/get f3 => null;
1116+
/*severe:InvalidMethodOverride*/dynamic get f4 => null;
1117+
}
1118+
'''
1119+
});
1120+
});
1121+
1122+
test('setter/setter override', () {
1123+
testChecker({
1124+
'/main.dart': '''
1125+
class A {}
1126+
class B extends A {}
1127+
class C extends B {}
1128+
1129+
abstract class Base {
1130+
void set f1(B value);
1131+
void set f2(B value);
1132+
void set f3(B value);
1133+
void set f4(B value);
1134+
void set f5(B value);
1135+
}
1136+
1137+
class Child extends Base {
1138+
void set f1(A value) {}
1139+
/*severe:InvalidMethodOverride*/void set f2(C value) {}
1140+
void set f3(value) {}
1141+
void set f4(dynamic value) {}
1142+
/*pass should be warning:InferableOverride*/set f5(B value) {}
1143+
}
1144+
'''
1145+
});
1146+
});
1147+
1148+
test('field/setter override', () {
1149+
testChecker({
1150+
'/main.dart': '''
1151+
class A {}
1152+
class B extends A {}
1153+
class C extends B {}
1154+
1155+
class Base {
1156+
B f1;
1157+
B f2;
1158+
B f3;
1159+
B f4;
1160+
B f5;
1161+
}
1162+
1163+
class Child extends Base {
1164+
B get f1 => null;
1165+
B get f2 => null;
1166+
B get f3 => null;
1167+
B get f4 => null;
1168+
B get f5 => null;
1169+
1170+
void set f1(A value) {}
1171+
/*severe:InvalidMethodOverride*/void set f2(C value) {}
1172+
void set f3(value) {}
1173+
void set f4(dynamic value) {}
1174+
/*pass should be warning:InferableOverride*/set f5(B value) {}
1175+
}
1176+
'''
1177+
});
1178+
});
1179+
1180+
test('method override', () {
1181+
testChecker({
1182+
'/main.dart': '''
1183+
class A {}
1184+
class B extends A {}
1185+
class C extends B {}
1186+
1187+
class Base {
1188+
B m1(B a);
1189+
B m2(B a);
1190+
B m3(B a);
1191+
B m4(B a);
1192+
B m5(B a);
1193+
B m6(B a);
1194+
}
1195+
1196+
class Child extends Base {
1197+
/*severe:InvalidMethodOverride*/A m1(A value) {}
1198+
/*severe:InvalidMethodOverride*/C m2(C value) {}
1199+
/*severe:InvalidMethodOverride*/A m3(C value) {}
1200+
C m4(A value) {}
1201+
/*warning:InferableOverride*/m5(value) {}
1202+
/*severe:InvalidMethodOverride*/dynamic m6(dynamic value) {}
1203+
}
1204+
'''
1205+
});
1206+
});
10491207
}

0 commit comments

Comments
 (0)