Refactor params for interface (#42652)

* apply refactor to interface methods

* handle calls

* no available refactors for union type

* add tests

* Update src/services/refactors/convertParamsToDestructuredObject.ts

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

* address comments

* Update src/services/refactors/convertParamsToDestructuredObject.ts

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

* Update src/services/refactors/convertParamsToDestructuredObject.ts

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

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(<ObjectLiteralElementLike>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<ParameterDeclaration> {
+    function createNewParameters(functionDeclaration: ValidFunctionDeclaration | ValidMethodSignature, program: Program, host: LanguageServiceHost): NodeArray<ParameterDeclaration> {
         const checker = program.getTypeChecker();
         const refactorableParameters = getRefactorableParameters(functionDeclaration.parameters);
         const bindingElements = map(refactorableParameters, createBindingElementFromParameterDeclaration);
@@ -584,6 +646,10 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         parameters: NodeArray<ValidParameterDeclaration>;
     }
 
+    interface ValidMethodSignature extends MethodSignature {
+        parameters: NodeArray<ValidParameterDeclaration>;
+    }
+
     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;
     }

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interface.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+////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 {},
+};`
+});

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceAssignability.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+////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 {},
+};`
+});

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceContextualParams.ts

@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+////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 {},
+};`
+});

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceMultipleSignatures.ts

@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+////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");

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoIntersection.ts

@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+////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");

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_interfaceNoUnion.ts

@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+////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");

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_typeLiteral.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+////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 {},
+};`
+});