diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 73e32241f813b..eca7dc7aee97b 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1330,6 +1330,10 @@ namespace ts { return findAncestor(node.parent, isFunctionLike); } + export function getContainingFunctionDeclaration(node: Node): FunctionLikeDeclaration | undefined { + return findAncestor(node.parent, isFunctionLikeDeclaration); + } + export function getContainingClass(node: Node): ClassLikeDeclaration | undefined { return findAncestor(node.parent, isClassLike); } diff --git a/src/services/refactors/convertParamsToDestructuredObject.ts b/src/services/refactors/convertParamsToDestructuredObject.ts index 7d2eded741e0f..fa795f20a8f98 100644 --- a/src/services/refactors/convertParamsToDestructuredObject.ts +++ b/src/services/refactors/convertParamsToDestructuredObject.ts @@ -90,8 +90,8 @@ namespace ts.refactor.convertParamsToDestructuredObject { function groupReferences(referenceEntries: ReadonlyArray): GroupedReferences { const classReferences: ClassReferences = { accessExpressions: [], typeUsages: [] }; const groupedReferences: GroupedReferences = { functionCalls: [], declarations: [], classReferences, valid: true }; - const functionSymbols = map(functionNames, checker.getSymbolAtLocation); - const classSymbols = map(classNames, checker.getSymbolAtLocation); + const functionSymbols = map(functionNames, getSymbolTargetAtLocation); + const classSymbols = map(classNames, getSymbolTargetAtLocation); const isConstructor = isConstructorDeclaration(functionDeclaration); for (const entry of referenceEntries) { @@ -111,7 +111,11 @@ namespace ts.refactor.convertParamsToDestructuredObject { So we need to add a special case for this because when calling a constructor of a class through one of its subclasses, the symbols are going to be different. */ - if (contains(functionSymbols, checker.getSymbolAtLocation(entry.node), symbolComparer) || isNewExpressionTarget(entry.node)) { + if (contains(functionSymbols, getSymbolTargetAtLocation(entry.node)) || isNewExpressionTarget(entry.node)) { + const importOrExportReference = entryToImportOrExport(entry); + if (importOrExportReference) { + continue; + } const decl = entryToDeclaration(entry); if (decl) { groupedReferences.declarations.push(decl); @@ -125,7 +129,12 @@ namespace ts.refactor.convertParamsToDestructuredObject { } } // if the refactored function is a constructor, we must also check if the references to its class are valid - if (isConstructor && contains(classSymbols, checker.getSymbolAtLocation(entry.node), symbolComparer)) { + if (isConstructor && contains(classSymbols, getSymbolTargetAtLocation(entry.node))) { + const importOrExportReference = entryToImportOrExport(entry); + if (importOrExportReference) { + continue; + } + const decl = entryToDeclaration(entry); if (decl) { groupedReferences.declarations.push(decl); @@ -153,10 +162,27 @@ namespace ts.refactor.convertParamsToDestructuredObject { return groupedReferences; } + + function getSymbolTargetAtLocation(node: Node) { + const symbol = checker.getSymbolAtLocation(node); + return symbol && getSymbolTarget(symbol, checker); + } } - function symbolComparer(a: Symbol, b: Symbol): boolean { - return getSymbolTarget(a) === getSymbolTarget(b); + function entryToImportOrExport(entry: FindAllReferences.NodeEntry): Node | undefined { + const node = entry.node; + + if (isImportSpecifier(node.parent) + || isImportClause(node.parent) + || isImportEqualsDeclaration(node.parent) + || isNamespaceImport(node.parent)) { + return node; + } + + if (isExportSpecifier(node.parent) || isExportAssignment(node.parent)) { + return node; + } + return undefined; } function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | undefined { @@ -171,37 +197,31 @@ namespace ts.refactor.convertParamsToDestructuredObject { const functionReference = entry.node; const parent = functionReference.parent; switch (parent.kind) { - // Function call (foo(...) or super(...)) + // foo(...) or super(...) or new Foo(...) case SyntaxKind.CallExpression: - const callExpression = tryCast(parent, isCallExpression); - if (callExpression && callExpression.expression === functionReference) { - return callExpression; - } - break; - // Constructor call (new Foo(...)) case SyntaxKind.NewExpression: - const newExpression = tryCast(parent, isNewExpression); - if (newExpression && newExpression.expression === functionReference) { - return newExpression; + const callOrNewExpression = tryCast(parent, isCallOrNewExpression); + if (callOrNewExpression && callOrNewExpression.expression === functionReference) { + return callOrNewExpression; } break; - // Method call (x.foo(...)) + // x.foo(...) case SyntaxKind.PropertyAccessExpression: const propertyAccessExpression = tryCast(parent, isPropertyAccessExpression); if (propertyAccessExpression && propertyAccessExpression.parent && propertyAccessExpression.name === functionReference) { - const callExpression = tryCast(propertyAccessExpression.parent, isCallExpression); - if (callExpression && callExpression.expression === propertyAccessExpression) { - return callExpression; + const callOrNewExpression = tryCast(propertyAccessExpression.parent, isCallOrNewExpression); + if (callOrNewExpression && callOrNewExpression.expression === propertyAccessExpression) { + return callOrNewExpression; } } break; - // Method call (x["foo"](...)) + // x["foo"](...) case SyntaxKind.ElementAccessExpression: const elementAccessExpression = tryCast(parent, isElementAccessExpression); if (elementAccessExpression && elementAccessExpression.parent && elementAccessExpression.argumentExpression === functionReference) { - const callExpression = tryCast(elementAccessExpression.parent, isCallExpression); - if (callExpression && callExpression.expression === elementAccessExpression) { - return callExpression; + const callOrNewExpression = tryCast(elementAccessExpression.parent, isCallOrNewExpression); + if (callOrNewExpression && callOrNewExpression.expression === elementAccessExpression) { + return callOrNewExpression; } } break; @@ -244,7 +264,7 @@ namespace ts.refactor.convertParamsToDestructuredObject { function getFunctionDeclarationAtPosition(file: SourceFile, startPosition: number, checker: TypeChecker): ValidFunctionDeclaration | undefined { const node = getTouchingToken(file, startPosition); - const functionDeclaration = getContainingFunction(node); + const functionDeclaration = getContainingFunctionDeclaration(node); // don't offer refactor on top-level JSDoc if (isTopLevelJSDoc(node)) return undefined; @@ -267,25 +287,21 @@ namespace ts.refactor.convertParamsToDestructuredObject { } function isValidFunctionDeclaration( - functionDeclaration: SignatureDeclaration, + functionDeclaration: FunctionLikeDeclaration, checker: TypeChecker): functionDeclaration is ValidFunctionDeclaration { if (!isValidParameterNodeArray(functionDeclaration.parameters, checker)) return false; switch (functionDeclaration.kind) { case SyntaxKind.FunctionDeclaration: + return hasNameOrDefault(functionDeclaration) && isSingleImplementation(functionDeclaration, checker); case SyntaxKind.MethodDeclaration: - return !!functionDeclaration.name - && !!functionDeclaration.body - && !checker.isImplementationOfOverload(functionDeclaration); + return isSingleImplementation(functionDeclaration, checker); case SyntaxKind.Constructor: if (isClassDeclaration(functionDeclaration.parent)) { - return !!functionDeclaration.body - && !!functionDeclaration.parent.name - && !checker.isImplementationOfOverload(functionDeclaration); + return hasNameOrDefault(functionDeclaration.parent) && isSingleImplementation(functionDeclaration, checker); } else { return isValidVariableDeclaration(functionDeclaration.parent.parent) - && !!functionDeclaration.body - && !checker.isImplementationOfOverload(functionDeclaration); + && isSingleImplementation(functionDeclaration, checker); } case SyntaxKind.FunctionExpression: case SyntaxKind.ArrowFunction: @@ -294,6 +310,18 @@ namespace ts.refactor.convertParamsToDestructuredObject { return false; } + function isSingleImplementation(functionDeclaration: FunctionLikeDeclaration, checker: TypeChecker): boolean { + return !!functionDeclaration.body && !checker.isImplementationOfOverload(functionDeclaration); + } + + function hasNameOrDefault(functionOrClassDeclaration: FunctionDeclaration | ClassDeclaration): boolean { + if (!functionOrClassDeclaration.name) { + const defaultKeyword = findModifier(functionOrClassDeclaration, SyntaxKind.DefaultKeyword); + return !!defaultKeyword; + } + return true; + } + function isValidParameterNodeArray( parameters: NodeArray, checker: TypeChecker): parameters is ValidParameterNodeArray { @@ -488,11 +516,17 @@ namespace ts.refactor.convertParamsToDestructuredObject { return getTextOfIdentifierOrLiteral(paramDeclaration.name); } - function getClassNames(constructorDeclaration: ValidConstructor): Identifier[] { + function getClassNames(constructorDeclaration: ValidConstructor): (Identifier | Modifier)[] { switch (constructorDeclaration.parent.kind) { case SyntaxKind.ClassDeclaration: const classDeclaration = constructorDeclaration.parent; - return [classDeclaration.name]; + if (classDeclaration.name) return [classDeclaration.name]; + // If the class declaration doesn't have a name, it should have a default modifier. + // We validated this in `isValidFunctionDeclaration` through `hasNameOrDefault` + const defaultModifier = Debug.assertDefined( + findModifier(classDeclaration, SyntaxKind.DefaultKeyword), + "Nameless class declaration should be a default export"); + return [defaultModifier]; case SyntaxKind.ClassExpression: const classExpression = constructorDeclaration.parent; const variableDeclaration = constructorDeclaration.parent.parent; @@ -505,10 +539,19 @@ namespace ts.refactor.convertParamsToDestructuredObject { function getFunctionNames(functionDeclaration: ValidFunctionDeclaration): Node[] { switch (functionDeclaration.kind) { case SyntaxKind.FunctionDeclaration: + if (functionDeclaration.name) return [functionDeclaration.name]; + // If the function declaration doesn't have a name, it should have a default modifier. + // We validated this in `isValidFunctionDeclaration` through `hasNameOrDefault` + const defaultModifier = Debug.assertDefined( + findModifier(functionDeclaration, SyntaxKind.DefaultKeyword), + "Nameless function declaration should be a default export"); + return [defaultModifier]; case SyntaxKind.MethodDeclaration: return [functionDeclaration.name]; case SyntaxKind.Constructor: - const ctrKeyword = findChildOfKind(functionDeclaration, SyntaxKind.ConstructorKeyword, functionDeclaration.getSourceFile())!; + const ctrKeyword = Debug.assertDefined( + findChildOfKind(functionDeclaration, SyntaxKind.ConstructorKeyword, functionDeclaration.getSourceFile()), + "Constructor declaration should have constructor keyword"); if (functionDeclaration.parent.kind === SyntaxKind.ClassExpression) { const variableDeclaration = functionDeclaration.parent.parent; return [variableDeclaration.name, ctrKeyword]; @@ -532,13 +575,12 @@ namespace ts.refactor.convertParamsToDestructuredObject { } interface ValidConstructor extends ConstructorDeclaration { - parent: (ClassDeclaration & { name: Identifier }) | (ClassExpression & { parent: ValidVariableDeclaration }); + parent: ClassDeclaration | (ClassExpression & { parent: ValidVariableDeclaration }); parameters: NodeArray; body: FunctionBody; } interface ValidFunction extends FunctionDeclaration { - name: Identifier; parameters: NodeArray; body: FunctionBody; } diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 1e9c02fba9664..7dc96c108041d 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1664,10 +1664,15 @@ namespace ts { return ensureScriptKind(fileName, host && host.getScriptKind && host.getScriptKind(fileName)); } - export function getSymbolTarget(symbol: Symbol): Symbol { + export function getSymbolTarget(symbol: Symbol, checker: TypeChecker): Symbol { let next: Symbol = symbol; - while (isTransientSymbol(next) && next.target) { - next = next.target; + while (isAliasSymbol(next) || (isTransientSymbol(next) && next.target)) { + if (isTransientSymbol(next) && next.target) { + next = next.target; + } + else { + next = skipAlias(next, checker); + } } return next; } @@ -1676,6 +1681,10 @@ namespace ts { return (symbol.flags & SymbolFlags.Transient) !== 0; } + function isAliasSymbol(symbol: Symbol): boolean { + return (symbol.flags & SymbolFlags.Alias) !== 0; + } + export function getUniqueSymbolId(symbol: Symbol, checker: TypeChecker) { return getSymbolId(skipAlias(symbol, checker)); } diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts index 187086bcec8fa..74d02caec133f 100644 --- a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_badRestParam.ts @@ -14,7 +14,7 @@ ////} goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to named parameters"); +verify.not.refactorAvailable("Convert parameters to destructured object"); goTo.select("c", "d"); -verify.not.refactorAvailable("Convert to named parameters"); \ No newline at end of file +verify.not.refactorAvailable("Convert parameters to destructured object"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_defaultClass.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_defaultClass.ts deleted file mode 100644 index 17c7580fc8830..0000000000000 --- a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_defaultClass.ts +++ /dev/null @@ -1,8 +0,0 @@ -/// - -/////export default class { -//// constructor(/*a*/a: number, b = { x: 1 }/*b*/) {} -////} - -goTo.select("a", "b"); -verify.not.refactorAvailable("Convert parameters to destructured object"); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction.ts new file mode 100644 index 0000000000000..a4b37c7405a6b --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction.ts @@ -0,0 +1,24 @@ +/// + +// @Filename: f.ts +////export function f(/*a*/a: number, b: string/*b*/): string { +//// return b; +////} + +// @Filename: a.ts +////import { f as g } from "./f"; +////g(4, "b"); + +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: `export function f({ a, b }: { a: number; b: string; }): string { + return b; +}` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import { f as g } from "./f"; +g({ a: 4, b: "b" });`) diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction2.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction2.ts new file mode 100644 index 0000000000000..a83c11bd1fd10 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction2.ts @@ -0,0 +1,24 @@ +/// + +// @Filename: f.ts +////export default function f(/*a*/a: number, b: string/*b*/): string { +//// return b; +////} + +// @Filename: a.ts +////import g from "./f"; +////g(4, "b"); + +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: `export default function f({ a, b }: { a: number; b: string; }): string { + return b; +}` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import g from "./f"; +g({ a: 4, b: "b" });`) \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction3.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction3.ts new file mode 100644 index 0000000000000..6a2ecc1cfc373 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction3.ts @@ -0,0 +1,23 @@ +/// + +// @Filename: f.ts +////function foo(/*a*/a: string, b: string/*b*/) { } +////export = foo; + +// @Filename: a.ts +////import bar = require("./f"); +////bar("a", "b"); + + +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 foo({ a, b }: { a: string; b: string; }) { } +export = foo;` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import bar = require("./f"); +bar({ a: "a", b: "b" });`) \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction4.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction4.ts new file mode 100644 index 0000000000000..729878b2b2233 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction4.ts @@ -0,0 +1,27 @@ +/// + +// @Filename: f.ts +////export { foo as default }; +////function /*a*/foo/*b*/(a: number, b: number) { +//// return a + b; +////} + +// @Filename: a.ts +////import bar from "./f"; +////bar(1, 2); + + +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: `export { foo as default }; +function foo({ a, b }: { a: number; b: number; }) { + return a + b; +}` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import bar from "./f"; +bar({ a: 1, b: 2 });`) \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction5.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction5.ts new file mode 100644 index 0000000000000..17a41880a5547 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction5.ts @@ -0,0 +1,27 @@ +/// + +// @Filename: f.ts +////export class C { +//// /*a*/constructor/*b*/(a: number, b: number) { } +////} + +// @Filename: a.ts +////import f = require("./f"); +////const c = new f.C(1, 2); +////const c1 = new f["C"](1, 2); + + +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: `export class C { + constructor({ a, b }: { a: number; b: number; }) { } +}` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import f = require("./f"); +const c = new f.C({ a: 1, b: 2 }); +const c1 = new f["C"]({ a: 1, b: 2 });`) \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction6.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction6.ts new file mode 100644 index 0000000000000..615a49c026767 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_importedFunction6.ts @@ -0,0 +1,21 @@ +/// + +// @Filename: f.ts +////export function /*a*/foo/*b*/(a: string, b: string) { } + +// @Filename: a.ts +////import * as f from "./f"; +////f.foo("a", "b"); + + +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: `export function foo({ a, b }: { a: string; b: string; }) { }` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import * as f from "./f"; +f.foo({ a: "a", b: "b" });`) \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_namelessClass.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_namelessClass.ts new file mode 100644 index 0000000000000..d38bff48b249e --- /dev/null +++ b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_namelessClass.ts @@ -0,0 +1,25 @@ +/// + +// @Filename: f.ts +////export default class { +//// /*a*/constructor/*b*/(a: string, b: string) { } +////} + +// @Filename: a.ts +////import C from "./f"; +////const c = new C("a", "b"); + + +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: `export default class { + constructor({ a, b }: { a: string; b: string; }) { } +}` +}); + +goTo.file("a.ts"); +verify.currentFileContentIs(`import C from "./f"; +const c = new C({ a: "a", b: "b" });`) \ No newline at end of file