From f3bc891ae95bbc5ce64d87914f25bf229ea31ad9 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 2 Jul 2018 11:26:50 -0700 Subject: [PATCH 1/3] fixAddMissingMember: Improve deduplication in code-fix-all --- src/services/codeFixProvider.ts | 4 +- src/services/codefixes/fixAddMissingMember.ts | 117 +++++++++++++----- src/services/utilities.ts | 34 +++++ .../fourslash/codeFixAddMissingMember_all.ts | 30 +++++ 4 files changed, 149 insertions(+), 36 deletions(-) diff --git a/src/services/codeFixProvider.ts b/src/services/codeFixProvider.ts index 076bf553677ab..58a3e3d1375d5 100644 --- a/src/services/codeFixProvider.ts +++ b/src/services/codeFixProvider.ts @@ -71,7 +71,7 @@ namespace ts { return fixIdToRegistration.get(cast(context.fixId, isString))!.getAllCodeActions!(context); } - function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions { + export function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions { return { changes, commands }; } @@ -89,7 +89,7 @@ namespace ts { return createCombinedCodeActions(changes, commands.length === 0 ? undefined : commands); } - function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void { + export function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void { for (const diag of program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken))) { if (contains(errorCodes, diag.code)) { cb(diag as DiagnosticWithLocation); diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index b63ba394b0b89..8d820cbd3a2f9 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -13,55 +13,104 @@ namespace ts.codefix { if (!info) return undefined; if (info.kind === InfoKind.enum) { - const { token, enumDeclaration } = info; - const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, enumDeclaration)); + const { token, parentDeclaration } = info; + const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, parentDeclaration)); return singleElementArray(createCodeFixAction(fixName, changes, [Diagnostics.Add_missing_enum_member_0, token.text], fixId, Diagnostics.Add_all_missing_enum_members)); } - const { classDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info; - const methodCodeAction = call && getActionForMethodDeclaration(context, classDeclarationSourceFile, classDeclaration, token, call, makeStatic, inJs, context.preferences); + const { parentDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info; + const methodCodeAction = call && getActionForMethodDeclaration(context, classDeclarationSourceFile, parentDeclaration, token, call, makeStatic, inJs, context.preferences); const addMember = inJs ? - singleElementArray(getActionsForAddMissingMemberInJavaScriptFile(context, classDeclarationSourceFile, classDeclaration, token.text, makeStatic)) : - getActionsForAddMissingMemberInTypeScriptFile(context, classDeclarationSourceFile, classDeclaration, token, makeStatic); + singleElementArray(getActionsForAddMissingMemberInJavaScriptFile(context, classDeclarationSourceFile, parentDeclaration, token.text, makeStatic)) : + getActionsForAddMissingMemberInTypeScriptFile(context, classDeclarationSourceFile, parentDeclaration, token, makeStatic); return concatenate(singleElementArray(methodCodeAction), addMember); }, fixIds: [fixId], getAllCodeActions: context => { - const seenNames = createMap(); - return codeFixAll(context, errorCodes, (changes, diag) => { - const { program, preferences } = context; - const checker = program.getTypeChecker(); - const info = getInfo(diag.file, diag.start, checker); - if (!info || !addToSeen(seenNames, info.token.text)) { - return; - } - - if (info.kind === InfoKind.enum) { - const { token, enumDeclaration } = info; - addEnumMemberDeclaration(changes, checker, token, enumDeclaration); - } - else { - const { classDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info; - // Always prefer to add a method declaration if possible. - if (call) { - addMethodDeclaration(context, changes, classDeclarationSourceFile, classDeclaration, token, call, makeStatic, inJs, preferences); + const { program, preferences } = context; + const checker = program.getTypeChecker(); + const seen = createMap(); + + const classToMembers = new NodeMap(); + + return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => { + eachDiagnostic(context, errorCodes, diag => { + const checker = program.getTypeChecker(); + const info = getInfo(diag.file, diag.start, checker); + if (!info || !addToSeen(seen, getNodeId(info.parentDeclaration) + "#" + info.token.text)) { + return; + } + + if (info.kind === InfoKind.enum) { + const { token, parentDeclaration } = info; + addEnumMemberDeclaration(changes, checker, token, parentDeclaration); } else { - if (inJs) { - addMissingMemberInJs(changes, classDeclarationSourceFile, classDeclaration, token.text, makeStatic); + const { parentDeclaration, token } = info; + const infos = classToMembers.getOrUpdate(parentDeclaration, () => []); + if (!infos.some(i => i.token.text === token.text)) infos.push(info); + } + }); + + classToMembers.forEach((infos, classDeclaration) => { + const superClasses = getAllSuperClasses(classDeclaration, checker); + for (const info of infos) { + // If some superclass added this property, don't add it again. + if (superClasses.some(superClass => { + const superInfos = classToMembers.get(superClass); + return !!superInfos && superInfos.some(({ token }) => token.text === info.token.text); + })) continue; + + const { parentDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info; + + // Always prefer to add a method declaration if possible. + if (call) { + addMethodDeclaration(context, changes, classDeclarationSourceFile, parentDeclaration, token, call, makeStatic, inJs, preferences); } else { - const typeNode = getTypeNode(program.getTypeChecker(), classDeclaration, token); - addPropertyDeclaration(changes, classDeclarationSourceFile, classDeclaration, token.text, typeNode, makeStatic); + if (inJs) { + addMissingMemberInJs(changes, classDeclarationSourceFile, parentDeclaration, token.text, makeStatic); + } + else { + const typeNode = getTypeNode(program.getTypeChecker(), parentDeclaration, token); + addPropertyDeclaration(changes, classDeclarationSourceFile, parentDeclaration, token.text, typeNode, makeStatic); + } } } - } - }); + }); + })); }, }); + function getAllSuperClasses(cls: ClassLikeDeclaration | undefined, checker: TypeChecker): ReadonlyArray { + const res: ClassLikeDeclaration[] = []; + while (cls) { + const superElement = getClassExtendsHeritageElement(cls); + const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression); + const superDecl = superSymbol && find(superSymbol.declarations, isClassLike); + if (superDecl) { res.push(superDecl); } + cls = superDecl; + } + return res; + } + + interface InfoBase { + readonly kind: InfoKind; + readonly token: Identifier; + readonly parentDeclaration: EnumDeclaration | ClassLikeDeclaration; + } enum InfoKind { enum, class } - interface EnumInfo { kind: InfoKind.enum; token: Identifier; enumDeclaration: EnumDeclaration; } - interface ClassInfo { kind: InfoKind.class; token: Identifier; classDeclaration: ClassLikeDeclaration; makeStatic: boolean; classDeclarationSourceFile: SourceFile; inJs: boolean; call: CallExpression | undefined; } + interface EnumInfo extends InfoBase { + readonly kind: InfoKind.enum; + readonly parentDeclaration: EnumDeclaration; + } + interface ClassInfo extends InfoBase { + readonly kind: InfoKind.class; + readonly parentDeclaration: ClassLikeDeclaration; + readonly makeStatic: boolean; + readonly classDeclarationSourceFile: SourceFile; + readonly inJs: boolean; + readonly call: CallExpression | undefined; + } type Info = EnumInfo | ClassInfo; function getInfo(tokenSourceFile: SourceFile, tokenPos: number, checker: TypeChecker): Info | undefined { @@ -86,11 +135,11 @@ namespace ts.codefix { const classDeclarationSourceFile = classDeclaration.getSourceFile(); const inJs = isSourceFileJavaScript(classDeclarationSourceFile); const call = tryCast(parent.parent, isCallExpression); - return { kind: InfoKind.class, token, classDeclaration, makeStatic, classDeclarationSourceFile, inJs, call }; + return { kind: InfoKind.class, token, parentDeclaration: classDeclaration, makeStatic, classDeclarationSourceFile, inJs, call }; } const enumDeclaration = find(symbol.declarations, isEnumDeclaration); if (enumDeclaration) { - return { kind: InfoKind.enum, token, enumDeclaration }; + return { kind: InfoKind.enum, token, parentDeclaration: enumDeclaration }; } return undefined; } diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 789882a491c24..a8c79ee8620e9 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1351,6 +1351,40 @@ namespace ts { } } + export interface ReadonlyNodeMap { + get(node: TNode): TValue | undefined; + has(node: TNode): boolean; + } + + export class NodeMap implements ReadonlyNodeMap { + private map = createMap<{ node: TNode, value: TValue }>(); + + get(node: TNode): TValue | undefined { + const res = this.map.get(String(getNodeId(node))); + return res && res.value; + } + + getOrUpdate(node: TNode, setValue: () => TValue): TValue { + const res = this.get(node); + if (res) return res; + const value = setValue(); + this.set(node, value); + return value; + } + + set(node: TNode, value: TValue): void { + this.map.set(String(getNodeId(node)), { node, value }); + } + + has(node: TNode): boolean { + return this.map.has(String(getNodeId(node))); + } + + forEach(cb: (value: TValue, node: TNode) => void): void { + this.map.forEach(({ node, value }) => cb(value, node)); + } + } + export function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined { if (!node) return undefined; diff --git a/tests/cases/fourslash/codeFixAddMissingMember_all.ts b/tests/cases/fourslash/codeFixAddMissingMember_all.ts index 021968a8c6a92..9eec7700a395e 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember_all.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember_all.ts @@ -7,6 +7,20 @@ //// this.x = ""; //// } ////} +//// +////class D extends C {} +////class E extends D { +//// method() { +//// this.x = 0; +//// this.ex = 0; +//// } +////} +//// +////class Unrelated { +//// method() { +//// this.x = 0; +//// } +////} verify.codeFixAll({ fixId: "addMissingMember", @@ -22,5 +36,21 @@ verify.codeFixAll({ y(): any { throw new Error("Method not implemented."); } +} + +class D extends C {} +class E extends D { + ex: number; + method() { + this.x = 0; + this.ex = 0; + } +} + +class Unrelated { + x: number; + method() { + this.x = 0; + } }`, }); From e0109ea93ed84a34a09289c6a375bc9cdff92dd9 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 2 Jul 2018 13:26:31 -0700 Subject: [PATCH 2/3] Remove shadowed variable --- src/services/codefixes/fixAddMissingMember.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index 8d820cbd3a2f9..5815c9e7bf36c 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -34,7 +34,6 @@ namespace ts.codefix { return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => { eachDiagnostic(context, errorCodes, diag => { - const checker = program.getTypeChecker(); const info = getInfo(diag.file, diag.start, checker); if (!info || !addToSeen(seen, getNodeId(info.parentDeclaration) + "#" + info.token.text)) { return; From 911658bbd29f6a8032eb4517e4ee654e46e44e6d Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 2 Jul 2018 13:29:15 -0700 Subject: [PATCH 3/3] Update API (#24966) --- tests/baselines/reference/api/tsserverlibrary.d.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index a58523852cb28..cd624ddb42e3d 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -10832,6 +10832,18 @@ declare namespace ts { forEach(cb: (node: Node) => void): void; some(pred: (node: Node) => boolean): boolean; } + interface ReadonlyNodeMap { + get(node: TNode): TValue | undefined; + has(node: TNode): boolean; + } + class NodeMap implements ReadonlyNodeMap { + private map; + get(node: TNode): TValue | undefined; + getOrUpdate(node: TNode, setValue: () => TValue): TValue; + set(node: TNode, value: TValue): void; + has(node: TNode): boolean; + forEach(cb: (value: TValue, node: TNode) => void): void; + } function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined; function findModifier(node: Node, kind: Modifier["kind"]): Modifier | undefined; function insertImport(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDecl: Statement): void; @@ -11592,8 +11604,10 @@ declare namespace ts { function getSupportedErrorCodes(): string[]; function getFixes(context: CodeFixContext): CodeFixAction[]; function getAllFixes(context: CodeFixAllContext): CombinedCodeActions; + function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions; function createFileTextChanges(fileName: string, textChanges: TextChange[]): FileTextChanges; function codeFixAll(context: CodeFixAllContext, errorCodes: number[], use: (changes: textChanges.ChangeTracker, error: DiagnosticWithLocation, commands: Push) => void): CombinedCodeActions; + function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void; } } declare namespace ts {