Skip to content

Commit 19ff4ca

Browse files
vsmenonvsmenon
committed
Check field overrides
Fixes dart-archive/dev_compiler#52 This hits once in the SDK - naturally Devon hit it writing his version of hello world. Fix coming in DDC. [email protected] Review URL: https://codereview.chromium.org/1430953004 .
1 parent 5a0e4a5 commit 19ff4ca

File tree

4 files changed

+184
-37
lines changed

4 files changed

+184
-37
lines changed

‎pkg/analyzer/lib/src/task/strong/checker.dart

+71-18
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,10 @@ class _OverrideChecker {
5151
var errorLocation = node.withClause.mixinTypes[i];
5252
for (int j = i - 1; j >= 0; j--) {
5353
_checkIndividualOverridesFromType(
54-
current, mixins[j], errorLocation, seen);
54+
current, mixins[j], errorLocation, seen, true);
5555
}
56-
_checkIndividualOverridesFromType(current, parent, errorLocation, seen);
56+
_checkIndividualOverridesFromType(
57+
current, parent, errorLocation, seen, true);
5758
}
5859
}
5960

@@ -85,9 +86,9 @@ class _OverrideChecker {
8586
var visited = new Set<InterfaceType>();
8687
do {
8788
visited.add(current);
88-
current.mixins.reversed
89-
.forEach((m) => _checkIndividualOverridesFromClass(node, m, seen));
90-
_checkIndividualOverridesFromClass(node, current.superclass, seen);
89+
current.mixins.reversed.forEach(
90+
(m) => _checkIndividualOverridesFromClass(node, m, seen, true));
91+
_checkIndividualOverridesFromClass(node, current.superclass, seen, true);
9192
current = current.superclass;
9293
} while (!current.isObject && !visited.contains(current));
9394
}
@@ -171,10 +172,10 @@ class _OverrideChecker {
171172
// Check direct overrides on [type]
172173
for (var interfaceType in interfaces) {
173174
if (node != null) {
174-
_checkIndividualOverridesFromClass(node, interfaceType, seen);
175+
_checkIndividualOverridesFromClass(node, interfaceType, seen, false);
175176
} else {
176177
_checkIndividualOverridesFromType(
177-
type, interfaceType, errorLocation, seen);
178+
type, interfaceType, errorLocation, seen, false);
178179
}
179180
}
180181

@@ -186,7 +187,7 @@ class _OverrideChecker {
186187
// We copy [seen] so we can report separately if more than one mixin or
187188
// the base class have an invalid override.
188189
_checkIndividualOverridesFromType(
189-
type.mixins[i], interfaceType, loc, new Set.from(seen));
190+
type.mixins[i], interfaceType, loc, new Set.from(seen), false);
190191
}
191192
}
192193

