Skip to content

Commit fb705a6

Browse files
pcjpcj
committed
Implements override keyword checks and the --noImplicitOverride compiler option.
* Moves `OverrideKeyword` to the more appropriate contextual keyword section (types.ts) * Introduces the `CompilerOptions.noImplicitOverride` boolean option (types.ts). * Moves the location for performing override assertions to `checkKindsOfPropertyMemberOverrides` as suggested (checker.ts). * Adds several other diagnostic messages (diagnosticMessages.json). * Improves the breadth of the `overrideKeyword.ts` test case. * Adds a new `noImplicitOverride.ts` test case.
1 parent 65f98b7 commit fb705a6

14 files changed

+881
-68
lines changed

‎src/compiler/checker.ts‎

Lines changed: 119 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -15756,6 +15756,10 @@ namespace ts {
1575615756
checkGrammarDecorators(node) || checkGrammarModifiers(node) || checkGrammarProperty(node) || checkGrammarComputedPropertyName(node.name);
1575715757

1575815758
checkVariableLikeDeclaration(node);
15759+
15760+
if (getModifierFlags(node) & ModifierFlags.Override) {
15761+
checkOverrideDeclaration(node);
15762+
}
1575915763
}
1576015764

1576115765
function checkMethodDeclaration(node: MethodDeclaration) {
@@ -15771,20 +15775,15 @@ namespace ts {
1577115775
error(node, Diagnostics.Method_0_cannot_have_an_implementation_because_it_is_marked_abstract, declarationNameToString(node.name));
1577215776
}
1577315777

15774-
// Is this the correct time to make assertions against the inheritance chain?
15775-
// Have all other methods been resolved? Probably need to record that an override exists
15776-
// and perform the actual resolution later.
1577715778
if (getModifierFlags(node) & ModifierFlags.Override) {
15778-
checkOverrideMethodDeclaration(node);
15779+
checkOverrideDeclaration(node);
1577915780
}
1578015781
}
1578115782

