diff --git a/src/services/refactors/moveToFile.ts b/src/services/refactors/moveToFile.ts index 29aa22de802..7d99368a385 100644 --- a/src/services/refactors/moveToFile.ts +++ b/src/services/refactors/moveToFile.ts @@ -68,6 +68,7 @@ import { Identifier, ImportDeclaration, ImportEqualsDeclaration, + importFromModuleSpecifier, insertImports, InterfaceDeclaration, InternalSymbolName, @@ -84,6 +85,8 @@ import { isImportDeclaration, isImportEqualsDeclaration, isNamedExports, + isNamedImports, + isObjectBindingPattern, isObjectLiteralExpression, isOmittedExpression, isPrologueDirective, @@ -96,6 +99,7 @@ import { isStringLiteralLike, isValidTypeOnlyAliasUseSite, isVariableDeclaration, + isVariableDeclarationInitializedToRequire, isVariableDeclarationList, isVariableStatement, LanguageServiceHost, @@ -194,16 +198,15 @@ function error(notApplicableReason: string) { function doChange(context: RefactorContext, oldFile: SourceFile, targetFile: string, program: Program, toMove: ToMove, changes: textChanges.ChangeTracker, host: LanguageServiceHost, preferences: UserPreferences): void { const checker = program.getTypeChecker(); - const usage = getUsageInfo(oldFile, toMove.all, checker); // For a new file if (!host.fileExists(targetFile)) { - changes.createNewFile(oldFile, targetFile, getNewStatementsAndRemoveFromOldFile(oldFile, targetFile, usage, changes, toMove, program, host, preferences)); + changes.createNewFile(oldFile, targetFile, getNewStatementsAndRemoveFromOldFile(oldFile, targetFile, getUsageInfo(oldFile, toMove.all, checker), changes, toMove, program, host, preferences)); addNewFileToTsconfig(program, changes, oldFile.fileName, targetFile, hostGetCanonicalFileName(host)); } else { const targetSourceFile = Debug.checkDefined(program.getSourceFile(targetFile)); const importAdder = codefix.createImportAdder(targetSourceFile, context.program, context.preferences, context.host); - getNewStatementsAndRemoveFromOldFile(oldFile, targetSourceFile, usage, changes, toMove, program, host, preferences, importAdder); + getNewStatementsAndRemoveFromOldFile(oldFile, targetSourceFile, getUsageInfo(oldFile, toMove.all, checker, getExistingImports(targetSourceFile, checker)), changes, toMove, program, host, preferences, importAdder); } } @@ -993,7 +996,7 @@ function isPureImport(node: Node): boolean { } /** @internal */ -export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], checker: TypeChecker): UsageInfo { +export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], checker: TypeChecker, existingTargetImports: ReadonlySet = new Set()): UsageInfo { const movedSymbols = new Set(); const oldImportsNeededByTargetFile = new Map(); const targetFileImportsFromOldFile = new Set(); @@ -1010,9 +1013,17 @@ export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], movedSymbols.add(Debug.checkDefined(isExpressionStatement(decl) ? checker.getSymbolAtLocation(decl.expression.left) : decl.symbol, "Need a symbol here")); }); } + + const unusedImportsFromOldFile = new Set(); for (const statement of toMove) { forEachReference(statement, checker, (symbol, isValidTypeOnlyUseSite) => { - if (!symbol.declarations) return; + if (!symbol.declarations) { + return; + } + if (existingTargetImports.has(skipAlias(symbol, checker))) { + unusedImportsFromOldFile.add(symbol); + return; + } for (const decl of symbol.declarations) { if (isInImport(decl)) { const prevIsTypeOnly = oldImportsNeededByTargetFile.get(symbol); @@ -1024,7 +1035,10 @@ export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], } }); } - const unusedImportsFromOldFile = new Set(oldImportsNeededByTargetFile.keys()); + + for (const unusedImport of oldImportsNeededByTargetFile.keys()) { + unusedImportsFromOldFile.add(unusedImport); + } const oldFileImportsFromTargetFile = new Set(); for (const statement of oldFile.statements) { @@ -1228,3 +1242,30 @@ function getOverloadRangeToMove(sourceFile: SourceFile, statement: Statement) { } return undefined; } + +function getExistingImports(sourceFile: SourceFile, checker: TypeChecker) { + const imports = new Set(); + for (const moduleSpecifier of sourceFile.imports) { + const declaration = importFromModuleSpecifier(moduleSpecifier); + if ( + isImportDeclaration(declaration) && declaration.importClause && + declaration.importClause.namedBindings && isNamedImports(declaration.importClause.namedBindings) + ) { + for (const e of declaration.importClause.namedBindings.elements) { + const symbol = checker.getSymbolAtLocation(e.propertyName || e.name); + if (symbol) { + imports.add(skipAlias(symbol, checker)); + } + } + } + if (isVariableDeclarationInitializedToRequire(declaration.parent) && isObjectBindingPattern(declaration.parent.name)) { + for (const e of declaration.parent.name.elements) { + const symbol = checker.getSymbolAtLocation(e.propertyName || e.name); + if (symbol) { + imports.add(skipAlias(symbol, checker)); + } + } + } + } + return imports; +} diff --git a/tests/cases/fourslash/moveToFile_existingImports1.ts b/tests/cases/fourslash/moveToFile_existingImports1.ts new file mode 100644 index 00000000000..f7f5feaf54e --- /dev/null +++ b/tests/cases/fourslash/moveToFile_existingImports1.ts @@ -0,0 +1,24 @@ +/// + +// @filename: /common.ts +////export const x = 1; + +// @filename: /a.ts +////import { x } from "./common"; +////[|export const bar = x;|] + +// @filename: /b.ts +////import { x } from "./common"; +////export const foo = x; + +verify.moveToFile({ + newFileContents: { + "/a.ts": "", + "/b.ts": +`import { x } from "./common"; +export const foo = x; +export const bar = x; +`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_existingImports2.ts b/tests/cases/fourslash/moveToFile_existingImports2.ts new file mode 100644 index 00000000000..a93b542b59d --- /dev/null +++ b/tests/cases/fourslash/moveToFile_existingImports2.ts @@ -0,0 +1,28 @@ +/// + +// @filename: /common.ts +////export const x = 1; + +// @filename: /a.ts +////import { x } from "./common"; +////export const a = x; +////[|export const b = x;|] + +// @filename: /b.ts +////import { x } from "./common"; +////export const a = x; + +verify.moveToFile({ + newFileContents: { + "/a.ts": +`import { x } from "./common"; +export const a = x; +`, + "/b.ts": +`import { x } from "./common"; +export const a = x; +export const b = x; +`, + }, + interactiveRefactorArguments: { targetFile: "/b.ts" }, +}); diff --git a/tests/cases/fourslash/moveToFile_existingImports3.ts b/tests/cases/fourslash/moveToFile_existingImports3.ts new file mode 100644 index 00000000000..0a45aa3dece --- /dev/null +++ b/tests/cases/fourslash/moveToFile_existingImports3.ts @@ -0,0 +1,26 @@ +/// + +// @allowJs: true +// @module: commonjs +// @filename: /common.js +////export const x = 1; + +// @filename: /a.js +////const { x } = require("./common"); +////[|module.exports.b = x;|] + +// @filename: /b.js +////const { x } = require("./common"); +////module.exports.a = x; + +verify.moveToFile({ + newFileContents: { + "/a.js": "", + "/b.js": +`const { x } = require("./common"); +module.exports.a = x; +module.exports.b = x; +`, + }, + interactiveRefactorArguments: { targetFile: "/b.js" }, +}); diff --git a/tests/cases/fourslash/moveToFile_existingImports4.ts b/tests/cases/fourslash/moveToFile_existingImports4.ts new file mode 100644 index 00000000000..f69ddfc5429 --- /dev/null +++ b/tests/cases/fourslash/moveToFile_existingImports4.ts @@ -0,0 +1,30 @@ +/// + +// @allowJs: true +// @module: commonjs +// @filename: /common.js +////export const x = 1; + +// @filename: /a.js +////const { x } = require("./common"); +////module.exports.a = x; +////[|module.exports.b = x;|] + +// @filename: /b.js +////const { x } = require("./common"); +////module.exports.a = x; + +verify.moveToFile({ + newFileContents: { + "/a.js": +`const { x } = require("./common"); +module.exports.a = x; +`, + "/b.js": +`const { x } = require("./common"); +module.exports.a = x; +module.exports.b = x; +`, + }, + interactiveRefactorArguments: { targetFile: "/b.js" }, +});