Fix convert to named parameters rest parameter tuple (#30286)

* check if rest parameter is of tuple type in isOptionalParameter

* expose isArrayType and isTupleType in checker

* don't offer refactor if rest parameter type is neither array nor tuple type

* add tests for rest parameters

* fix tests for renamed refactor

* remove unnecessary conditional operator

src/compiler/checker.ts

@@ -305,6 +305,8 @@ namespace ts {
             getNeverType: () => neverType,
             isSymbolAccessible,
             getObjectFlags,
+            isArrayType,
+            isTupleType,
             isArrayLikeType,
             isTypeInvalidDueToUnionDiscriminant,
             getAllPossiblePropertiesOfTypes,

src/compiler/types.ts

@@ -3252,6 +3252,8 @@ namespace ts {
         /* @internal */ getSymbolCount(): number;
         /* @internal */ getTypeCount(): number;
 
+        /* @internal */ isArrayType(type: Type): boolean;
+        /* @internal */ isTupleType(type: Type): boolean;
         /* @internal */ isArrayLikeType(type: Type): boolean;
         /* @internal */ getObjectFlags(type: Type): ObjectFlags;
 

src/services/refactors/convertParamsToDestructuredObject.ts

@@ -81,7 +81,7 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         const references = flatMap(names, /*mapfn*/ name => FindAllReferences.getReferenceEntriesForNode(-1, name, program, program.getSourceFiles(), cancellationToken));
         const groupedReferences = groupReferences(references);
 
-        if (!every(groupedReferences.declarations, decl => contains(names, decl))) {
+        if (!every(groupedReferences.declarations, /*callback*/ decl => contains(names, decl))) {
             groupedReferences.valid = false;
         }
 
@@ -241,18 +241,26 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         return undefined;
     }
 
-    function isValidFunctionDeclaration(functionDeclaration: SignatureDeclaration, checker: TypeChecker): functionDeclaration is ValidFunctionDeclaration {
-        if (!isValidParameterNodeArray(functionDeclaration.parameters)) return false;
+    function isValidFunctionDeclaration(
+        functionDeclaration: SignatureDeclaration,
+        checker: TypeChecker): functionDeclaration is ValidFunctionDeclaration {
+        if (!isValidParameterNodeArray(functionDeclaration.parameters, checker)) return false;
         switch (functionDeclaration.kind) {
             case SyntaxKind.FunctionDeclaration:
             case SyntaxKind.MethodDeclaration:
-                return !!functionDeclaration.name && !!functionDeclaration.body && !checker.isImplementationOfOverload(functionDeclaration);
+                return !!functionDeclaration.name
+                    && !!functionDeclaration.body
+                    && !checker.isImplementationOfOverload(functionDeclaration);
             case SyntaxKind.Constructor:
                 if (isClassDeclaration(functionDeclaration.parent)) {
-                    return !!functionDeclaration.body && !!functionDeclaration.parent.name && !checker.isImplementationOfOverload(functionDeclaration);
+                    return !!functionDeclaration.body
+                        && !!functionDeclaration.parent.name
+                        && !checker.isImplementationOfOverload(functionDeclaration);
                 }
                 else {
-                    return isValidVariableDeclaration(functionDeclaration.parent.parent) && !!functionDeclaration.body && !checker.isImplementationOfOverload(functionDeclaration);
+                    return isValidVariableDeclaration(functionDeclaration.parent.parent)
+                        && !!functionDeclaration.body
+                        && !checker.isImplementationOfOverload(functionDeclaration);
                 }
             case SyntaxKind.FunctionExpression:
             case SyntaxKind.ArrowFunction:
@@ -261,12 +269,21 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         return false;
     }
 
-    function isValidParameterNodeArray(parameters: NodeArray<ParameterDeclaration>): parameters is ValidParameterNodeArray {
-        return getRefactorableParametersLength(parameters) >= minimumParameterLength && every(parameters, isValidParameterDeclaration);
+    function isValidParameterNodeArray(
+        parameters: NodeArray<ParameterDeclaration>,
+        checker: TypeChecker): parameters is ValidParameterNodeArray {
+        return getRefactorableParametersLength(parameters) >= minimumParameterLength
+            && every(parameters, /*callback*/ paramDecl => isValidParameterDeclaration(paramDecl, checker));
     }
 
-    function isValidParameterDeclaration(paramDeclaration: ParameterDeclaration): paramDeclaration is ValidParameterDeclaration {
-        return !paramDeclaration.modifiers && !paramDeclaration.decorators && isIdentifier(paramDeclaration.name);
+    function isValidParameterDeclaration(
+        parameterDeclaration: ParameterDeclaration,
+        checker: TypeChecker): parameterDeclaration is ValidParameterDeclaration {
+        if (isRestParameter(parameterDeclaration)) {
+            const type = checker.getTypeAtLocation(parameterDeclaration);
+            if (!checker.isArrayType(type) && !checker.isTupleType(type)) return false;
+        }
+        return !parameterDeclaration.modifiers && !parameterDeclaration.decorators && isIdentifier(parameterDeclaration.name);
     }
 
     function isValidVariableDeclaration(node: Node): node is ValidVariableDeclaration {
@@ -313,15 +330,15 @@ namespace ts.refactor.convertParamsToDestructuredObject {
     }
 
     function createNewParameters(functionDeclaration: ValidFunctionDeclaration, program: Program, host: LanguageServiceHost): NodeArray<ParameterDeclaration> {
+        const checker = program.getTypeChecker();
         const refactorableParameters = getRefactorableParameters(functionDeclaration.parameters);
         const bindingElements = map(refactorableParameters, createBindingElementFromParameterDeclaration);
         const objectParameterName = createObjectBindingPattern(bindingElements);
         const objectParameterType = createParameterTypeNode(refactorableParameters);
-        const checker = program.getTypeChecker();
 
         let objectInitializer: Expression | undefined;
         // If every parameter in the original function was optional, add an empty object initializer to the new object parameter
-        if (every(refactorableParameters, checker.isOptionalParameter)) {
+        if (every(refactorableParameters, isOptionalParameter)) {
             objectInitializer = createObjectLiteral();
         }
 
@@ -355,6 +372,20 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         }
         return createNodeArray([objectParameter]);
 
+        function createBindingElementFromParameterDeclaration(parameterDeclaration: ValidParameterDeclaration): BindingElement {
+            const element = createBindingElement(
+                /*dotDotDotToken*/ undefined,
+                /*propertyName*/ undefined,
+                getParameterName(parameterDeclaration),
+                isRestParameter(parameterDeclaration) && isOptionalParameter(parameterDeclaration) ? createArrayLiteral() : parameterDeclaration.initializer);
+
+            suppressLeadingAndTrailingTrivia(element);
+            if (parameterDeclaration.initializer && element.initializer) {
+                copyComments(parameterDeclaration.initializer, element.initializer);
+            }
+            return element;
+        }
+
         function createParameterTypeNode(parameters: NodeArray<ValidParameterDeclaration>): TypeLiteralNode {
             const members = map(parameters, createPropertySignatureFromParameterDeclaration);
             const typeNode = addEmitFlags(createTypeLiteralNode(members), EmitFlags.SingleLine);
@@ -370,7 +401,7 @@ namespace ts.refactor.convertParamsToDestructuredObject {
             const propertySignature = createPropertySignature(
                 /*modifiers*/ undefined,
                 getParameterName(parameterDeclaration),
-                parameterDeclaration.initializer || isRestParameter(parameterDeclaration) ? createToken(SyntaxKind.QuestionToken) : parameterDeclaration.questionToken,
+                isOptionalParameter(parameterDeclaration) ? createToken(SyntaxKind.QuestionToken) : parameterDeclaration.questionToken,
                 parameterType,
                 /*initializer*/ undefined);
 
@@ -384,24 +415,17 @@ namespace ts.refactor.convertParamsToDestructuredObject {
         }
 
         function getTypeNode(node: Node): TypeNode | undefined {
-            const checker = program.getTypeChecker();
             const type = checker.getTypeAtLocation(node);
             return getTypeNodeIfAccessible(type, node, program, host);
         }
-    }
 
-    function createBindingElementFromParameterDeclaration(parameterDeclaration: ValidParameterDeclaration): BindingElement {
-        const element = createBindingElement(
-            /*dotDotDotToken*/ undefined,
-            /*propertyName*/ undefined,
-            getParameterName(parameterDeclaration),
-            isRestParameter(parameterDeclaration) ? createArrayLiteral() : parameterDeclaration.initializer);
-
-        suppressLeadingAndTrailingTrivia(element);
-        if (parameterDeclaration.initializer && element.initializer) {
-            copyComments(parameterDeclaration.initializer, element.initializer);
+        function isOptionalParameter(parameterDeclaration: ValidParameterDeclaration): boolean {
+            if (isRestParameter(parameterDeclaration)) {
+                const type = checker.getTypeAtLocation(parameterDeclaration);
+                return !checker.isTupleType(type);
+            }
+            return checker.isOptionalParameter(parameterDeclaration);
         }
-        return element;
     }
 
     function copyComments(sourceNode: Node, targetNode: Node) {

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts

@@ -0,0 +1,20 @@
+/// <reference path='fourslash.ts' />
+
+// @Filename: a.ts
+////function /*a*/gen<T extends any[]>/*b*/(a: boolean, ...b: T): T {
+////    return b;
+////}
+////gen(false);
+////gen(false, 1);
+////gen(true, 1, "two");
+
+// @Filename: b.ts
+////function /*c*/un/*d*/(a: boolean, ...b: number[] | [string, string]) {
+////    return b;
+////}
+
+goTo.select("a", "b");
+verify.not.refactorAvailable("Convert to named parameters");
+
+goTo.select("c", "d");
+verify.not.refactorAvailable("Convert to named parameters");

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts

@@ -0,0 +1,57 @@
+/// <reference path='fourslash.ts' />
+
+// @Filename: a.ts
+////function /*a*/fn1/*b*/(a: number, b: number, ...args: [number, number]) { }
+////fn1(1, 2, 3, 4);
+
+// @Filename: b.ts
+////function /*c*/fn2/*d*/(a: number, b: number, ...args: [number, number, ...string[]]) { }
+////fn2(1, 2, 3, 4);
+////fn2(1, 2, 3, 4, "a");
+
+// @Filename: c.ts
+////function /*e*/fn3/*f*/(b: boolean, c: []) { }
+////fn3(true, []);
+
+// @Filename: d.ts
+////function /*g*/fn4/*h*/(a: number, ...args: [...string[]] ) { }
+////fn4(2);
+////fn4(1, "two", "three");
+
+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: `function fn1({ a, b, args }: { a: number; b: number; args: [number, number]; }) { }
+fn1({ a: 1, b: 2, args: [3, 4] });`
+});
+
+goTo.select("c", "d");
+edit.applyRefactor({
+    refactorName: "Convert parameters to destructured object",
+    actionName: "Convert parameters to destructured object",
+    actionDescription: "Convert parameters to destructured object",
+    newContent: `function fn2({ a, b, args }: { a: number; b: number; args: [number, number, ...string[]]; }) { }
+fn2({ a: 1, b: 2, args: [3, 4] });
+fn2({ a: 1, b: 2, args: [3, 4, "a"] });`
+});
+
+goTo.select("e", "f");
+edit.applyRefactor({
+    refactorName: "Convert parameters to destructured object",
+    actionName: "Convert parameters to destructured object",
+    actionDescription: "Convert parameters to destructured object",
+    newContent: `function fn3({ b, c }: { b: boolean; c: []; }) { }
+fn3({ b: true, c: [] });`
+});
+
+goTo.select("g", "h");
+edit.applyRefactor({
+    refactorName: "Convert parameters to destructured object",
+    actionName: "Convert parameters to destructured object",
+    actionDescription: "Convert parameters to destructured object",
+    newContent: `function fn4({ a, args = [] }: { a: number; args?: [...string[]]; }) { }
+fn4({ a: 2 });
+fn4({ a: 1, args: ["two", "three"] });`
+});