@@ -212,12 +213,16 @@ class _OverrideChecker {
212213
/// is used when invoking this function multiple times when checking several
213214
/// types in a class hierarchy. Errors are reported only the first time an
214215
/// invalid override involving a specific member is encountered.
215-
_checkIndividualOverridesFromType(InterfaceType subType,
216-
InterfaceType baseType, AstNode errorLocation, Set<String> seen) {
216+
_checkIndividualOverridesFromType(
217+
InterfaceType subType,
218+
InterfaceType baseType,
219+
AstNode errorLocation,
220+
Set<String> seen,
221+
bool isSubclass) {
217222
void checkHelper(ExecutableElement e) {
218223
if (e.isStatic) return;
219224
if (seen.contains(e.name)) return;
220-
if (_checkSingleOverride(e, baseType, null, errorLocation)) {
225+
if (_checkSingleOverride(e, baseType, null, errorLocation, isSubclass)) {
221226
seen.add(e.name);
222227
}
223228
}
@@ -230,8 +235,8 @@ class _OverrideChecker {
230235
///
231236
/// The [errorLocation] node indicates where errors are reported, see
232237
/// [_checkSingleOverride] for more details.
233-
_checkIndividualOverridesFromClass(
234-
ClassDeclaration node, InterfaceType baseType, Set<String> seen) {
238+
_checkIndividualOverridesFromClass(ClassDeclaration node,
239+
InterfaceType baseType, Set<String> seen, bool isSubclass) {
235240
for (var member in node.members) {
236241
if (member is ConstructorDeclaration) continue;
237242
if (member is FieldDeclaration) {
@@ -242,10 +247,12 @@ class _OverrideChecker {
242247
if (seen.contains(name)) continue;
243248
var getter = element.getter;
244249
var setter = element.setter;
245-
bool found = _checkSingleOverride(getter, baseType, variable, member);
250+
bool found = _checkSingleOverride(
251+
getter, baseType, variable, member, isSubclass);
246252
if (!variable.isFinal &&
247253
!variable.isConst &&
248-
_checkSingleOverride(setter, baseType, variable, member)) {
254+
_checkSingleOverride(
255+
setter, baseType, variable, member, isSubclass)) {
249256
found = true;
250257
}
251258
if (found) seen.add(name);
@@ -254,7 +261,8 @@ class _OverrideChecker {
254261
if ((member as MethodDeclaration).isStatic) continue;
255262
var method = (member as MethodDeclaration).element;
256263
if (seen.contains(method.name)) continue;
257-
if (_checkSingleOverride(method, baseType, member, member)) {
264+
if (_checkSingleOverride(
265+
method, baseType, member, member, isSubclass)) {
258266
seen.add(method.name);
259267
}
260268
}
@@ -288,14 +296,23 @@ class _OverrideChecker {
288296
/// the AST node that defines [element]. This is used to determine whether the
289297
/// type of the element could be inferred from the types in the super classes.
290298
bool _checkSingleOverride(ExecutableElement element, InterfaceType type,
291-
AstNode node, AstNode errorLocation) {
299+
AstNode node, AstNode errorLocation, bool isSubclass) {
292300
assert(!element.isStatic);
293301

294302
FunctionType subType = _rules.elementType(element);
295303
// TODO(vsm): Test for generic
296304
FunctionType baseType = _getMemberType(type, element);
297-
298305
if (baseType == null) return false;
306+
307+
if (isSubclass && element is PropertyAccessorElement) {
308+
// Disallow any overriding if the base class defines this member
309+
// as a field. We effectively treat fields as final / non-virtual.
310+
PropertyInducingElement field = _getMemberField(type, element);
311+
if (field != null) {
312+
_recordMessage(new InvalidFieldOverride(
313+
errorLocation, element, type, subType, baseType));
314+
}
315+
}
299316
if (!_rules.isAssignable(subType, baseType)) {
300317
// See whether non-assignable cases fit one of our common patterns:
301318
//
@@ -948,6 +965,33 @@ class CodeChecker extends RecursiveAstVisitor {
948965
}
949966
}
950967

968+
// Return the field on type corresponding to member, or null if none
969+
// exists or the "field" is actually a getter/setter.
970+
PropertyInducingElement _getMemberField(
971+
InterfaceType type, PropertyAccessorElement member) {
972+
String memberName = member.name;
973+
PropertyInducingElement field;
974+
if (member.isGetter) {
975+
// The subclass member is an explicit getter or a field
976+
// - lookup the getter on the superclass.
977+
var getter = type.getGetter(memberName);
978+
if (getter == null || getter.isStatic) return null;
979+
field = getter.variable;
980+
} else if (!member.isSynthetic) {
981+
// The subclass member is an explicit setter
982+
// - lookup the setter on the superclass.
983+
// Note: an implicit (synthetic) setter would have already been flagged on
984+
// the getter above.
985+
var setter = type.getSetter(memberName);
986+
if (setter == null || setter.isStatic) return null;
987+
field = setter.variable;
988+
} else {
989+
return null;
990+
}
991+
if (field.isSynthetic) return null;
992+
return field;
993+
}
994+
951995
/// Looks up the declaration that matches [member] in [type] and returns it's
952996
/// declared type.
953997
FunctionType _getMemberType(InterfaceType type, ExecutableElement member) =>
@@ -962,6 +1006,15 @@ _MemberTypeGetter _memberTypeGetter(ExecutableElement member) {
9621006

9631007
FunctionType f(InterfaceType type) {
9641008
ExecutableElement baseMethod;
1009+
1010+
if (member.isPrivate) {
1011+
var subtypeLibrary = member.library;
1012+
var baseLibrary = type.element.library;
1013+
if (baseLibrary != subtypeLibrary) {
1014+
return null;
1015+
}
1016+
}
1017+
9651018
try {
9661019
if (isGetter) {
9671020
assert(!isSetter);

‎pkg/analyzer/lib/src/task/strong/info.dart

+9
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,15 @@ class InvalidMethodOverride extends InvalidOverride {
452452
String get message => _messageHelper('Invalid override');
453453
}
454454

455+
class InvalidFieldOverride extends InvalidOverride {
456+
InvalidFieldOverride(AstNode node, ExecutableElement element,
457+
InterfaceType base, DartType subType, DartType baseType)
458+
: super(node, element, base, subType, baseType);
459+
460+
String get message => 'Field declaration {3}.{1} cannot be '
461+
'overridden in {0}.';
462+
}
463+
455464
/// Dart constructors have one weird quirk, illustrated with this example:
456465
///
457466
/// class Base {

‎pkg/analyzer/test/src/task/strong/checker_test.dart

+92-7
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,13 @@ void main() {
11321132
}
11331133
11341134
class Child extends Base {
1135+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/A f1; // invalid for getter
1136+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/C f2; // invalid for setter
1137+
/*severe:InvalidFieldOverride*/var f3;
1138+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride,severe:InvalidMethodOverride*/dynamic f4;
1139+
}
1140+
1141+
class Child2 implements Base {
11351142
/*severe:InvalidMethodOverride*/A f1; // invalid for getter
11361143
/*severe:InvalidMethodOverride*/C f2; // invalid for setter
11371144
var f3;
@@ -1140,6 +1147,40 @@ void main() {
11401147
'''
11411148
});
11421149

1150+
testChecker('private override', {
1151+
'/helper.dart': '''
1152+
import 'main.dart' as main;
1153+
1154+
class Base {
1155+
var f1;
1156+
var _f2;
1157+
var _f3;
1158+
get _f4 => null;
1159+
1160+
int _m1();
1161+
}
1162+
1163+
class GrandChild extends main.Child {
1164+
/*severe:InvalidFieldOverride*/var _f2;
1165+
/*severe:InvalidFieldOverride*/var _f3;
1166+
var _f4;
1167+
1168+
/*severe:InvalidMethodOverride*/String _m1();
1169+
}
1170+
''',
1171+
'/main.dart': '''
1172+
import 'helper.dart' as helper;
1173+
1174+
class Child extends helper.Base {
1175+
/*severe:InvalidFieldOverride*/var f1;
1176+
var _f2;
1177+
var _f4;
1178+
1179+
String _m1();
1180+
}
1181+
'''
1182+
});
1183+
11431184
testChecker('getter/getter override', {
11441185
'/main.dart': '''
11451186
class A {}
@@ -1176,6 +1217,13 @@ void main() {
11761217
}
11771218
11781219
class Child extends Base {
1220+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/A get f1 => null;
1221+
/*severe:InvalidFieldOverride*/C get f2 => null;
1222+
/*severe:InvalidFieldOverride*/get f3 => null;
1223+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/dynamic get f4 => null;
1224+
}
1225+
1226+
class Child2 implements Base {
11791227
/*severe:InvalidMethodOverride*/A get f1 => null;
11801228
C get f2 => null;
11811229
get f3 => null;
@@ -1223,6 +1271,20 @@ void main() {
12231271
}
12241272
12251273
class Child extends Base {
1274+
/*severe:InvalidFieldOverride*/B get f1 => null;
1275+
/*severe:InvalidFieldOverride*/B get f2 => null;
1276+
/*severe:InvalidFieldOverride*/B get f3 => null;
1277+
/*severe:InvalidFieldOverride*/B get f4 => null;
1278+
/*severe:InvalidFieldOverride*/B get f5 => null;
1279+
1280+
/*severe:InvalidFieldOverride*/void set f1(A value) {}
1281+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/void set f2(C value) {}
1282+
/*severe:InvalidFieldOverride*/void set f3(value) {}
1283+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/void set f4(dynamic value) {}
1284+
/*severe:InvalidFieldOverride*/set f5(B value) {}
1285+
}
1286+
1287+
class Child2 implements Base {
12261288
B get f1 => null;
12271289
B get f2 => null;
12281290
B get f3 => null;
@@ -1508,17 +1570,33 @@ void main() {
15081570
}
15091571
15101572
class T1 extends Base {
1511-
/*severe:InvalidMethodOverride*/B get f => null;
1573+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/B get f => null;
15121574
}
15131575
15141576
class T2 extends Base {
1515-
/*severe:InvalidMethodOverride*/set f(B b) => null;
1577+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/set f(B b) => null;
15161578
}
15171579
15181580
class T3 extends Base {
1519-
/*severe:InvalidMethodOverride*/final B f;
1581+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride*/final B f;
15201582
}
15211583
class T4 extends Base {
1584+
// two: one for the getter one for the setter.
1585+
/*severe:InvalidFieldOverride,severe:InvalidMethodOverride,severe:InvalidMethodOverride*/B f;
1586+
}
1587+
1588+
class T5 implements Base {
1589+
/*severe:InvalidMethodOverride*/B get f => null;
1590+
}
1591+
1592+
class T6 implements Base {
1593+
/*severe:InvalidMethodOverride*/set f(B b) => null;
1594+
}
1595+
1596+
class T7 implements Base {
1597+
/*severe:InvalidMethodOverride*/final B f;
1598+
}
1599+
class T8 implements Base {
15221600
// two: one for the getter one for the setter.
15231601
/*severe:InvalidMethodOverride,severe:InvalidMethodOverride*/B f;
15241602
}
@@ -1546,12 +1624,14 @@ void main() {
15461624
15471625
class Grandparent {
15481626
m(A a) {}
1627+
int x;
15491628
}
15501629
class Parent extends Grandparent {
15511630
}
15521631
15531632
class Test extends Parent {
15541633
/*severe:InvalidMethodOverride*/m(B a) {}
1634+
/*severe:InvalidFieldOverride*/int x;
15551635
}
15561636
'''
15571637
});
@@ -1600,17 +1680,20 @@ void main() {
16001680
16011681
class Base {
16021682
m(A a) {}
1683+
int x;
16031684
}
16041685
16051686
class M1 {
16061687
m(B a) {}
16071688
}
16081689
1609-
class M2 {}
1690+
class M2 {
1691+
int x;
1692+
}
16101693
16111694
class T1 extends Base with /*severe:InvalidMethodOverride*/M1 {}
1612-
class T2 extends Base with /*severe:InvalidMethodOverride*/M1, M2 {}
1613-
class T3 extends Base with M2, /*severe:InvalidMethodOverride*/M1 {}
1695+
class T2 extends Base with /*severe:InvalidMethodOverride*/M1, /*severe:InvalidFieldOverride*/M2 {}
1696+
class T3 extends Base with /*severe:InvalidFieldOverride*/M2, /*severe:InvalidMethodOverride*/M1 {}
16141697
'''
16151698
});
16161699

@@ -1624,13 +1707,15 @@ void main() {
16241707
16251708
class M1 {
16261709
m(B a) {}
1710+
int x;
16271711
}
16281712
16291713
class M2 {
16301714
m(A a) {}
1715+
int x;
16311716
}
16321717
1633-
class T1 extends Base with M1, /*severe:InvalidMethodOverride*/M2 {}
1718+
class T1 extends Base with M1, /*severe:InvalidMethodOverride,severe:InvalidFieldOverride*/M2 {}
16341719
'''
16351720
});
16361721

0 commit comments

Comments
 (0)