15782-
function checkOverrideMethodDeclaration(node: MethodDeclaration) {
15783-
forEachEnclosingClass(node, enclosingClass => {
15784-
// TODO: save the methodDeclaration node here in a cache and
15785-
// perform the actual assertion later.
15786-
console.log(`override method ${node.symbol.name} is enclosed by class ${enclosingClass.symbol.name}`);
15787-
});
15783+
function checkOverrideDeclaration(node: MethodDeclaration | PropertyDeclaration | AccessorDeclaration) {
15784+
if (node.questionToken) {
15785+
error(node, Diagnostics.override_modifier_cannot_be_used_with_an_optional_property_declaration);
15786+
}
1578815787
}
1578915788

1579015789
function checkConstructorDeclaration(node: ConstructorDeclaration) {
@@ -15899,6 +15898,11 @@ namespace ts {
1589915898
checkGrammarFunctionLikeDeclaration(node) || checkGrammarAccessor(node) || checkGrammarComputedPropertyName(node.name);
1590015899

1590115900
checkDecorators(node);
15901+
15902+
if (getModifierFlags(node) & ModifierFlags.Override) {
15903+
checkOverrideDeclaration(node);
15904+
}
15905+
1590215906
checkSignatureDeclaration(node);
1590315907
if (node.kind === SyntaxKind.GetAccessor) {
1590415908
if (!isInAmbientContext(node) && nodeIsPresent(node.body) && (node.flags & NodeFlags.HasImplicitReturn)) {
@@ -15925,6 +15929,9 @@ namespace ts {
1592515929
if (hasModifier(node, ModifierFlags.Abstract) !== hasModifier(otherAccessor, ModifierFlags.Abstract)) {
1592615930
error(node.name, Diagnostics.Accessors_must_both_be_abstract_or_non_abstract);
1592715931
}
15932+
if (hasModifier(node, ModifierFlags.Override) !== hasModifier(otherAccessor, ModifierFlags.Override)) {
15933+
error(node.name, Diagnostics.Accessors_must_both_be_override_or_non_override);
15934+
}
1592815935

1592915936
// TypeScript 1.0 spec (April 2014): 4.5
1593015937
// If both accessors include type annotations, the specified types must be identical.
@@ -15944,6 +15951,7 @@ namespace ts {
1594415951
else {
1594515952
checkNodeDeferred(node);
1594615953
}
15954+
1594715955
}
1594815956

1594915957
function checkAccessorDeclarationTypesIdentical(first: AccessorDeclaration, second: AccessorDeclaration, getAnnotatedType: (a: AccessorDeclaration) => Type, message: DiagnosticMessage) {
@@ -16119,6 +16127,9 @@ namespace ts {
1611916127
else if (deviation & ModifierFlags.Abstract) {
1612016128
error(o.name, Diagnostics.Overload_signatures_must_all_be_abstract_or_non_abstract);
1612116129
}
16130+
else if (deviation & ModifierFlags.Override) {
16131+
error(o.name, Diagnostics.Overload_signatures_must_all_be_override_or_non_override);
16132+
}
1612216133
});
1612316134
}
1612416135
}
@@ -18318,6 +18329,13 @@ namespace ts {
1831818329
checkKindsOfPropertyMemberOverrides(type, baseType);
1831918330
}
1832018331
}
18332+
else {
18333+
const properties = createMap<Symbol>();
18334+
for (const prop of getPropertiesOfObjectType(type)) {
18335+
properties[prop.name] = prop;
18336+
}
18337+
checkImplicitPropertyMemberOverrides(type, properties);
18338+
}
1832118339

1832218340
const implementedTypeNodes = getClassImplementsHeritageClauseElements(node);
1832318341
if (implementedTypeNodes) {
@@ -18370,6 +18388,44 @@ namespace ts {
1837018388
return forEach(symbol.declarations, d => isClassLike(d) ? d : undefined);
1837118389
}
1837218390

18391+
function checkImplicitPropertyMemberOverrides(type: InterfaceType, propertiesToCheck: Map<Symbol>): void {
18392+
// If the class does not explicitly declare 'extends Object',
18393+
// declarations that mask 'Object' members ('toString()', 'hasOwnProperty()', etc...)
18394+
// are considered here.
18395+
const objectType = getSymbol(globals, "Object", SymbolFlags.Type);
18396+
if (!objectType) {
18397+
return;
18398+
}
18399+
for (const name in propertiesToCheck) {
18400+
const derived = getTargetSymbol(propertiesToCheck[name]);
18401+
const derivedDeclarationFlags = getDeclarationModifierFlagsFromSymbol(derived);
18402+
const found = objectType.members[name];
18403+
if (found) {
18404+
if (compilerOptions.noImplicitOverride) {
18405+
const foundSymbol = getTargetSymbol(found);
18406+
let detail = "masks Object." + symbolToString(found);
18407+
// assert that the type of the derived
18408+
// property matches that of the base property.
18409+
if (!isPropertyIdenticalTo(derived, foundSymbol)) {
18410+
detail += "). The override declaration ("
18411+
+ typeToString(getTypeOfSymbol(derived))
18412+
+ ") also has a different type signature than the original ("
18413+
+ typeToString(getTypeOfSymbol(foundSymbol));
18414+
}
18415+
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_1,
18416+
symbolToString(derived), detail);
18417+
}
18418+
}
18419+
// No matching property found on the object type. It
18420+
// is an error for the derived property to falsely
18421+
// claim 'override'.
18422+
else if (derivedDeclarationFlags & ModifierFlags.Override) {
18423+
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_was_marked_override_but_no_matching_definition_was_found_in_any_supertype_of_1,
18424+
symbolToString(derived), typeToString(type));
18425+
}
18426+
}
18427+
}
18428+
1837318429
function checkKindsOfPropertyMemberOverrides(type: InterfaceType, baseType: ObjectType): void {
1837418430

1837518431
// TypeScript 1.0 spec (April 2014): 8.2.3
@@ -18387,9 +18443,17 @@ namespace ts {
1838718443
// derived class instance member variables and accessors, but not by other kinds of members.
1838818444

1838918445
// NOTE: assignability is checked in checkClassDeclaration
18446+
18447+
// Track which symbols in the derived class have not been seen.
18448+
const onlyInDerived = createMap<Symbol>();
18449+
for (const prop of getPropertiesOfObjectType(type)) {
18450+
onlyInDerived[prop.name] = prop;
18451+
}
18452+
1839018453
const baseProperties = getPropertiesOfObjectType(baseType);
1839118454
for (const baseProperty of baseProperties) {
1839218455
const base = getTargetSymbol(baseProperty);
18456+
delete onlyInDerived[base.name];
1839318457

1839418458
if (base.flags & SymbolFlags.Prototype) {
1839518459
continue;
@@ -18422,6 +18486,7 @@ namespace ts {
1842218486
typeToString(type), symbolToString(baseProperty), typeToString(baseType));
1842318487
}
1842418488
}
18489+
1842518490
}
1842618491
else {
1842718492
// derived overrides base.
@@ -18438,6 +18503,16 @@ namespace ts {
1843818503

1843918504
if ((base.flags & derived.flags & SymbolFlags.Method) || ((base.flags & SymbolFlags.PropertyOrAccessor) && (derived.flags & SymbolFlags.PropertyOrAccessor))) {
1844018505
// method is overridden with method or property/accessor is overridden with property/accessor - correct case
18506+
18507+
// Before accepting the correct case, ensure 'override' is marked if --noImplicitOverride is true.
18508+
// Abstract members are an exception as override checks are suspended until the implementation solidifies.
18509+
if (compilerOptions.noImplicitOverride
18510+
&& !(derivedDeclarationFlags & ModifierFlags.Abstract)
18511+
&& !(derivedDeclarationFlags & ModifierFlags.Override)) {
18512+
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_1,
18513+
symbolToString(derived), "inherited from " + typeToString(baseType));
18514+
}
18515+
1844118516
continue;
1844218517
}
1844318518

@@ -18465,6 +18540,8 @@ namespace ts {
1846518540
}
1846618541
}
1846718542
}
18543+
18544+
checkImplicitPropertyMemberOverrides(type, onlyInDerived);
1846818545
}
1846918546

1847018547
function isAccessor(kind: SyntaxKind): boolean {
@@ -21075,38 +21152,38 @@ namespace ts {
2107521152
flags |= ModifierFlags.Abstract;
2107621153
break;
2107721154

21078-
case SyntaxKind.AsyncKeyword:
21079-
if (flags & ModifierFlags.Async) {
21080-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "async");
21081-
}
21082-
else if (flags & ModifierFlags.Ambient || isInAmbientContext(node.parent)) {
21083-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_in_an_ambient_context, "async");
21084-
}
21085-
else if (node.kind === SyntaxKind.Parameter) {
21086-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_parameter, "async");
21087-
}
21088-
flags |= ModifierFlags.Async;
21089-
lastAsync = modifier;
21090-
break;
21091-
21092-
case SyntaxKind.OverrideKeyword:
21093-
if (flags & ModifierFlags.Override) {
21094-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "override");
21095-
}
21096-
else if (flags & ModifierFlags.Static) {
21097-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "override");
21098-
}
21099-
else if (flags & ModifierFlags.Private) {
21100-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "override");
21101-
}
21102-
else if (flags & ModifierFlags.Abstract) {
21103-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "abstract", "override");
21104-
}
21105-
else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {
21106-
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_module_or_namespace_element, "override");
21107-
}
21108-
flags |= ModifierFlags.Override;
21109-
break;
21155+
case SyntaxKind.OverrideKeyword:
21156+
if (flags & ModifierFlags.Override) {
21157+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "override");
21158+
}
21159+
else if (flags & ModifierFlags.Static) {
21160+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "override");
21161+
}
21162+
else if (flags & ModifierFlags.Private) {
21163+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "override");
21164+
}
21165+
else if (flags & ModifierFlags.Abstract) {
21166+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "abstract", "override");
21167+
}
21168+
else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {
21169+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_module_or_namespace_element, "override");
21170+
}
21171+
flags |= ModifierFlags.Override;
21172+
break;
21173+
21174+
case SyntaxKind.AsyncKeyword:
21175+
if (flags & ModifierFlags.Async) {
21176+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "async");
21177+
}
21178+
else if (flags & ModifierFlags.Ambient || isInAmbientContext(node.parent)) {
21179+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_in_an_ambient_context, "async");
21180+
}
21181+
else if (node.kind === SyntaxKind.Parameter) {
21182+
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_parameter, "async");
21183+
}
21184+
flags |= ModifierFlags.Async;
21185+
lastAsync = modifier;
21186+
break;
2111021187
}
2111121188
}
2111221189

