From 905caad677422399f769e32a76e49feb02f9cc74 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 23 Sep 2021 15:02:32 -0700 Subject: [PATCH] expose check override modifier in checker --- src/compiler/checker.ts | 198 ++++++++++++++---- src/compiler/types.ts | 8 + src/services/completions.ts | 6 +- .../fourslash/completionsOverridingMethod.ts | 33 +++ 4 files changed, 209 insertions(+), 36 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index b71cfe8f3e9..6c47db65622 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -720,6 +720,7 @@ namespace ts { getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, isDeclarationVisible, isPropertyAccessible, + getMemberOverrideModifierDiagnostic, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { @@ -37781,7 +37782,7 @@ namespace ts { } } - checkMembersForMissingOverrideModifier(node, type, typeWithThis, staticType); + checkMembersForOverrideModifier(node, type, typeWithThis, staticType); const implementedTypeNodes = getEffectiveImplementsTypeNodes(node); if (implementedTypeNodes) { @@ -37818,8 +37819,7 @@ namespace ts { } } - function checkMembersForMissingOverrideModifier(node: ClassLikeDeclaration, type: InterfaceType, typeWithThis: Type, staticType: ObjectType) { - const nodeInAmbientContext = !!(node.flags & NodeFlags.Ambient); + function checkMembersForOverrideModifier(node: ClassLikeDeclaration, type: InterfaceType, typeWithThis: Type, staticType: ObjectType) { const baseTypeNode = getEffectiveBaseTypeNode(node); const baseTypes = baseTypeNode && getBaseTypes(type); const baseWithThis = baseTypes?.length ? getTypeWithThisArgument(first(baseTypes), type.thisType) : undefined; @@ -37833,56 +37833,142 @@ namespace ts { if (isConstructorDeclaration(member)) { forEach(member.parameters, param => { if (isParameterPropertyDeclaration(param, member)) { - checkClassMember(param, /*memberIsParameterProperty*/ true); + checkExistingMemberForOverrideModifier( + node, + staticType, + baseStaticType, + baseWithThis, + type, + typeWithThis, + param, + /* memberIsParameterProperty */ true + ); } }); } - checkClassMember(member); + checkExistingMemberForOverrideModifier( + node, + staticType, + baseStaticType, + baseWithThis, + type, + typeWithThis, + member, + /* memberIsParameterProperty */ false, + ); + } + } + + /** + * @param member Existing member node to be checked. + * Note: `member` cannot be a synthetic node. + */ + function checkExistingMemberForOverrideModifier( + node: ClassLikeDeclaration, + staticType: ObjectType, + baseStaticType: Type, + baseWithThis: Type | undefined, + type: InterfaceType, + typeWithThis: Type, + member: ClassElement | ParameterPropertyDeclaration, + memberIsParameterProperty: boolean, + reportErrors = true, + ): MemberOverrideDiagnostic { + const declaredProp = member.name + && getSymbolAtLocation(member.name) + || getSymbolAtLocation(member); + if (!declaredProp) { + return MemberOverrideDiagnostic.Ok; } - function checkClassMember(member: ClassElement | ParameterPropertyDeclaration, memberIsParameterProperty?: boolean) { - const hasOverride = hasOverrideModifier(member); - const hasStatic = isStatic(member); - if (baseWithThis && (hasOverride || compilerOptions.noImplicitOverride)) { - const declaredProp = member.name && getSymbolAtLocation(member.name) || getSymbolAtLocation(member); - if (!declaredProp) { - return; - } + return checkMemberForOverrideModifier( + node, + staticType, + baseStaticType, + baseWithThis, + type, + typeWithThis, + hasOverrideModifier(member), + hasAbstractModifier(member), + isStatic(member), + memberIsParameterProperty, + symbolName(declaredProp), + reportErrors ? member : undefined, + ); + } - const thisType = hasStatic ? staticType : typeWithThis; - const baseType = hasStatic ? baseStaticType : baseWithThis; - const prop = getPropertyOfType(thisType, declaredProp.escapedName); - const baseProp = getPropertyOfType(baseType, declaredProp.escapedName); + /** + * Checks a class member declaration for either a missing or an invalid `override` modifier. + * Note: this function can be used for speculative checking, + * i.e. checking a member that does not yet exist in the program. + * An example of that would be to call this function in a completions scenario, + * when offering a method declaration as completion. + * @param errorNode The node where we should report an error, or undefined if we should not report errors. + */ + function checkMemberForOverrideModifier( + node: ClassLikeDeclaration, + staticType: ObjectType, + baseStaticType: Type, + baseWithThis: Type | undefined, + type: InterfaceType, + typeWithThis: Type, + memberHasOverrideModifier: boolean, // >> Note: we need this because this is computed from a node, but we don't have any (it would be synthetic) + memberHasAbstractModifier: boolean, + memberIsStatic: boolean, + memberIsParameterProperty: boolean, + memberName: string, + errorNode?: Node, + ): MemberOverrideDiagnostic { + const nodeInAmbientContext = !!(node.flags & NodeFlags.Ambient); + if (baseWithThis && (memberHasOverrideModifier || compilerOptions.noImplicitOverride)) { + const memberEscapedName = escapeLeadingUnderscores(memberName); + const thisType = memberIsStatic ? staticType : typeWithThis; + const baseType = memberIsStatic ? baseStaticType : baseWithThis; + const prop = getPropertyOfType(thisType, memberEscapedName); + const baseProp = getPropertyOfType(baseType, memberEscapedName); - const baseClassName = typeToString(baseWithThis); - if (prop && !baseProp && hasOverride) { - const suggestion = getSuggestedSymbolForNonexistentClassMember(symbolName(declaredProp), baseType); + const baseClassName = typeToString(baseWithThis); + if (prop && !baseProp && memberHasOverrideModifier) { + if (errorNode) { + const suggestion = getSuggestedSymbolForNonexistentClassMember(memberName, baseType); // Again, using symbol name: note that's different from `symbol.escapedName` suggestion ? - error(member, Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1, baseClassName, symbolToString(suggestion)) : - error(member, Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0, baseClassName); + error(errorNode, Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1, baseClassName, symbolToString(suggestion)) : + error(errorNode, Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0, baseClassName); + } + return MemberOverrideDiagnostic.HasInvalidOverride; + } + else if (prop && baseProp?.declarations && compilerOptions.noImplicitOverride && !nodeInAmbientContext) { + const baseHasAbstract = some(baseProp.declarations, hasAbstractModifier); + if (memberHasOverrideModifier) { + return MemberOverrideDiagnostic.Ok; } - else if (prop && baseProp?.declarations && compilerOptions.noImplicitOverride && !nodeInAmbientContext) { - const baseHasAbstract = some(baseProp.declarations, hasAbstractModifier); - if (hasOverride) { - return; - } - if (!baseHasAbstract) { + if (!baseHasAbstract) { + if (errorNode) { const diag = memberIsParameterProperty ? Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0 : Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0; - error(member, diag, baseClassName); + error(errorNode, diag, baseClassName); } - else if (hasAbstractModifier(member) && baseHasAbstract) { - error(member, Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0, baseClassName); + return MemberOverrideDiagnostic.NeedsOverride; + } + else if (memberHasAbstractModifier && baseHasAbstract) { + if (errorNode) { + error(errorNode, Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0, baseClassName); } + return MemberOverrideDiagnostic.NeedsOverride; } } - else if (hasOverride) { - const className = typeToString(type); - error(member, Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class, className); - } } + else if (memberHasOverrideModifier) { + if (errorNode) { + const className = typeToString(type); + error(errorNode, Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class, className); + } + return MemberOverrideDiagnostic.HasInvalidOverride; + } + + return MemberOverrideDiagnostic.Ok; } function issueMemberSpecificError(node: ClassLikeDeclaration, typeWithThis: Type, baseWithThis: Type, broadDiag: DiagnosticMessage) { @@ -37929,6 +38015,48 @@ namespace ts { } } + /** + * Checks a member declaration node to see if has a missing or invalid `override` modifier. + * @param node Class-like node where the member is declared. + * @param member Member declaration node. + * Note: `member` can be a synthetic node without a parent. + */ + function getMemberOverrideModifierDiagnostic(node: ClassLikeDeclaration, member: ClassElement): MemberOverrideDiagnostic { + if (!member.name) { + return MemberOverrideDiagnostic.Ok; + } + + const symbol = getSymbolOfNode(node); + const type = getDeclaredTypeOfSymbol(symbol) as InterfaceType; + const typeWithThis = getTypeWithThisArgument(type); + const staticType = getTypeOfSymbol(symbol) as ObjectType; + + const baseTypeNode = getEffectiveBaseTypeNode(node); + const baseTypes = baseTypeNode && getBaseTypes(type); + const baseWithThis = baseTypes?.length ? getTypeWithThisArgument(first(baseTypes), type.thisType) : undefined; + const baseStaticType = getBaseConstructorTypeOfClass(type); + + const memberHasOverrideModifier = member.parent + ? hasOverrideModifier(member) + : hasSyntacticModifier(member, ModifierFlags.Override); + + const memberName = unescapeLeadingUnderscores(getTextOfPropertyName(member.name)); + + return checkMemberForOverrideModifier( + node, + staticType, + baseStaticType, + baseWithThis, + type, + typeWithThis, + memberHasOverrideModifier, + hasAbstractModifier(member), + isStatic(member), + /* memberIsParameterProperty */ false, + memberName, + ); + } + function getTargetSymbol(s: Symbol) { // if symbol is instantiated its flags are not copied from the 'target' // so we'll need to get back original 'target' symbol to work with correct set of flags diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 08efc5d5457..3c46e680571 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4363,6 +4363,14 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; /* @internal */ isPropertyAccessible(node: Node, isSuper: boolean, isWrite: boolean, containingType: Type, property: Symbol): boolean; + /* @internal */ getMemberOverrideModifierDiagnostic(node: ClassLikeDeclaration, member: ClassElement): MemberOverrideDiagnostic; + } + + /* @internal */ + export const enum MemberOverrideDiagnostic { + Ok, + NeedsOverride, + HasInvalidOverride } /* @internal */ diff --git a/src/services/completions.ts b/src/services/completions.ts index 700b1e0fe42..c1ba3f4045a 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -746,6 +746,8 @@ namespace ts.Completions { let isSnippet: true | undefined; let insertText: string = name; + + const checker = program.getTypeChecker(); const sourceFile = location.getSourceFile(); const printer = createPrinter({ removeComments: true, @@ -754,6 +756,7 @@ namespace ts.Completions { omitTrailingSemicolon: true, }); const importAdder = codefix.createImportAdder(sourceFile, program, preferences, host); + let body; if (preferences.includeCompletionsWithSnippetText) { isSnippet = true; @@ -803,7 +806,8 @@ namespace ts.Completions { // ); // } // } - if (options.noImplicitOverride && /* TODO: isOverride(node) */ undefined) { + if (isClassElement(node) + && checker.getMemberOverrideModifierDiagnostic(classLikeDeclaration, node) === MemberOverrideDiagnostic.NeedsOverride) { node = factory.updateModifiers( node, concatenate([factory.createModifier(SyntaxKind.OverrideKeyword)], node.modifiers), diff --git a/tests/cases/fourslash/completionsOverridingMethod.ts b/tests/cases/fourslash/completionsOverridingMethod.ts index 289b3af3783..3df65b736b4 100644 --- a/tests/cases/fourslash/completionsOverridingMethod.ts +++ b/tests/cases/fourslash/completionsOverridingMethod.ts @@ -81,6 +81,17 @@ //// f/*g*/ ////} +// @Filename: h.ts +// @noImplicitOverride: true // >> TODO: move this to a new test file, because this option is global +// Case: Suggested method needs `override` modifier +////class HBase { +//// foo(a: string): void {} +////} +//// +////class HSub extends HBase { +//// f/*h*/ +////} + // format.setFormatOptions({ // newLineCharacter: "\n", // }); @@ -249,3 +260,25 @@ foo(a: any, b?: any): string {\r\n $1;\r\n}\r\n", ], }); +verify.completions({ + marker: "h", + isNewIdentifierLocation: true, + preferences: { + includeCompletionsWithInsertText: true, + includeCompletionsWithSnippetText: true, + }, + includes: [ + { + name: "foo", + sortText: completion.SortText.LocationPriority, + replacementSpan: { + fileName: "", + pos: 0, + end: 0, + }, + isSnippet: true, + insertText: +"override foo(): void {\r\n $1;\r\n}\r\n", + } + ], +}); \ No newline at end of file