From 683b3ec058142c1b2af87775d4e99253c0884532 Mon Sep 17 00:00:00 2001 From: Alexander T Date: Thu, 7 May 2020 01:10:02 +0300 Subject: [PATCH] feat(37782): 'declare method' quick fix for adding a private method (#37806) * feat(37782): add quick-fix action to declare a private method for names that start from underscore * better merge order in messages json Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- src/compiler/diagnosticMessages.json | 4 + src/services/codefixes/fixAddMissingMember.ts | 134 +++++++++--------- src/services/codefixes/helpers.ts | 8 +- ...AddMissingMember18_declarePrivateMethod.ts | 29 ++++ ...AddMissingMember19_declarePrivateMethod.ts | 29 ++++ .../fourslash/codeFixUndeclaredMethod.ts | 32 ++--- .../codeFixUndeclaredMethodFunctionArgs.ts | 7 +- ...odeFixUndeclaredMethodObjectLiteralArgs.ts | 7 +- 8 files changed, 160 insertions(+), 90 deletions(-) create mode 100644 tests/cases/fourslash/codeFixAddMissingMember18_declarePrivateMethod.ts create mode 100644 tests/cases/fourslash/codeFixAddMissingMember19_declarePrivateMethod.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 47f9b79205..78c047a5e4 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -5205,6 +5205,10 @@ "category": "Message", "code": 90037 }, + "Declare private method '{0}'": { + "category": "Message", + "code": 90038 + }, "Declare a private field named '{0}'.": { "category": "Message", "code": 90053 diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index ad7b115f57..27c7330669 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -13,19 +13,15 @@ namespace ts.codefix { errorCodes, getCodeActions(context) { const info = getInfo(context.sourceFile, context.span.start, context.program.getTypeChecker(), context.program); - if (!info) return undefined; - + if (!info) { + return undefined; + } if (info.kind === InfoKind.Enum) { const { token, parentDeclaration } = info; const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, parentDeclaration)); return [createCodeFixAction(fixName, changes, [Diagnostics.Add_missing_enum_member_0, token.text], fixId, Diagnostics.Add_all_missing_members)]; } - const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; - const methodCodeAction = call && getActionForMethodDeclaration(context, declSourceFile, parentDeclaration, token, call, makeStatic, inJs); - const addMember = inJs && !isInterfaceDeclaration(parentDeclaration) ? - singleElementArray(getActionsForAddMissingMemberInJavascriptFile(context, declSourceFile, parentDeclaration, token, makeStatic)) : - getActionsForAddMissingMemberInTypeScriptFile(context, declSourceFile, parentDeclaration, token, makeStatic); - return concatenate(singleElementArray(methodCodeAction), addMember); + return concatenate(getActionsForMissingMethodDeclaration(context, info), getActionsForMissingMemberDeclaration(context, info)); }, fixIds: [fixId], getAllCodeActions: context => { @@ -62,19 +58,18 @@ namespace ts.codefix { return !!superInfos && superInfos.some(({ token }) => token.text === info.token.text); })) continue; - const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; - + const { parentDeclaration, declSourceFile, modifierFlags, token, call, isJSFile } = info; // Always prefer to add a method declaration if possible. if (call && !isPrivateIdentifier(token)) { - addMethodDeclaration(context, changes, declSourceFile, parentDeclaration, token, call, makeStatic, inJs); + addMethodDeclaration(context, changes, call, token.text, modifierFlags & ModifierFlags.Static, parentDeclaration, declSourceFile, isJSFile); } else { - if (inJs && !isInterfaceDeclaration(parentDeclaration)) { - addMissingMemberInJs(changes, declSourceFile, parentDeclaration, token, makeStatic); + if (isJSFile && !isInterfaceDeclaration(parentDeclaration)) { + addMissingMemberInJs(changes, declSourceFile, parentDeclaration, token, !!(modifierFlags & ModifierFlags.Static)); } else { const typeNode = getTypeNode(program.getTypeChecker(), parentDeclaration, token); - addPropertyDeclaration(changes, declSourceFile, parentDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0); + addPropertyDeclaration(changes, declSourceFile, parentDeclaration, token.text, typeNode, modifierFlags & ModifierFlags.Static); } } } @@ -104,12 +99,12 @@ namespace ts.codefix { } interface ClassOrInterfaceInfo { readonly kind: InfoKind.ClassOrInterface; - readonly token: Identifier | PrivateIdentifier; - readonly parentDeclaration: ClassOrInterface; - readonly makeStatic: boolean; - readonly declSourceFile: SourceFile; - readonly inJs: boolean; readonly call: CallExpression | undefined; + readonly token: Identifier | PrivateIdentifier; + readonly modifierFlags: ModifierFlags; + readonly parentDeclaration: ClassOrInterface; + readonly declSourceFile: SourceFile; + readonly isJSFile: boolean; } type Info = EnumInfo | ClassOrInterfaceInfo; @@ -144,9 +139,10 @@ namespace ts.codefix { } const declSourceFile = classOrInterface.getSourceFile(); - const inJs = isSourceFileJS(declSourceFile); + const modifierFlags = (makeStatic ? ModifierFlags.Static : 0) | (startsWithUnderscore(token.text) ? ModifierFlags.Private : 0); + const isJSFile = isSourceFileJS(declSourceFile); const call = tryCast(parent.parent, isCallExpression); - return { kind: InfoKind.ClassOrInterface, token, parentDeclaration: classOrInterface, makeStatic, declSourceFile, inJs, call }; + return { kind: InfoKind.ClassOrInterface, token, call, modifierFlags, parentDeclaration: classOrInterface, declSourceFile, isJSFile }; } const enumDeclaration = find(symbol.declarations, isEnumDeclaration); @@ -156,13 +152,22 @@ namespace ts.codefix { return undefined; } - function getActionsForAddMissingMemberInJavascriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction | undefined { - const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, declSourceFile, classDeclaration, token, makeStatic)); + function getActionsForMissingMemberDeclaration(context: CodeFixContext, info: ClassOrInterfaceInfo): CodeFixAction[] | undefined { + return info.isJSFile ? singleElementArray(createActionForAddMissingMemberInJavascriptFile(context, info)) : + createActionsForAddMissingMemberInTypeScriptFile(context, info); + } + + function createActionForAddMissingMemberInJavascriptFile(context: CodeFixContext, { parentDeclaration, declSourceFile, modifierFlags, token }: ClassOrInterfaceInfo): CodeFixAction | undefined { + if (isInterfaceDeclaration(parentDeclaration)) { + return undefined; + } + + const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, declSourceFile, parentDeclaration, token, !!(modifierFlags & ModifierFlags.Static))); if (changes.length === 0) { return undefined; } - const diagnostic = makeStatic ? Diagnostics.Initialize_static_property_0 : + const diagnostic = modifierFlags & ModifierFlags.Static ? Diagnostics.Initialize_static_property_0 : isPrivateIdentifier(token) ? Diagnostics.Declare_a_private_field_named_0 : Diagnostics.Initialize_property_0_in_the_constructor; return createCodeFixAction(fixName, changes, [diagnostic, token.text], fixId, Diagnostics.Add_all_missing_members); @@ -209,18 +214,22 @@ namespace ts.codefix { return createStatement(createAssignment(createPropertyAccess(obj, propertyName), createIdentifier("undefined"))); } - function getActionsForAddMissingMemberInTypeScriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction[] | undefined { - const typeNode = getTypeNode(context.program.getTypeChecker(), classDeclaration, token); - const actions: CodeFixAction[] = [createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0)]; - if (makeStatic || isPrivateIdentifier(token)) { + function createActionsForAddMissingMemberInTypeScriptFile(context: CodeFixContext, { parentDeclaration, declSourceFile, modifierFlags, token }: ClassOrInterfaceInfo): CodeFixAction[] | undefined { + const memberName = token.text; + const isStatic = modifierFlags & ModifierFlags.Static; + const typeNode = getTypeNode(context.program.getTypeChecker(), parentDeclaration, token); + const addPropertyDeclarationChanges = (modifierFlags: ModifierFlags) => textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, declSourceFile, parentDeclaration, memberName, typeNode, modifierFlags)); + + const actions = [createCodeFixAction(fixName, addPropertyDeclarationChanges(modifierFlags & ModifierFlags.Static), [isStatic ? Diagnostics.Declare_static_property_0 : Diagnostics.Declare_property_0, memberName], fixId, Diagnostics.Add_all_missing_members)]; + if (isStatic || isPrivateIdentifier(token)) { return actions; } - if (startsWithUnderscore(token.text)) { - actions.unshift(createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, ModifierFlags.Private)); + if (modifierFlags & ModifierFlags.Private) { + actions.unshift(createCodeFixActionWithoutFixAll(fixName, addPropertyDeclarationChanges(ModifierFlags.Private), [Diagnostics.Declare_private_property_0, memberName])); } - actions.push(createAddIndexSignatureAction(context, declSourceFile, classDeclaration, token.text, typeNode)); + actions.push(createAddIndexSignatureAction(context, declSourceFile, parentDeclaration, token.text, typeNode)); return actions; } @@ -239,14 +248,6 @@ namespace ts.codefix { return typeNode || createKeywordTypeNode(SyntaxKind.AnyKeyword); } - function createAddPropertyDeclarationAction(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): CodeFixAction { - const changes = textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, declSourceFile, classDeclaration, tokenName, typeNode, modifierFlags)); - if (modifierFlags & ModifierFlags.Private) { - return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Declare_private_property_0, tokenName]); - } - return createCodeFixAction(fixName, changes, [modifierFlags & ModifierFlags.Static ? Diagnostics.Declare_static_property_0 : Diagnostics.Declare_property_0, tokenName], fixId, Diagnostics.Add_all_missing_members); - } - function addPropertyDeclaration(changeTracker: textChanges.ChangeTracker, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): void { const property = createProperty( /*decorators*/ undefined, @@ -297,41 +298,46 @@ namespace ts.codefix { return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]); } - function getActionForMethodDeclaration( - context: CodeFixContext, - declSourceFile: SourceFile, - classDeclaration: ClassOrInterface, - token: Identifier | PrivateIdentifier, - callExpression: CallExpression, - makeStatic: boolean, - inJs: boolean - ): CodeFixAction | undefined { + function getActionsForMissingMethodDeclaration(context: CodeFixContext, info: ClassOrInterfaceInfo): CodeFixAction[] | undefined { + const { parentDeclaration, declSourceFile, modifierFlags, token, call, isJSFile } = info; + if (call === undefined) { + return undefined; + } + // Private methods are not implemented yet. - if (isPrivateIdentifier(token)) { return undefined; } - const changes = textChanges.ChangeTracker.with(context, t => addMethodDeclaration(context, t, declSourceFile, classDeclaration, token, callExpression, makeStatic, inJs)); - return createCodeFixAction(fixName, changes, [makeStatic ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0, token.text], fixId, Diagnostics.Add_all_missing_members); + if (isPrivateIdentifier(token)) { + return undefined; + } + + const methodName = token.text; + const addMethodDeclarationChanges = (modifierFlags: ModifierFlags) => textChanges.ChangeTracker.with(context, t => addMethodDeclaration(context, t, call, methodName, modifierFlags, parentDeclaration, declSourceFile, isJSFile)); + const actions = [createCodeFixAction(fixName, addMethodDeclarationChanges(modifierFlags & ModifierFlags.Static), [modifierFlags & ModifierFlags.Static ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0, methodName], fixId, Diagnostics.Add_all_missing_members)]; + if (modifierFlags & ModifierFlags.Private) { + actions.unshift(createCodeFixActionWithoutFixAll(fixName, addMethodDeclarationChanges(ModifierFlags.Private), [Diagnostics.Declare_private_method_0, methodName])); + } + return actions; } function addMethodDeclaration( context: CodeFixContextBase, - changeTracker: textChanges.ChangeTracker, - declSourceFile: SourceFile, - typeDecl: ClassOrInterface, - token: Identifier, + changes: textChanges.ChangeTracker, callExpression: CallExpression, - makeStatic: boolean, - inJs: boolean + methodName: string, + modifierFlags: ModifierFlags, + parentDeclaration: ClassOrInterface, + sourceFile: SourceFile, + isJSFile: boolean ): void { - const importAdder = createImportAdder(declSourceFile, context.program, context.preferences, context.host); - const methodDeclaration = createMethodFromCallExpression(context, callExpression, token.text, inJs, makeStatic, typeDecl, importAdder); - const containingMethodDeclaration = getAncestor(callExpression, SyntaxKind.MethodDeclaration); - if (containingMethodDeclaration && containingMethodDeclaration.parent === typeDecl) { - changeTracker.insertNodeAfter(declSourceFile, containingMethodDeclaration, methodDeclaration); + const importAdder = createImportAdder(sourceFile, context.program, context.preferences, context.host); + const methodDeclaration = createMethodFromCallExpression(context, importAdder, callExpression, methodName, modifierFlags, parentDeclaration, isJSFile); + const containingMethodDeclaration = findAncestor(callExpression, n => isMethodDeclaration(n) || isConstructorDeclaration(n)); + if (containingMethodDeclaration && containingMethodDeclaration.parent === parentDeclaration) { + changes.insertNodeAfter(sourceFile, containingMethodDeclaration, methodDeclaration); } else { - changeTracker.insertNodeAtClassStart(declSourceFile, typeDecl, methodDeclaration); + changes.insertNodeAtClassStart(sourceFile, parentDeclaration, methodDeclaration); } - importAdder.writeFixes(changeTracker); + importAdder.writeFixes(changes); } function addEnumMemberDeclaration(changes: textChanges.ChangeTracker, checker: TypeChecker, token: Identifier, enumDeclaration: EnumDeclaration) { diff --git a/src/services/codefixes/helpers.ts b/src/services/codefixes/helpers.ts index 51fe5b8fad..4e9c3a9c5a 100644 --- a/src/services/codefixes/helpers.ts +++ b/src/services/codefixes/helpers.ts @@ -214,12 +214,12 @@ namespace ts.codefix { export function createMethodFromCallExpression( context: CodeFixContextBase, + importAdder: ImportAdder, call: CallExpression, methodName: string, - inJs: boolean, - makeStatic: boolean, + modifierFlags: ModifierFlags, contextNode: Node, - importAdder: ImportAdder + inJs: boolean ): MethodDeclaration { const body = !isInterfaceDeclaration(contextNode); const { typeArguments, arguments: args, parent } = call; @@ -234,7 +234,7 @@ namespace ts.codefix { const returnType = (inJs || !contextualType) ? undefined : checker.typeToTypeNode(contextualType, contextNode, /*flags*/ undefined, tracker); return createMethod( /*decorators*/ undefined, - /*modifiers*/ makeStatic ? [createToken(SyntaxKind.StaticKeyword)] : undefined, + /*modifiers*/ modifierFlags ? createNodeArray(createModifiersFromModifierFlags(modifierFlags)) : undefined, /*asteriskToken*/ isYieldExpression(parent) ? createToken(SyntaxKind.AsteriskToken) : undefined, methodName, /*questionToken*/ undefined, diff --git a/tests/cases/fourslash/codeFixAddMissingMember18_declarePrivateMethod.ts b/tests/cases/fourslash/codeFixAddMissingMember18_declarePrivateMethod.ts new file mode 100644 index 0000000000..d0fa87d6a0 --- /dev/null +++ b/tests/cases/fourslash/codeFixAddMissingMember18_declarePrivateMethod.ts @@ -0,0 +1,29 @@ +/// + +////class A { +//// constructor() { +//// this._foo(); +//// } +////} + +verify.codeFixAvailable([ + { description: "Declare private method '_foo'" }, + { description: "Declare method '_foo'" }, + { description: "Declare private property '_foo'" }, + { description: "Declare property '_foo'" }, + { description: "Add index signature for property '_foo'" } +]) + +verify.codeFix({ + description: [ts.Diagnostics.Declare_private_method_0.message, "_foo"], + index: 0, + newFileContent: +`class A { + constructor() { + this._foo(); + } + private _foo() { + throw new Error("Method not implemented."); + } +}` +}); diff --git a/tests/cases/fourslash/codeFixAddMissingMember19_declarePrivateMethod.ts b/tests/cases/fourslash/codeFixAddMissingMember19_declarePrivateMethod.ts new file mode 100644 index 0000000000..72f4e27b5f --- /dev/null +++ b/tests/cases/fourslash/codeFixAddMissingMember19_declarePrivateMethod.ts @@ -0,0 +1,29 @@ +/// + +////class A { +//// foo() { +//// this._bar(); +//// } +////} + +verify.codeFixAvailable([ + { description: "Declare private method '_bar'" }, + { description: "Declare method '_bar'" }, + { description: "Declare private property '_bar'" }, + { description: "Declare property '_bar'" }, + { description: "Add index signature for property '_bar'" } +]) + +verify.codeFix({ + description: [ts.Diagnostics.Declare_private_method_0.message, "_bar"], + index: 0, + newFileContent: +`class A { + foo() { + this._bar(); + } + private _bar() { + throw new Error("Method not implemented."); + } +}` +}); diff --git a/tests/cases/fourslash/codeFixUndeclaredMethod.ts b/tests/cases/fourslash/codeFixUndeclaredMethod.ts index 32713e5dd1..ac7a8b32b2 100644 --- a/tests/cases/fourslash/codeFixUndeclaredMethod.ts +++ b/tests/cases/fourslash/codeFixUndeclaredMethod.ts @@ -15,9 +15,6 @@ verify.codeFix({ index: 0, newFileContent: `class A { - foo1(arg0: number, arg1: number, arg2: number) { - throw new Error("Method not implemented."); - } constructor() { this.foo1(1,2,3); // 7 type args @@ -25,6 +22,9 @@ verify.codeFix({ // 8 type args this.foo3<1,2,3,4,5,6,7,8>(); } + foo1(arg0: number, arg1: number, arg2: number) { + throw new Error("Method not implemented."); + } }`, applyChanges: true, }); @@ -34,12 +34,6 @@ verify.codeFix({ index: 0, newFileContent: `class A { - foo2() { - throw new Error("Method not implemented."); - } - foo1(arg0: number, arg1: number, arg2: number) { - throw new Error("Method not implemented."); - } constructor() { this.foo1(1,2,3); // 7 type args @@ -47,6 +41,12 @@ verify.codeFix({ // 8 type args this.foo3<1,2,3,4,5,6,7,8>(); } + foo2() { + throw new Error("Method not implemented."); + } + foo1(arg0: number, arg1: number, arg2: number) { + throw new Error("Method not implemented."); + } }`, applyChanges: true, }); @@ -56,6 +56,13 @@ verify.codeFix({ index: 0, newFileContent: `class A { + constructor() { + this.foo1(1,2,3); + // 7 type args + this.foo2<1,2,3,4,5,6,7>(); + // 8 type args + this.foo3<1,2,3,4,5,6,7,8>(); + } foo3() { throw new Error("Method not implemented."); } @@ -65,12 +72,5 @@ verify.codeFix({ foo1(arg0: number, arg1: number, arg2: number) { throw new Error("Method not implemented."); } - constructor() { - this.foo1(1,2,3); - // 7 type args - this.foo2<1,2,3,4,5,6,7>(); - // 8 type args - this.foo3<1,2,3,4,5,6,7,8>(); - } }` }); diff --git a/tests/cases/fourslash/codeFixUndeclaredMethodFunctionArgs.ts b/tests/cases/fourslash/codeFixUndeclaredMethodFunctionArgs.ts index c4056625b7..faa5a165ac 100644 --- a/tests/cases/fourslash/codeFixUndeclaredMethodFunctionArgs.ts +++ b/tests/cases/fourslash/codeFixUndeclaredMethodFunctionArgs.ts @@ -1,10 +1,11 @@ /// -//// class A {[| -//// |]constructor() { +//// class A { +//// constructor() { //// this.foo1(() => 1, () => "2", () => false); //// this.foo2((a: number) => a, (b: string) => b, (c: boolean) => c); -//// } +//// }[| +//// |] //// } verify.codeFix({ diff --git a/tests/cases/fourslash/codeFixUndeclaredMethodObjectLiteralArgs.ts b/tests/cases/fourslash/codeFixUndeclaredMethodObjectLiteralArgs.ts index 5c0698ad65..9d339c50fa 100644 --- a/tests/cases/fourslash/codeFixUndeclaredMethodObjectLiteralArgs.ts +++ b/tests/cases/fourslash/codeFixUndeclaredMethodObjectLiteralArgs.ts @@ -1,11 +1,12 @@ /// -//// class A {[| -//// |]constructor() { +//// class A { +//// constructor() { //// this.foo1(null, {}, { a: 1, b: "2"}); //// const bar = this.foo2(null, {}, { a: 1, b: "2"}); //// const baz: number = this.foo3(null, {}, { a: 1, b: "2"}); -//// } +//// }[| +//// |] //// } verify.codeFix({