diff --git a/typescript/src/codeActions/custom/addDestructure.ts b/typescript/src/codeActions/custom/addDestructure.ts index ce0b6ef..221f080 100644 --- a/typescript/src/codeActions/custom/addDestructure.ts +++ b/typescript/src/codeActions/custom/addDestructure.ts @@ -1,10 +1,4 @@ -import { - findChildContainingExactPosition, - getChangesTracker, - getPositionHighlights, - isValidInitializerForDestructure, - isNameUniqueAtNodeClosestScope, -} from '../../utils' +import { findChildContainingExactPosition, getChangesTracker, getPositionHighlights, isValidInitializerForDestructure, makeUniqueName } from '../../utils' import { CodeAction } from '../getCodeActions' const createDestructuredDeclaration = (initializer: ts.Expression, type: ts.TypeNode | undefined, declarationName: ts.BindingName) => { @@ -32,14 +26,15 @@ const addDestructureToVariableWithSplittedPropertyAccessors = ( formatOptions: ts.FormatCodeSettings | undefined, languageService: ts.LanguageService, ) => { - if (!ts.isIdentifier(node) && !(ts.isPropertyAccessExpression(node.parent) || ts.isParameter(node.parent))) return + if (!ts.isIdentifier(node) && !(ts.isPropertyAccessExpression(node.parent) || ts.isParameter(node.parent) || !ts.isElementAccessExpression(node.parent))) + return const highlightPositions = getPositionHighlights(node.getStart(), sourceFile, languageService) if (!highlightPositions) return const tracker = getChangesTracker(formatOptions ?? {}) - const propertyNames: Array<{ initial: string; unique: string | undefined }> = [] + const propertyNames: Array<{ initial: string; unique: string | undefined; dotDotDotToken?: ts.DotDotDotToken }> = [] let nodeToReplaceWithBindingPattern: ts.Identifier | undefined for (const pos of highlightPositions) { @@ -47,16 +42,47 @@ const addDestructureToVariableWithSplittedPropertyAccessors = ( if (!highlightedNode) continue - if (ts.isIdentifier(highlightedNode) && ts.isPropertyAccessExpression(highlightedNode.parent)) { - const propertyAccessorName = highlightedNode.parent.name.getText() + if ( + ts.isIdentifier(highlightedNode) && + (ts.isPropertyAccessExpression(highlightedNode.parent) || ts.isElementAccessExpression(highlightedNode.parent)) + ) { + if (ts.isElementAccessExpression(highlightedNode.parent) && ts.isIdentifier(highlightedNode.parent.argumentExpression)) { + const uniqueName = makeUniqueName('newVariable', node, languageService, sourceFile) - const uniquePropertyName = isNameUniqueAtNodeClosestScope(propertyAccessorName, node, languageService.getProgram()!.getTypeChecker()) - ? undefined - : tsFull.getUniqueName(propertyAccessorName, sourceFile as any) + propertyNames.push({ + initial: 'newVariable', + unique: uniqueName === 'newVariable' ? undefined : uniqueName, + dotDotDotToken: ts.factory.createToken(ts.SyntaxKind.DotDotDotToken), + }) - propertyNames.push({ initial: propertyAccessorName, unique: uniquePropertyName }) + tracker.replaceRangeWithText(sourceFile, { pos, end: highlightedNode.end }, uniqueName) - tracker.replaceRangeWithText(sourceFile, { pos, end: highlightedNode.parent.end }, uniquePropertyName ?? propertyAccessorName) + continue + } + const indexedAccessorName = + ts.isElementAccessExpression(highlightedNode.parent) && ts.isStringLiteral(highlightedNode.parent.argumentExpression) + ? highlightedNode.parent.argumentExpression.text + : undefined + + const accessorName = ts.isPropertyAccessExpression(highlightedNode.parent) ? highlightedNode.parent.name.getText() : indexedAccessorName + + if (!accessorName) continue + + const uniqueName = makeUniqueName(accessorName, node, languageService, sourceFile) + + propertyNames.push({ initial: accessorName, unique: uniqueName === accessorName ? undefined : uniqueName }) + + // Replace both variable and property access expression `a.fo|o` -> `foo` + // if (ts.isIdentifier(highlightedNode.parent.expression)) { + // tracker.replaceRangeWithText( + // sourceFile, + // { pos: highlightedNode.parent.end, end: highlightedNode.parent.expression.end }, + // uniquePropertyName || propertyAccessorName, + // ) + // continue + // } + + tracker.replaceRangeWithText(sourceFile, { pos, end: highlightedNode.parent.end }, uniqueName) continue } @@ -64,14 +90,22 @@ const addDestructureToVariableWithSplittedPropertyAccessors = ( nodeToReplaceWithBindingPattern = highlightedNode continue } + // Support for `const a = { foo: 1 }; a.fo|o` refactor activation + // if (ts.isIdentifier(highlightedNode) && ts.isPropertyAssignment(highlightedNode.parent)) { + // const closestParent = ts.findAncestor(highlightedNode.parent, n => ts.isVariableDeclaration(n)) + + // if (!closestParent || !ts.isVariableDeclaration(closestParent) || !ts.isIdentifier(closestParent.name)) continue + // nodeToReplaceWithBindingPattern = closestParent.name + // } } if (!nodeToReplaceWithBindingPattern || propertyNames.length === 0) return - const bindings = propertyNames.map(({ initial, unique }) => { - return ts.factory.createBindingElement(undefined, unique ? initial : undefined, unique ?? initial) + const bindings = propertyNames.map(({ initial, unique, dotDotDotToken }) => { + return ts.factory.createBindingElement(dotDotDotToken, unique ? initial : undefined, unique ?? initial) }) - const bindingPattern = ts.factory.createObjectBindingPattern(bindings) + const bindingsWithRestLast = bindings.sort((a, b) => (!a.dotDotDotToken && !b.dotDotDotToken ? 0 : -1)) + const bindingPattern = ts.factory.createObjectBindingPattern(bindingsWithRestLast) const { pos, end } = nodeToReplaceWithBindingPattern tracker.replaceRange( diff --git a/typescript/src/codeActions/custom/fromDestructure.ts b/typescript/src/codeActions/custom/fromDestructure.ts index 4bbd57a..6a6a655 100644 --- a/typescript/src/codeActions/custom/fromDestructure.ts +++ b/typescript/src/codeActions/custom/fromDestructure.ts @@ -66,12 +66,14 @@ const convertFromDestructureWithVariableNameReplacement = ( const BASE_VARIABLE_NAME = 'newVariable' - const variableName = isNameUniqueAtNodeClosestScope(BASE_VARIABLE_NAME, declarationName, languageService.getProgram()!.getTypeChecker()) + const uniqueVariableName = isNameUniqueAtNodeClosestScope(BASE_VARIABLE_NAME, declarationName, languageService.getProgram()!.getTypeChecker()) ? BASE_VARIABLE_NAME : tsFull.getUniqueName(BASE_VARIABLE_NAME, sourceFile as unknown as FullSourceFile) + const uniqueVariableIdentifier = ts.factory.createIdentifier(uniqueVariableName) + for (const binding of bindings) { - const declaration = createFlattenedExpressionFromDestructuring(binding, ts.factory.createIdentifier(variableName)) + const declaration = createFlattenedExpressionFromDestructuring(binding, uniqueVariableIdentifier) /** Important to use `getEnd()` here to get correct highlights for destructured and renamed binding, e.g. `{ bar: bar_1 }` */ const bindingNameEndPos = binding.getEnd() @@ -85,20 +87,18 @@ const convertFromDestructureWithVariableNameReplacement = ( } const node = findChildContainingExactPosition(sourceFile, pos) - if (!node) continue + if (!node || ts.isPropertyAssignment(node.parent)) continue const printer = ts.createPrinter() - tracker.replaceRangeWithText(sourceFile, { pos, end: node.end }, printer.printNode(ts.EmitHint.Unspecified, declaration, sourceFile)) + // If dotDotDotToken is present, we work with rest element, so we need to replace it with identifier + const replacement = binding.dotDotDotToken ? uniqueVariableIdentifier : declaration + tracker.replaceRangeWithText(sourceFile, { pos, end: node.end }, printer.printNode(ts.EmitHint.Unspecified, replacement, sourceFile)) } } const declarationNameLeadingTrivia = declarationName.getLeadingTriviaWidth(sourceFile) - tracker.replaceRange( - sourceFile, - { pos: declarationName.pos + declarationNameLeadingTrivia, end: declarationName.end }, - ts.factory.createIdentifier(variableName), - ) + tracker.replaceRange(sourceFile, { pos: declarationName.pos + declarationNameLeadingTrivia, end: declarationName.end }, uniqueVariableIdentifier) const changes = tracker.getChanges() return { edits: [ diff --git a/typescript/src/utils.ts b/typescript/src/utils.ts index 45d5196..b71c9bf 100644 --- a/typescript/src/utils.ts +++ b/typescript/src/utils.ts @@ -80,8 +80,8 @@ export const getIndentFromPos = (typescript: typeof ts, sourceFile: ts.SourceFil ) } -export const findClosestParent = (node: ts.Node, stopKinds: ts.SyntaxKind[], rejectKinds: ts.SyntaxKind[]) => { - rejectKinds = [...rejectKinds, ts.SyntaxKind.SourceFile] +export const findClosestParent = (node: ts.Node, stopKinds: ts.SyntaxKind[], rejectKinds: ts.SyntaxKind[], skipSourceFile = true) => { + rejectKinds = [...rejectKinds, ...(skipSourceFile ? [ts.SyntaxKind.SourceFile] : [])] while (node && !stopKinds.includes(node.kind)) { if (rejectKinds.includes(node.kind)) return node = node.parent @@ -352,24 +352,79 @@ export const isValidInitializerForDestructure = (match: ts.Expression) => { } export const isNameUniqueAtLocation = (name: string, location: ts.Node | undefined, typeChecker: ts.TypeChecker) => { const checker = getFullTypeChecker(typeChecker) - let result: boolean | undefined + let hasCollision: boolean | undefined const checkCollision = (childNode: ts.Node) => { - if (result) return - result = !!checker.resolveName(name, childNode as unknown as import('typescript-full').Node, ts.SymbolFlags.Value, true) + if (hasCollision) return + hasCollision = !!checker.resolveName(name, childNode as unknown as import('typescript-full').Node, ts.SymbolFlags.Value, true) if (ts.isBlock(childNode)) { childNode.forEachChild(checkCollision) } } - location?.forEachChild(checkCollision) - return !result + if (!location) return + + if (ts.isSourceFile(location)) { + hasCollision = createUniqueName(name, location as any) !== name + } else { + location.forEachChild(checkCollision) + } + return !hasCollision } export const isNameUniqueAtNodeClosestScope = (name: string, node: ts.Node, typeChecker: ts.TypeChecker) => { const closestScope = findClosestParent( node, - [ts.SyntaxKind.Block, ts.SyntaxKind.FunctionDeclaration, ts.SyntaxKind.FunctionExpression, ts.SyntaxKind.ArrowFunction], + [ts.SyntaxKind.Block, ts.SyntaxKind.FunctionDeclaration, ts.SyntaxKind.FunctionExpression, ts.SyntaxKind.ArrowFunction, ts.SyntaxKind.SourceFile], [], + false, ) return isNameUniqueAtLocation(name, closestScope, typeChecker) } + +const createUniqueName = (name: string, sourceFile: ts.SourceFile) => { + /** + * A free identifier is an identifier that can be accessed through name lookup as a local variable. + * In the expression `x.y`, `x` is a free identifier, but `y` is not. + */ + const forEachFreeIdentifier = (node: ts.Node, cb: (id: ts.Identifier) => void) => { + if (ts.isIdentifier(node) && isFreeIdentifier(node)) cb(node) + node.forEachChild(child => forEachFreeIdentifier(child, cb)) + } + + const isFreeIdentifier = (node: ts.Identifier): boolean => { + const { parent } = node + switch (parent.kind) { + case ts.SyntaxKind.PropertyAccessExpression: + return (parent as ts.PropertyAccessExpression).name !== node + case ts.SyntaxKind.BindingElement: + return (parent as ts.BindingElement).propertyName !== node + case ts.SyntaxKind.ImportSpecifier: + return (parent as ts.ImportSpecifier).propertyName !== node + case ts.SyntaxKind.PropertyAssignment: + return (parent as ts.PropertyAssignment).name !== node + default: + return true + } + } + const collectFreeIdentifiers = (file: ts.SourceFile) => { + const arr: string[] = [] + forEachFreeIdentifier(file, id => arr.push(id.text)) + return arr + } + + const identifiers = collectFreeIdentifiers(sourceFile) + while (identifiers.includes(name)) { + name = `_${name}` + } + return name +} + +export const makeUniqueName = (accessorName: string, node: ts.Node, languageService: ts.LanguageService, sourceFile: ts.SourceFile) => { + const isNameUniqueInScope = isNameUniqueAtNodeClosestScope(accessorName, node, languageService.getProgram()!.getTypeChecker()) + const isReservedWord = tsFull.isIdentifierANonContextualKeyword(tsFull.factory.createIdentifier(accessorName)) + + const uniquePropertyName = isNameUniqueInScope ? undefined : createUniqueName(accessorName, sourceFile) + + const uniqueReservedPropName = isReservedWord ? createUniqueName(`_${accessorName}`, sourceFile) : undefined + return uniqueReservedPropName || uniquePropertyName || accessorName +} diff --git a/typescript/test/codeActions.spec.ts b/typescript/test/codeActions.spec.ts index 02b5ca9..b1d7026 100644 --- a/typescript/test/codeActions.spec.ts +++ b/typescript/test/codeActions.spec.ts @@ -1,3 +1,4 @@ +import { initial } from 'lodash' import { fourslashLikeTester } from './testing' test('Split Declaration and Initialization', () => { @@ -122,24 +123,37 @@ describe('Add destructure', () => { `, }) }) - test('Should destruct function params', () => { - const { codeAction } = fourslashLikeTester( - /* ts */ ` + describe('Should destruct function params', () => { + const expected = /* ts */ ` + function fn({ bar, foo }) { + const something = bar + foo + } + ` + test('Cursor position on param', () => { + const cursorOnParam = /* ts */ ` function fn(/*t*/newVariable/*t*/) { const something = newVariable.bar + newVariable.foo } - `, - undefined, - { dedent: true }, - ) + ` + const { codeAction } = fourslashLikeTester(cursorOnParam, undefined, { dedent: true }) - codeAction(0, { - refactorName: 'Add Destruct', - newContent: /* ts */ ` - function fn({ bar, foo }) { - const something = bar + foo + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + test.skip('Cursor position on accessor', () => { + const cursorOnParam = /* ts */ ` + function fn(newVariable) { + const something = newVariable./*t*/bar/*t*/ + newVariable.foo } - `, + ` + const { codeAction } = fourslashLikeTester(cursorOnParam, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) }) }) test('Should work with name collisions', () => { @@ -158,14 +172,121 @@ describe('Add destructure', () => { codeAction(0, { refactorName: 'Add Destruct', newContent: /* ts */ ` - function fn({ bar: bar_1, foo: foo_1 }) { + function fn({ bar: _bar, foo: _foo }) { const bar = 4 const foo = 5 - const something = bar_1 + foo_1 + const something = _bar + _foo } `, }) }) + describe('Works with inline object', () => { + const expected = /* ts */ ` + const { foo } = { + foo: 1, + } + foo + ` + test('Cursor position on object variable declaration', () => { + const cursorOnObjVarDecl = /* ts */ ` + const /*t*/a/*t*/ = { + foo: 1, + } + a.foo + ` + const { codeAction } = fourslashLikeTester(cursorOnObjVarDecl, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + test.skip('Cursor position on accessor', () => { + const cursorOnAccessor = /* ts */ ` + const a = { + foo: 1, + } + + a./*t*/foo/*t*/ + ` + const { codeAction } = fourslashLikeTester(cursorOnAccessor, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + }) + describe('Handles reserved words', () => { + test('Makes unique identifier for reserved word', () => { + const initial = /* ts */ ` + const /*t*/a/*t*/ = { + class: 1, + } + a.class + ` + const expected = /* ts */ ` + const { class: _class } = { + class: 1, + } + _class + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + }) + describe('Should work with index access', () => { + test('Adds destructure when index access content is string', () => { + const initial = /* ts */ ` + const /*t*/newVariable/*t*/ = { + foo: 1, + } + newVariable['foo'] + ` + const expected = /* ts */ ` + const { foo } = { + foo: 1, + } + foo + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + test('Should add rest elements to destructure when index access content is expression', () => { + const initial = /* ts */ ` + const /*t*/object/*t*/ = { + foo: 1, + bar: 2, + } + const foo = 'foo' + object[foo] + object.bar + ` + const expected = /* ts */ ` + const { bar, ...newVariable } = { + foo: 1, + bar: 2, + } + const foo = 'foo' + newVariable[foo] + bar + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + }) }) describe('From destructure', () => { @@ -343,4 +464,80 @@ describe('From destructure', () => { `, }) }) + test('Should work with rest elements destructure', () => { + const initial = /* ts */ ` + const { /*t*/foo/*t*/, ...a } = { + bar: 1, + foo: 2, + } + + a.bar + foo + ` + const expected = /* ts */ ` + const newVariable = { + bar: 1, + foo: 2, + } + + newVariable.bar + newVariable.foo + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'From Destruct', + newContent: expected, + }) + }) + describe('Works with inline object', () => { + test('Destructured only one property', () => { + const initial = /* ts*/ ` + const { /*t*/foo/*t*/ } = { + foo: 1, + } + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + const newContent = codeAction( + 0, + { + refactorName: 'From Destruct', + }, + {}, + { compareContent: true }, + ) + expect(newContent).toMatchInlineSnapshot(` + " + const foo = { + foo: 1, + }.foo + " + `) + }) + test('Destructured two or more properties', () => { + const initial = /* ts*/ ` + const { /*t*/foo/*t*/, bar } = { + foo: 1, + bar: 2, + } + foo; + bar; + ` + const expected = /* ts*/ ` + const newVariable = { + foo: 1, + bar: 2, + } + newVariable.foo; + newVariable.bar; + ` + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'From Destruct', + newContent: expected, + }) + }) + }) })