‎src/compiler/commandLineParser.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ namespace ts {
147147
{
148148
name: "noImplicitOverride",
149149
type: "boolean",
150-
description: Diagnostics.Member_function_0_was_marked_override_but_no_matching_definition_was_found_in_the_superclass_chain_1,
150+
description: Diagnostics.Raise_an_error_when_inherited_Slashoverridden_class_members_are_not_explicitly_marked_override,
151151
},
152152
{
153153
name: "noImplicitThis",

‎src/compiler/declarationEmitter.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,9 @@ namespace ts {
717717
}
718718

719719
function emitClassMemberDeclarationFlags(flags: ModifierFlags) {
720+
if (flags & ModifierFlags.Override) {
721+
write("override ");
722+
}
720723
if (flags & ModifierFlags.Private) {
721724
write("private ");
722725
}

‎src/compiler/diagnosticMessages.json‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1683,6 +1683,10 @@
16831683
"category": "Error",
16841684
"code": 2518
16851685
},
1686+
"Overload signatures must all be override or non-override.": {
1687+
"category": "Error",
1688+
"code": 2519
1689+
},
16861690
"Duplicate identifier '{0}'. Compiler uses declaration '{1}' to support async functions.": {
16871691
"category": "Error",
16881692
"code": 2520
@@ -2015,6 +2019,10 @@
20152019
"category": "Error",
20162020
"code": 2698
20172021
},
2022+
"Accessors must both be override or non-override.": {
2023+
"category": "Error",
2024+
"code": 2699
2025+
},
20182026
"Rest types may only be created from object types.": {
20192027
"category": "Error",
20202028
"code": 2700
@@ -3279,10 +3287,26 @@
32793287
"category": "Message",
32803288
"code": 90015
32813289
},
3282-
"Member function {0} was marked 'override', but no matching definition was found in the superclass chain ({1})": {
3290+
"Raise an error when inherited/overridden class members are not explicitly marked 'override'": {
32833291
"category": "Error",
32843292
"code": 90030
32853293
},
3294+
"Class member '{0}' was marked 'override', but no matching definition was found in any supertype of '{1}'": {
3295+
"category": "Error",
3296+
"code": 90032
3297+
},
3298+
"Class member '{0}' must be marked 'override' when noImplicitOverride is enabled ({1})": {
3299+
"category": "Error",
3300+
"code": 90033
3301+
},
3302+
"override modifier cannot be used with an optional property declaration": {
3303+
"category": "Error",
3304+
"code": 90034
3305+
},
3306+
"All declarations of an override method must be consecutive.": {
3307+
"category": "Error",
3308+
"code": 90035
3309+
},
32863310
"Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": {
32873311
"category": "Error",
32883312
"code": 8017

‎src/compiler/emitter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ namespace ts {
324324
case SyntaxKind.PrivateKeyword:
325325
case SyntaxKind.ProtectedKeyword:
326326
case SyntaxKind.PublicKeyword:
327-
case SyntaxKind.OverrideKeyword:
328327
case SyntaxKind.StaticKeyword:
329328

330329
// Contextual keywords
@@ -341,6 +340,7 @@ namespace ts {
341340
case SyntaxKind.ModuleKeyword:
342341
case SyntaxKind.NamespaceKeyword:
343342
case SyntaxKind.NeverKeyword:
343+
case SyntaxKind.OverrideKeyword:
344344
case SyntaxKind.ReadonlyKeyword:
345345
case SyntaxKind.RequireKeyword:
346346
case SyntaxKind.NumberKeyword:

‎src/compiler/types.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,6 @@ namespace ts {
155155
PrivateKeyword,
156156
ProtectedKeyword,
157157
PublicKeyword,
158-
OverrideKeyword,
159158
StaticKeyword,
160159
YieldKeyword,
161160
// Contextual keywords
@@ -184,6 +183,7 @@ namespace ts {
184183
UndefinedKeyword,
185184
FromKeyword,
186185
GlobalKeyword,
186+
OverrideKeyword,
187187
OfKeyword, // LastKeyword and LastToken
188188

189189
// Parse tree nodes
@@ -3195,6 +3195,7 @@ namespace ts {
31953195
noErrorTruncation?: boolean;
31963196
noFallthroughCasesInSwitch?: boolean;
31973197
noImplicitAny?: boolean;
3198+
noImplicitOverride?: boolean;
31983199
noImplicitReturns?: boolean;
31993200
noImplicitThis?: boolean;
32003201
noUnusedLocals?: boolean;

‎src/harness/unittests/transpile.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,10 @@ var x = 0;`, {
341341
options: { compilerOptions: { noImplicitAny: true }, fileName: "input.js", reportDiagnostics: true }
342342
});
343343

344+
transpilesCorrectly("Supports setting 'noImplicitOverride'", "x;", {
345+
options: { compilerOptions: { noImplicitOverride: true }, fileName: "input.js", reportDiagnostics: true }
346+
});
347+
344348
transpilesCorrectly("Supports setting 'noImplicitReturns'", "x;", {
345349
options: { compilerOptions: { noImplicitReturns: true }, fileName: "input.js", reportDiagnostics: true }
346350
});

0 commit comments

Comments
 (0)