From b1da6eead3c34b9237cd55b10366def7eb6a895a Mon Sep 17 00:00:00 2001 From: Oleksandr T Date: Fri, 7 Jul 2023 20:51:07 +0300 Subject: [PATCH] fix(54310): "Move to file" does not eliminate re-export (#54329) --- src/services/refactors/moveToFile.ts | 63 ++++++++++++++++++- src/services/textChanges.ts | 4 ++ .../cases/fourslash/moveToFile_reExports1.ts | 31 +++++++++ .../cases/fourslash/moveToFile_reExports2.ts | 28 +++++++++ .../cases/fourslash/moveToFile_reExports3.ts | 26 ++++++++ .../cases/fourslash/moveToFile_reExports4.ts | 25 ++++++++ .../cases/fourslash/moveToFile_reExports5.ts | 32 ++++++++++ 7 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 tests/cases/fourslash/moveToFile_reExports1.ts create mode 100644 tests/cases/fourslash/moveToFile_reExports2.ts create mode 100644 tests/cases/fourslash/moveToFile_reExports3.ts create mode 100644 tests/cases/fourslash/moveToFile_reExports4.ts create mode 100644 tests/cases/fourslash/moveToFile_reExports5.ts diff --git a/src/services/refactors/moveToFile.ts b/src/services/refactors/moveToFile.ts index 8c5480b9c75..5c7abdd314e 100644 --- a/src/services/refactors/moveToFile.ts +++ b/src/services/refactors/moveToFile.ts @@ -1,8 +1,10 @@ import { getModuleSpecifier } from "../../compiler/moduleSpecifiers"; import { + __String, AnyImportOrRequireStatement, append, ApplicableRefactorInfo, + arrayFrom, AssignmentDeclarationKind, BinaryExpression, BindingElement, @@ -26,15 +28,18 @@ import { emptyArray, EnumDeclaration, escapeLeadingUnderscores, + ExportDeclaration, Expression, ExpressionStatement, extensionFromPath, ExternalModuleReference, factory, fileShouldUseJavaScriptRequire, + filter, find, FindAllReferences, findIndex, + findLast, findLastIndex, firstDefined, flatMap, @@ -69,6 +74,8 @@ import { isBinaryExpression, isBindingElement, isDeclarationName, + isExportDeclaration, + isExportSpecifier, isExpressionStatement, isExternalModuleReference, isFunctionLikeDeclaration, @@ -76,6 +83,7 @@ import { isImportDeclaration, isImportEqualsDeclaration, isNamedDeclaration, + isNamedExports, isObjectLiteralExpression, isOmittedExpression, isPrologueDirective, @@ -236,7 +244,7 @@ function getNewStatementsAndRemoveFromOldFile( const body = addExports(oldFile, toMove.all, usage.oldFileImportsFromTargetFile, useEsModuleSyntax); if (typeof targetFile !== "string") { if (targetFile.statements.length > 0) { - changes.insertNodesAfter(targetFile, targetFile.statements[targetFile.statements.length - 1], body); + moveStatementsToTargetFile(changes, program, body, targetFile, toMove); } else { changes.insertNodesAtEndOfFile(targetFile, body, /*blankLineBetween*/ false); @@ -1137,6 +1145,59 @@ function isNonVariableTopLevelDeclaration(node: Node): node is NonVariableTopLev } } +function moveStatementsToTargetFile(changes: textChanges.ChangeTracker, program: Program, statements: readonly Statement[], targetFile: SourceFile, toMove: ToMove) { + const removedExports = new Set(); + const targetExports = targetFile.symbol?.exports; + if (targetExports) { + const checker = program.getTypeChecker(); + const targetToSourceExports = new Map>(); + + for (const node of toMove.all) { + if (isTopLevelDeclarationStatement(node) && hasSyntacticModifier(node, ModifierFlags.Export)) { + forEachTopLevelDeclaration(node, declaration => { + const targetDeclarations = canHaveSymbol(declaration) ? targetExports.get(declaration.symbol.escapedName)?.declarations : undefined; + const exportDeclaration = firstDefined(targetDeclarations, d => + isExportDeclaration(d) ? d : + isExportSpecifier(d) ? tryCast(d.parent.parent, isExportDeclaration) : undefined); + if (exportDeclaration && exportDeclaration.moduleSpecifier) { + targetToSourceExports.set(exportDeclaration, + (targetToSourceExports.get(exportDeclaration) || new Set()).add(declaration)); + } + }); + } + } + + for (const [exportDeclaration, topLevelDeclarations] of arrayFrom(targetToSourceExports)) { + if (exportDeclaration.exportClause && isNamedExports(exportDeclaration.exportClause) && length(exportDeclaration.exportClause.elements)) { + const elements = exportDeclaration.exportClause.elements; + const updatedElements = filter(elements, elem => + find(skipAlias(elem.symbol, checker).declarations, d => isTopLevelDeclaration(d) && topLevelDeclarations.has(d)) === undefined); + + if (length(updatedElements) === 0) { + changes.deleteNode(targetFile, exportDeclaration); + removedExports.add(exportDeclaration); + continue; + } + + if (length(updatedElements) < length(elements)) { + changes.replaceNode(targetFile, exportDeclaration, + factory.updateExportDeclaration(exportDeclaration, exportDeclaration.modifiers, exportDeclaration.isTypeOnly, + factory.updateNamedExports(exportDeclaration.exportClause , factory.createNodeArray(updatedElements, elements.hasTrailingComma)), exportDeclaration.moduleSpecifier, exportDeclaration.assertClause)); + } + } + } + } + + const lastReExport = findLast(targetFile.statements, n => + isExportDeclaration(n) && !!n.moduleSpecifier && !removedExports.has(n)); + if (lastReExport) { + changes.insertNodesBefore(targetFile, lastReExport, statements, /*blankLineBetween*/ true); + } + else { + changes.insertNodesAfter(targetFile, targetFile.statements[targetFile.statements.length - 1], statements); + } +} + function getOverloadRangeToMove(sourceFile: SourceFile, statement: Statement) { if (isFunctionLikeDeclaration(statement)) { const declarations = statement.symbol.declarations; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index d52ab642a5e..fbfdba0f59b 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -671,6 +671,10 @@ export class ChangeTracker { this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, options), newNode, this.getOptionsForInsertNodeBefore(before, newNode, blankLineBetween)); } + public insertNodesBefore(sourceFile: SourceFile, before: Node, newNodes: readonly Node[], blankLineBetween = false, options: ConfigurableStartEnd = {}): void { + this.insertNodesAt(sourceFile, getAdjustedStartPosition(sourceFile, before, options), newNodes, this.getOptionsForInsertNodeBefore(before, first(newNodes), blankLineBetween)); + } + public insertModifierAt(sourceFile: SourceFile, pos: number, modifier: SyntaxKind, options: InsertNodeOptions = {}): void { this.insertNodeAt(sourceFile, pos, factory.createToken(modifier), options); } diff --git a/tests/cases/fourslash/moveToFile_reExports1.ts b/tests/cases/fourslash/moveToFile_reExports1.ts new file mode 100644 index 00000000000..e3d8be9f945 --- /dev/null +++ b/tests/cases/fourslash/moveToFile_reExports1.ts @@ -0,0 +1,31 @@ +/// + +// @module: esnext + +//@Filename: /a.ts +////export function foo() { } +////[|export function bar() {}|] + +// @Filename: /b.ts +////export function baz() { } +////export { +//// foo, +//// bar, +////} from "./a"; + +verify.moveToFile({ + newFileContents: { + "/a.ts": +`export function foo() { } +`, + + "/b.ts": +`export function baz() { } +export function bar() { } + +export { + foo, +} from "./a";`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_reExports2.ts b/tests/cases/fourslash/moveToFile_reExports2.ts new file mode 100644 index 00000000000..4d15e318383 --- /dev/null +++ b/tests/cases/fourslash/moveToFile_reExports2.ts @@ -0,0 +1,28 @@ +/// + +// @module: esnext + +//@Filename: /a.ts +////export function foo() { } +////[|export function bar() {}|] + +// @Filename: /b.ts +////export function baz() { } +////export { +//// bar, +////} from "./a"; + +verify.moveToFile({ + newFileContents: { + "/a.ts": +`export function foo() { } +`, + + "/b.ts": +`export function baz() { } + +export function bar() { } +`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_reExports3.ts b/tests/cases/fourslash/moveToFile_reExports3.ts new file mode 100644 index 00000000000..b2c97f6efd5 --- /dev/null +++ b/tests/cases/fourslash/moveToFile_reExports3.ts @@ -0,0 +1,26 @@ +/// + +// @module: esnext + +//@Filename: /a.ts +////export function foo() { } +////[|export function bar() {}|] + +// @Filename: /b.ts +////export function baz() { } +////export * from "./a"; + +verify.moveToFile({ + newFileContents: { + "/a.ts": +`export function foo() { } +`, + + "/b.ts": +`export function baz() { } +export function bar() { } + +export * from "./a";`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_reExports4.ts b/tests/cases/fourslash/moveToFile_reExports4.ts new file mode 100644 index 00000000000..8731497cc53 --- /dev/null +++ b/tests/cases/fourslash/moveToFile_reExports4.ts @@ -0,0 +1,25 @@ +/// + +// @module: esnext + +//@Filename: /a.ts +////[|export function foo() { } +////export function bar() {}|] + +// @Filename: /b.ts +////export function baz() { } +////export { foo, bar } from "./a"; + +verify.moveToFile({ + newFileContents: { + "/a.ts": "", + + "/b.ts": +`export function baz() { } + +export function foo() { } +export function bar() { } +`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_reExports5.ts b/tests/cases/fourslash/moveToFile_reExports5.ts new file mode 100644 index 00000000000..e2f6496440e --- /dev/null +++ b/tests/cases/fourslash/moveToFile_reExports5.ts @@ -0,0 +1,32 @@ +/// + +// @module: esnext + +//@Filename: /a.ts +////export function a() { } + +//@Filename: /b.ts +////export function foo() { } +////[|export function bar() {}|] + +// @Filename: /c.ts +////export function baz() { } +////export * from "./a"; +////export { +//// bar, +////} from "./b"; + +verify.moveToFile({ + newFileContents: { + "/b.ts": +`export function foo() { } +`, + "/c.ts": +`export function baz() { } +export function bar() { } + +export * from "./a"; +`, + }, + interactiveRefactorArguments: { targetFile: "/c.ts" }, +});