From e8e4db725bda2a1e29f3b3a86883641ec4dda847 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 7 Mar 2019 16:52:48 -0800 Subject: [PATCH 1/6] check if rest parameter is of tuple type in isOptionalParameter --- src/compiler/checker.ts | 1 + src/compiler/types.ts | 1 + .../refactors/convertToNamedParameters.ts | 39 +++++++++++-------- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 74545e366fdb8..fde502d87c7cf 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -303,6 +303,7 @@ namespace ts { getNeverType: () => neverType, isSymbolAccessible, getObjectFlags, + isTupleLikeType, isArrayLikeType, isTypeInvalidDueToUnionDiscriminant, getAllPossiblePropertiesOfTypes, diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 42fd1126d4cba..f0125619c40b0 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -3189,6 +3189,7 @@ namespace ts { /* @internal */ getSymbolCount(): number; /* @internal */ getTypeCount(): number; + /* @internal */ isTupleLikeType(type: Type): boolean; /* @internal */ isArrayLikeType(type: Type): boolean; /* @internal */ getObjectFlags(type: Type): ObjectFlags; diff --git a/src/services/refactors/convertToNamedParameters.ts b/src/services/refactors/convertToNamedParameters.ts index 61e9ccf981136..628adeeabaf8f 100644 --- a/src/services/refactors/convertToNamedParameters.ts +++ b/src/services/refactors/convertToNamedParameters.ts @@ -313,15 +313,15 @@ namespace ts.refactor.convertToNamedParameters { } 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 +355,20 @@ namespace ts.refactor.convertToNamedParameters { } 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 +384,7 @@ namespace ts.refactor.convertToNamedParameters { 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 +398,17 @@ namespace ts.refactor.convertToNamedParameters { } 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.isTupleLikeType(type) ? false : true; + } + return checker.isOptionalParameter(parameterDeclaration); } - return element; } function copyComments(sourceNode: Node, targetNode: Node) { From 5754d4f8755aadb35897b25013e34d633923460b Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Fri, 8 Mar 2019 16:41:38 -0800 Subject: [PATCH 2/6] expose isArrayType and isTupleType in checker --- src/compiler/checker.ts | 3 ++- src/compiler/types.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index fde502d87c7cf..b1a21ac13b772 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -303,7 +303,8 @@ namespace ts { getNeverType: () => neverType, isSymbolAccessible, getObjectFlags, - isTupleLikeType, + isArrayType, + isTupleType, isArrayLikeType, isTypeInvalidDueToUnionDiscriminant, getAllPossiblePropertiesOfTypes, diff --git a/src/compiler/types.ts b/src/compiler/types.ts index f0125619c40b0..6a324f3490314 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -3189,7 +3189,8 @@ namespace ts { /* @internal */ getSymbolCount(): number; /* @internal */ getTypeCount(): number; - /* @internal */ isTupleLikeType(type: Type): boolean; + /* @internal */ isArrayType(type: Type): boolean; + /* @internal */ isTupleType(type: Type): boolean; /* @internal */ isArrayLikeType(type: Type): boolean; /* @internal */ getObjectFlags(type: Type): ObjectFlags; From 8055c9da37ca1eb4241baead710f9410f7aeb0f5 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Fri, 8 Mar 2019 16:42:54 -0800 Subject: [PATCH 3/6] don't offer refactor if rest parameter type is neither array nor tuple type --- .../refactors/convertToNamedParameters.ts | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/services/refactors/convertToNamedParameters.ts b/src/services/refactors/convertToNamedParameters.ts index 628adeeabaf8f..5668c177befc4 100644 --- a/src/services/refactors/convertToNamedParameters.ts +++ b/src/services/refactors/convertToNamedParameters.ts @@ -81,7 +81,7 @@ namespace ts.refactor.convertToNamedParameters { 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.convertToNamedParameters { 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.convertToNamedParameters { 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 { @@ -405,7 +422,7 @@ namespace ts.refactor.convertToNamedParameters { function isOptionalParameter(parameterDeclaration: ValidParameterDeclaration): boolean { if (isRestParameter(parameterDeclaration)) { const type = checker.getTypeAtLocation(parameterDeclaration); - return checker.isTupleLikeType(type) ? false : true; + return checker.isTupleType(type) ? false : true; } return checker.isOptionalParameter(parameterDeclaration); } From ae5bcb5abe779080685e19bdbcaa2c020cc56868 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Fri, 8 Mar 2019 16:43:10 -0800 Subject: [PATCH 4/6] add tests for rest parameters --- ...orConvertToNamedParameters_badRestParam.ts | 20 +++++++ ...ConvertToNamedParameters_tupleRestParam.ts | 57 +++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 tests/cases/fourslash/refactorConvertToNamedParameters_badRestParam.ts create mode 100644 tests/cases/fourslash/refactorConvertToNamedParameters_tupleRestParam.ts diff --git a/tests/cases/fourslash/refactorConvertToNamedParameters_badRestParam.ts b/tests/cases/fourslash/refactorConvertToNamedParameters_badRestParam.ts new file mode 100644 index 0000000000000..187086bcec8fa --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToNamedParameters_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/refactorConvertToNamedParameters_tupleRestParam.ts b/tests/cases/fourslash/refactorConvertToNamedParameters_tupleRestParam.ts new file mode 100644 index 0000000000000..ecfc1eaf9a74a --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToNamedParameters_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 to named parameters", + actionName: "Convert to named parameters", + actionDescription: "Convert to named parameters", + 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 to named parameters", + actionName: "Convert to named parameters", + actionDescription: "Convert to named parameters", + 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 to named parameters", + actionName: "Convert to named parameters", + actionDescription: "Convert to named parameters", + newContent: `function fn3({ b, c }: { b: boolean; c: []; }) { } +fn3({ b: true, c: [] });` +}); + +goTo.select("g", "h"); +edit.applyRefactor({ + refactorName: "Convert to named parameters", + actionName: "Convert to named parameters", + actionDescription: "Convert to named parameters", + newContent: `function fn4({ a, args = [] }: { a: number; args?: [...string[]]; }) { } +fn4({ a: 2 }); +fn4({ a: 1, args: ["two", "three"] });` +}); \ No newline at end of file From ecadb8c5967e416b746dbbc4885eded90871edba Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Fri, 15 Mar 2019 14:15:00 -0700 Subject: [PATCH 5/6] fix tests for renamed refactor --- ...aramsToDestructuredObject_badRestParam.ts} | 0 ...amsToDestructuredObject_tupleRestParam.ts} | 24 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) rename tests/cases/fourslash/{refactorConvertToNamedParameters_badRestParam.ts => refactorConvertParamsToDestructuredObject_badRestParam.ts} (100%) rename tests/cases/fourslash/{refactorConvertToNamedParameters_tupleRestParam.ts => refactorConvertParamsToDestructuredObject_tupleRestParam.ts} (61%) diff --git a/tests/cases/fourslash/refactorConvertToNamedParameters_badRestParam.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToNamedParameters_badRestParam.ts rename to tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts diff --git a/tests/cases/fourslash/refactorConvertToNamedParameters_tupleRestParam.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts similarity index 61% rename from tests/cases/fourslash/refactorConvertToNamedParameters_tupleRestParam.ts rename to tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts index ecfc1eaf9a74a..7584308fec004 100644 --- a/tests/cases/fourslash/refactorConvertToNamedParameters_tupleRestParam.ts +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_tupleRestParam.ts @@ -20,18 +20,18 @@ goTo.select("a", "b"); edit.applyRefactor({ - refactorName: "Convert to named parameters", - actionName: "Convert to named parameters", - actionDescription: "Convert to named parameters", + 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 to named parameters", - actionName: "Convert to named parameters", - actionDescription: "Convert to named parameters", + 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"] });` @@ -39,18 +39,18 @@ fn2({ a: 1, b: 2, args: [3, 4, "a"] });` goTo.select("e", "f"); edit.applyRefactor({ - refactorName: "Convert to named parameters", - actionName: "Convert to named parameters", - actionDescription: "Convert to named parameters", + 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 to named parameters", - actionName: "Convert to named parameters", - actionDescription: "Convert to named parameters", + 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"] });` From 77d851af449cc990cf9e30ed4617c148d071b708 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Mon, 18 Mar 2019 09:40:40 -0700 Subject: [PATCH 6/6] remove unnecessary conditional operator --- src/services/refactors/convertParamsToDestructuredObject.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/convertParamsToDestructuredObject.ts b/src/services/refactors/convertParamsToDestructuredObject.ts index 78e7f7a9672eb..91836f298217b 100644 --- a/src/services/refactors/convertParamsToDestructuredObject.ts +++ b/src/services/refactors/convertParamsToDestructuredObject.ts @@ -422,7 +422,7 @@ namespace ts.refactor.convertParamsToDestructuredObject { function isOptionalParameter(parameterDeclaration: ValidParameterDeclaration): boolean { if (isRestParameter(parameterDeclaration)) { const type = checker.getTypeAtLocation(parameterDeclaration); - return checker.isTupleType(type) ? false : true; + return !checker.isTupleType(type); } return checker.isOptionalParameter(parameterDeclaration); }