Skip to content

Commit

Permalink
Fix convert to named parameters rest parameter tuple (#30286)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
gabritto authored Mar 18, 2019
1 parent d8591a8 commit 10b9051
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 26 deletions.
2 changes: 2 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,8 @@ namespace ts {
getNeverType: () => neverType,
isSymbolAccessible,
getObjectFlags,
isArrayType,
isTupleType,
isArrayLikeType,
isTypeInvalidDueToUnionDiscriminant,
getAllPossiblePropertiesOfTypes,
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
76 changes: 50 additions & 26 deletions src/services/refactors/convertParamsToDestructuredObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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:
Expand All @@ -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 {
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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");
Original file line number Diff line number Diff line change
@@ -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"] });`
});

0 comments on commit 10b9051

Please sign in to comment.