Skip to content

Commit

Permalink
Improves modifier checks and diagnostic messages.
Browse files Browse the repository at this point in the history 
* Rename check{Implicit -> Augmented}PropertyMemberOverrides.
* Disable augment checks in ambients contexts.
* Allow override on static methods.
* Add override keyword to spec.md.
* Use chained diagnostics when possible.
* Remove hardcoded diagnotic-related strings in checker.ts.
* Split tests into separate files overrideKeyword{es5 + es6}.ts.
  • Loading branch information
@pcj
pcj committed Jan 15, 2017
1 parent fb705a6 commit 3143818
Show file tree
Hide file tree
Showing 18 changed files with 2,520 additions and 2,122 deletions.
3,080 changes: 1,540 additions & 1,540 deletions doc/spec.md
View file

Large diffs are not rendered by default.

101 changes: 70 additions & 31 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18334,7 +18334,7 @@ namespace ts {
for (const prop of getPropertiesOfObjectType(type)) {
properties[prop.name] = prop;
}
checkImplicitPropertyMemberOverrides(type, properties);
checkAugmentedPropertyMemberOverrides(type, properties);
}

const implementedTypeNodes = getClassImplementsHeritageClauseElements(node);
Expand Down Expand Up @@ -18388,40 +18388,53 @@ namespace ts {
return forEach(symbol.declarations, d => isClassLike(d) ? d : undefined);
}

function checkImplicitPropertyMemberOverrides(type: InterfaceType, propertiesToCheck: Map<Symbol>): void {
function checkAugmentedPropertyMemberOverrides(type: InterfaceType, propertiesToCheck: Map<Symbol>): void {
// If the class does not explicitly declare 'extends Object',
// declarations that mask 'Object' members ('toString()', 'hasOwnProperty()', etc...)
// are considered here.

// check is disabled in ambient contexts
if (isInAmbientContext(type.symbol.valueDeclaration)) {
return;
}

const objectType = getSymbol(globals, "Object", SymbolFlags.Type);
if (!objectType) {
return;
}

for (const name in propertiesToCheck) {
const derived = getTargetSymbol(propertiesToCheck[name]);
const derivedDeclarationFlags = getDeclarationModifierFlagsFromSymbol(derived);
const found = objectType.members[name];
if (found) {
if (compilerOptions.noImplicitOverride) {
const foundSymbol = getTargetSymbol(found);
let detail = "masks Object." + symbolToString(found);
// assert that the type of the derived
// property matches that of the base property.
let errorInfo = chainDiagnosticMessages(
undefined,
Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_augmented_from_Object_1,
symbolToString(derived),
symbolToString(found)
);
if (!isPropertyIdenticalTo(derived, foundSymbol)) {
detail += "). The override declaration ("
+ typeToString(getTypeOfSymbol(derived))
+ ") also has a different type signature than the original ("
+ typeToString(getTypeOfSymbol(foundSymbol));
errorInfo = chainDiagnosticMessages(
errorInfo,
Diagnostics.Type_0_is_not_assignable_to_type_1,
typeToString(getTypeOfSymbol(derived)),
typeToString(getTypeOfSymbol(foundSymbol))
);
}
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_1,
symbolToString(derived), detail);
diagnostics.add(createDiagnosticForNodeFromMessageChain(derived.valueDeclaration.name, errorInfo));
}
}
// No matching property found on the object type. It
// is an error for the derived property to falsely
// claim 'override'.
else if (derivedDeclarationFlags & ModifierFlags.Override) {
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_was_marked_override_but_no_matching_definition_was_found_in_any_supertype_of_1,
symbolToString(derived), typeToString(type));
error(derived.valueDeclaration.name,
Diagnostics.Class_member_0_was_marked_override_but_no_matching_declaration_was_found_in_any_supertype_of_1,
symbolToString(derived),
typeToString(type));
}
}
}
Expand Down Expand Up @@ -18450,6 +18463,8 @@ namespace ts {
onlyInDerived[prop.name] = prop;
}

const ambient = isInAmbientContext(type.symbol.valueDeclaration);

const baseProperties = getPropertiesOfObjectType(baseType);
for (const baseProperty of baseProperties) {
const base = getTargetSymbol(baseProperty);
Expand Down Expand Up @@ -18507,10 +18522,13 @@ namespace ts {
// Before accepting the correct case, ensure 'override' is marked if --noImplicitOverride is true.
// Abstract members are an exception as override checks are suspended until the implementation solidifies.
if (compilerOptions.noImplicitOverride
&& !ambient
&& !(derivedDeclarationFlags & ModifierFlags.Abstract)
&& !(derivedDeclarationFlags & ModifierFlags.Override)) {
error(derived.valueDeclaration.name, Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_1,
symbolToString(derived), "inherited from " + typeToString(baseType));
error(derived.valueDeclaration.name,
Diagnostics.Class_member_0_must_be_marked_override_when_noImplicitOverride_is_enabled_augmented_from_Object_1,
symbolToString(derived),
typeToString(baseType));
}

continue;
Expand Down Expand Up @@ -18541,7 +18559,7 @@ namespace ts {
}
}

checkImplicitPropertyMemberOverrides(type, onlyInDerived);
checkAugmentedPropertyMemberOverrides(type, onlyInDerived);
}

