From 10b90516242dc3522140851ad357c60a75c44aed Mon Sep 17 00:00:00 2001 From: Gabriela Britto Date: Mon, 18 Mar 2019 10:31:38 -0700 Subject: [PATCH] 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 | 2 + src/compiler/types.ts | 2 + .../convertParamsToDestructuredObject.ts | 76 ++++++++++++------- ...ParamsToDestructuredObject_badRestParam.ts | 20 +++++ ...ramsToDestructuredObject_tupleRestParam.ts | 57 ++++++++++++++ 5 files changed, 131 insertions(+), 26 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts create mode 100644 tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 8ef0786629914..664a84ec99a97 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -305,6 +305,8 @@ namespace ts { getNeverType: () => neverType, isSymbolAccessible, getObjectFlags, + isArrayType, + isTupleType, isArrayLikeType, isTypeInvalidDueToUnionDiscriminant, getAllPossiblePropertiesOfTypes, diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 590bd2401a5af..088ae34ee34bf 100644 --- a/src/compiler/types.ts +++ b/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; diff --git a/src/services/refactors/convertParamsToDestructuredObject.ts b/src/services/refactors/convertParamsToDestructuredObject.ts index f813f8ef7686b..91836f298217b 100644 --- a/src/services/refactors/convertParamsToDestructuredObject.ts +++ b/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): parameters is ValidParameterNodeArray { - return getRefactorableParametersLength(parameters) >= minimumParameterLength && every(parameters, isValidParameterDeclaration); + function isValidParameterNodeArray( + parameters: NodeArray, + 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 { + 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): 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) { diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts new file mode 100644 index 0000000000000..187086bcec8fa --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts @@ -0,0 +1,20 @@ +/// + +// @Filename: a.ts +////function /*a*/gen/*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"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts new file mode 100644 index 0000000000000..7584308fec004 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts @@ -0,0 +1,57 @@ +/// + +// @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"] });` +}); \ No newline at end of file