diff --git a/src/services/refactors/convertParamsToDestructuredObject.ts b/src/services/refactors/convertParamsToDestructuredObject.ts index feef5606b113c..c9e587837ddd9 100644 --- a/src/services/refactors/convertParamsToDestructuredObject.ts +++ b/src/services/refactors/convertParamsToDestructuredObject.ts @@ -51,18 +51,14 @@ namespace ts.refactor.convertParamsToDestructuredObject { changes: textChanges.ChangeTracker, functionDeclaration: ValidFunctionDeclaration, groupedReferences: GroupedReferences): void { - const newParamDeclaration = map(createNewParameters(functionDeclaration, program, host), param => getSynthesizedDeepClone(param)); - changes.replaceNodeRangeWithNodes( - sourceFile, - first(functionDeclaration.parameters), - last(functionDeclaration.parameters), - newParamDeclaration, - { joiner: ", ", - // indentation is set to 0 because otherwise the object parameter will be indented if there is a `this` parameter - indentation: 0, - leadingTriviaOption: textChanges.LeadingTriviaOption.IncludeAll, - trailingTriviaOption: textChanges.TrailingTriviaOption.Include - }); + const signature = groupedReferences.signature; + const newFunctionDeclarationParams = map(createNewParameters(functionDeclaration, program, host), param => getSynthesizedDeepClone(param)); + + if (signature) { + const newSignatureParams = map(createNewParameters(signature, program, host), param => getSynthesizedDeepClone(param)); + replaceParameters(signature, newSignatureParams); + } + replaceParameters(functionDeclaration, newFunctionDeclarationParams); const functionCalls = sortAndDeduplicate(groupedReferences.functionCalls, /*comparer*/ (a, b) => compareValues(a.pos, b.pos)); for (const call of functionCalls) { @@ -76,6 +72,21 @@ namespace ts.refactor.convertParamsToDestructuredObject { { leadingTriviaOption: textChanges.LeadingTriviaOption.IncludeAll, trailingTriviaOption: textChanges.TrailingTriviaOption.Include }); } } + + function replaceParameters(declarationOrSignature: ValidFunctionDeclaration | ValidMethodSignature, parameterDeclarations: ParameterDeclaration[]) { + changes.replaceNodeRangeWithNodes( + sourceFile, + first(declarationOrSignature.parameters), + last(declarationOrSignature.parameters), + parameterDeclarations, + { + joiner: ", ", + // indentation is set to 0 because otherwise the object parameter will be indented if there is a `this` parameter + indentation: 0, + leadingTriviaOption: textChanges.LeadingTriviaOption.IncludeAll, + trailingTriviaOption: textChanges.TrailingTriviaOption.Include + }); + } } function getGroupedReferences(functionDeclaration: ValidFunctionDeclaration, program: Program, cancellationToken: CancellationToken): GroupedReferences { @@ -99,13 +110,41 @@ namespace ts.refactor.convertParamsToDestructuredObject { const functionSymbols = map(functionNames, getSymbolTargetAtLocation); const classSymbols = map(classNames, getSymbolTargetAtLocation); const isConstructor = isConstructorDeclaration(functionDeclaration); + const contextualSymbols = map(functionNames, name => getSymbolForContextualType(name, checker)); for (const entry of referenceEntries) { - if (entry.kind !== FindAllReferences.EntryKind.Node) { + if (entry.kind === FindAllReferences.EntryKind.Span) { groupedReferences.valid = false; continue; } + /* Declarations in object literals may be implementations of method signatures which have a different symbol from the declaration + For example: + interface IFoo { m(a: number): void } + const foo: IFoo = { m(a: number): void {} } + In these cases we get the symbol for the signature from the contextual type. + */ + if (contains(contextualSymbols, getSymbolTargetAtLocation(entry.node))) { + if (isValidMethodSignature(entry.node.parent)) { + groupedReferences.signature = entry.node.parent; + continue; + } + const call = entryToFunctionCall(entry); + if (call) { + groupedReferences.functionCalls.push(call); + continue; + } + } + + const contextualSymbol = getSymbolForContextualType(entry.node, checker); + if (contextualSymbol && contains(contextualSymbols, contextualSymbol)) { + const decl = entryToDeclaration(entry); + if (decl) { + groupedReferences.declarations.push(decl); + continue; + } + } + /* We compare symbols because in some cases find all references wil return a reference that may or may not be to the refactored function. Example from the refactorConvertParamsToDestructuredObject_methodCallUnion.ts test: class A { foo(a: number, b: number) { return a + b; } } @@ -175,6 +214,20 @@ namespace ts.refactor.convertParamsToDestructuredObject { } } + /** + * Gets the symbol for the contextual type of the node if it is not a union or intersection. + */ + function getSymbolForContextualType(node: Node, checker: TypeChecker): Symbol | undefined { + const element = getContainingObjectLiteralElement(node); + if (element) { + const contextualType = checker.getContextualTypeForObjectLiteralElement(element); + const symbol = contextualType?.getSymbol(); + if (symbol && !(getCheckFlags(symbol) & CheckFlags.Synthetic)) { + return symbol; + } + } + } + function entryToImportOrExport(entry: FindAllReferences.NodeEntry): Node | undefined { const node = entry.node; @@ -292,6 +345,10 @@ namespace ts.refactor.convertParamsToDestructuredObject { return false; } + function isValidMethodSignature(node: Node): node is ValidMethodSignature { + return isMethodSignature(node) && (isInterfaceDeclaration(node.parent) || isTypeLiteralNode(node.parent)); + } + function isValidFunctionDeclaration( functionDeclaration: FunctionLikeDeclaration, checker: TypeChecker): functionDeclaration is ValidFunctionDeclaration { @@ -300,6 +357,11 @@ namespace ts.refactor.convertParamsToDestructuredObject { case SyntaxKind.FunctionDeclaration: return hasNameOrDefault(functionDeclaration) && isSingleImplementation(functionDeclaration, checker); case SyntaxKind.MethodDeclaration: + if (isObjectLiteralExpression(functionDeclaration.parent)) { + const contextualSymbol = getSymbolForContextualType(functionDeclaration.name, checker); + // don't offer the refactor when there are multiple signatures since we won't know which ones the user wants to change + return contextualSymbol?.declarations.length === 1 && isSingleImplementation(functionDeclaration, checker); + } return isSingleImplementation(functionDeclaration, checker); case SyntaxKind.Constructor: if (isClassDeclaration(functionDeclaration.parent)) { @@ -398,7 +460,7 @@ namespace ts.refactor.convertParamsToDestructuredObject { return objectLiteral; } - function createNewParameters(functionDeclaration: ValidFunctionDeclaration, program: Program, host: LanguageServiceHost): NodeArray { + function createNewParameters(functionDeclaration: ValidFunctionDeclaration | ValidMethodSignature, program: Program, host: LanguageServiceHost): NodeArray { const checker = program.getTypeChecker(); const refactorableParameters = getRefactorableParameters(functionDeclaration.parameters); const bindingElements = map(refactorableParameters, createBindingElementFromParameterDeclaration); @@ -584,6 +646,10 @@ namespace ts.refactor.convertParamsToDestructuredObject { parameters: NodeArray; } + interface ValidMethodSignature extends MethodSignature { + parameters: NodeArray; + } + type ValidFunctionDeclaration = ValidConstructor | ValidFunction | ValidMethod | ValidArrowFunction | ValidFunctionExpression; interface ValidParameterDeclaration extends ParameterDeclaration { @@ -595,6 +661,7 @@ namespace ts.refactor.convertParamsToDestructuredObject { interface GroupedReferences { functionCalls: (CallExpression | NewExpression)[]; declarations: Node[]; + signature?: ValidMethodSignature; classReferences?: ClassReferences; valid: boolean; } diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interface.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interface.ts new file mode 100644 index 0000000000000..105c6ac56c940 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interface.ts @@ -0,0 +1,21 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +////} +////const x: IFoo = { +//// method(/*a*/x: string, y: string/*b*/): void {}, +////}; + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert parameters to destructured object", + actionName: "Convert parameters to destructured object", + actionDescription: "Convert parameters to destructured object", + newContent: `interface IFoo { + method({ x, y }: { x: string; y: string; }): void; +} +const x: IFoo = { + method({ x, y }: { x: string; y: string; }): void {}, +};` +}); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceAssignability.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceAssignability.ts new file mode 100644 index 0000000000000..d5ab8cab64119 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceAssignability.ts @@ -0,0 +1,21 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +////} +////const x: IFoo = { +//// method(/*a*/x: string, y: string, z?: string/*b*/): void {}, +////}; + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert parameters to destructured object", + actionName: "Convert parameters to destructured object", + actionDescription: "Convert parameters to destructured object", + newContent: `interface IFoo { + method({ x, y }: { x: string; y: string; }): void; +} +const x: IFoo = { + method({ x, y, z }: { x: string; y: string; z?: string; }): void {}, +};` +}); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceContextualParams.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceContextualParams.ts new file mode 100644 index 0000000000000..af00cbc82a97f --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceContextualParams.ts @@ -0,0 +1,28 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +////} +////const x: IFoo = { +//// method(/*a*/x, y/*b*/): void {}, +////}; + +/* +When there are no type annotations on the params in the implementation, we ultimately +would like to handle them like we do for calls resulting in `method({x, y}) {}`. + +Note that simply adding the annotations from the signature can fail as the implementation +can take more paramters than the signatures. +*/ +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert parameters to destructured object", + actionName: "Convert parameters to destructured object", + actionDescription: "Convert parameters to destructured object", + newContent: `interface IFoo { + method({ x, y }: { x: string; y: string; }): void; +} +const x: IFoo = { + method({ x, y }: { x; y; }): void {}, +};` +}); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceMultipleSignatures.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceMultipleSignatures.ts new file mode 100644 index 0000000000000..b21fb473bcdcd --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceMultipleSignatures.ts @@ -0,0 +1,14 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +//// method(x: number, y: string): void; +////} +////const x: IFoo = { +//// method(/*a*/x: string | number, y: string/*b*/): void {}, +////}; + +// For multiple signatures, we don't have a reliable way to determine +// which signature to match to or if all signatures should be changed. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert parameters to destructured object"); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoIntersection.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoIntersection.ts new file mode 100644 index 0000000000000..0a7230f3419f5 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoIntersection.ts @@ -0,0 +1,14 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +////} +////interface IBar { +//// method(x: number): void; +////} +////const x: IFoo & IBar = { +//// method(/*a*/x: string, y: string/*b*/): void {}, +////}; + +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert parameters to destructured object"); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoUnion.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoUnion.ts new file mode 100644 index 0000000000000..ceafa7282e02b --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoUnion.ts @@ -0,0 +1,14 @@ +/// + +////interface IFoo { +//// method(x: string, y: string): void; +////} +////interface IBar { +//// method(x: number): void; +////} +////const x: IFoo | IBar = { +//// method(/*a*/x: string, y: string/*b*/): void {}, +////}; + +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert parameters to destructured object"); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_typeLiteral.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_typeLiteral.ts new file mode 100644 index 0000000000000..79f6951562ddf --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_typeLiteral.ts @@ -0,0 +1,21 @@ +/// + +////type Foo = { +//// method(x: string, y: string): void; +////} +////const x: Foo = { +//// method(/*a*/x: string, y: string/*b*/): void {}, +////}; + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert parameters to destructured object", + actionName: "Convert parameters to destructured object", + actionDescription: "Convert parameters to destructured object", + newContent: `type Foo = { + method({ x, y }: { x: string; y: string; }): void; +} +const x: Foo = { + method({ x, y }: { x: string; y: string; }): void {}, +};` +});