function isAccessor(kind: SyntaxKind): boolean {
Expand Down Expand Up @@ -21030,9 +21048,6 @@ namespace ts {
else if (flags & ModifierFlags.Async) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, text, "async");
}
else if (flags & ModifierFlags.Override) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, text, "override");
}
else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_module_or_namespace_element, text);
}
Expand All @@ -21044,6 +21059,14 @@ namespace ts {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, text, "abstract");
}
}
else if (flags & ModifierFlags.Override) {
if (modifier.kind === SyntaxKind.PrivateKeyword) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, text, "override");
}
else {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, text, "override");
}
}
flags |= modifierToFlag(modifier.kind);
break;

Expand All @@ -21066,9 +21089,6 @@ namespace ts {
else if (flags & ModifierFlags.Abstract) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "abstract");
}
else if (flags & ModifierFlags.Override) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "override");
}
flags |= ModifierFlags.Static;
lastStatic = modifier;
break;
Expand Down Expand Up @@ -21131,20 +21151,23 @@ namespace ts {
if (flags & ModifierFlags.Abstract) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "abstract");
}
if (node.kind !== SyntaxKind.ClassDeclaration) {
else if (node.kind !== SyntaxKind.ClassDeclaration) {
if (node.kind !== SyntaxKind.MethodDeclaration &&
node.kind !== SyntaxKind.PropertyDeclaration &&
node.kind !== SyntaxKind.GetAccessor &&
node.kind !== SyntaxKind.SetAccessor) {
return grammarErrorOnNode(modifier, Diagnostics.abstract_modifier_can_only_appear_on_a_class_method_or_property_declaration);
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_can_only_appear_on_a_class_method_or_property_declaration, "abstract");
}
if (!(node.parent.kind === SyntaxKind.ClassDeclaration && getModifierFlags(node.parent) & ModifierFlags.Abstract)) {
else if (!(node.parent.kind === SyntaxKind.ClassDeclaration && getModifierFlags(node.parent) & ModifierFlags.Abstract)) {
return grammarErrorOnNode(modifier, Diagnostics.Abstract_methods_can_only_appear_within_an_abstract_class);
}
if (flags & ModifierFlags.Static) {
else if (flags & ModifierFlags.Override) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "override", "abstract");
}
else if (flags & ModifierFlags.Static) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "abstract");
}
if (flags & ModifierFlags.Private) {
else if (flags & ModifierFlags.Private) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "abstract");
}
}
Expand All @@ -21157,17 +21180,33 @@ namespace ts {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "override");
}
else if (flags & ModifierFlags.Static) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "static", "override");
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, "override", "static");
}
else if (flags & ModifierFlags.Private) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "override");
else if (flags & ModifierFlags.Async) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, "override", "async");
}
else if (flags & ModifierFlags.Readonly) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_must_precede_1_modifier, "override", "readonly");
}
else if (flags & ModifierFlags.Abstract) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "abstract", "override");
}
else if (flags & ModifierFlags.Private) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "override");
}
else if (node.kind === SyntaxKind.Parameter) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_parameter, "override");
}
else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_module_or_namespace_element, "override");
}
else if (node.kind !== SyntaxKind.MethodDeclaration &&
node.kind !== SyntaxKind.PropertyDeclaration &&
node.kind !== SyntaxKind.GetAccessor &&
node.kind !== SyntaxKind.SetAccessor) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_can_only_appear_on_a_class_method_or_property_declaration, "override");
}

flags |= ModifierFlags.Override;
break;

Expand All @@ -21191,7 +21230,7 @@ namespace ts {
if (flags & ModifierFlags.Static) {
return grammarErrorOnNode(lastStatic, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "static");
}
if (flags & ModifierFlags.Abstract) {
else if (flags & ModifierFlags.Abstract) {
return grammarErrorOnNode(lastStatic, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "abstract");
}
else if (flags & ModifierFlags.Async) {
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/declarationEmitter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,6 @@ namespace ts {
}

function emitClassMemberDeclarationFlags(flags: ModifierFlags) {
if (flags & ModifierFlags.Override) {
write("override ");
}
if (flags & ModifierFlags.Private) {
write("private ");
}
Expand All @@ -730,6 +727,9 @@ namespace ts {
if (flags & ModifierFlags.Static) {
write("static ");
}
if (flags & ModifierFlags.Override) {
write("override ");
}
if (flags & ModifierFlags.Readonly) {
write("readonly ");
}
Expand Down
14 changes: 9 additions & 5 deletions src/compiler/diagnosticMessages.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@
"category": "Error",
"code": 1241
},
"'abstract' modifier can only appear on a class, method, or property declaration.": {
"'{0}' modifier can only appear on a class, method, or property declaration.": {
"category": "Error",
"code": 1242
},
Expand Down Expand Up @@ -3291,21 +3291,25 @@
"category": "Error",
"code": 90030
},
"Class member '{0}' was marked 'override', but no matching definition was found in any supertype of '{1}'": {
"Class member '{0}' was marked 'override', but no matching declaration was found in any supertype of '{1}'": {
"category": "Error",
"code": 90032
},
"Class member '{0}' must be marked 'override' when noImplicitOverride is enabled ({1})": {
"Class member '{0}' must be marked 'override' when noImplicitOverride is enabled (inherited from {1})": {
"category": "Error",
"code": 90033
},
"Class member '{0}' must be marked 'override' when noImplicitOverride is enabled (augmented from Object.{1})": {
"category": "Error",
"code": 90034
},
"override modifier cannot be used with an optional property declaration": {
"category": "Error",
"code": 90034
"code": 90035
},
"All declarations of an override method must be consecutive.": {
"category": "Error",
"code": 90035
"code": 90036
},
"Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": {
"category": "Error",
Expand Down

0 comments on commit 3143818

Please sign in to comment.