Skip to content

Commit bf78d17

Browse files
authored
fix(55237): TypeScript 5.2 "move to file" results in duplicate imports (#56187)
1 parent f0374ce commit bf78d17

File tree

5 files changed

+155
-6
lines changed

5 files changed

+155
-6
lines changed

src/services/refactors/moveToFile.ts

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ import {
6868
Identifier,
6969
ImportDeclaration,
7070
ImportEqualsDeclaration,
71+
importFromModuleSpecifier,
7172
insertImports,
7273
InterfaceDeclaration,
7374
InternalSymbolName,
@@ -84,6 +85,8 @@ import {
8485
isImportDeclaration,
8586
isImportEqualsDeclaration,
8687
isNamedExports,
88+
isNamedImports,
89+
isObjectBindingPattern,
8790
isObjectLiteralExpression,
8891
isOmittedExpression,
8992
isPrologueDirective,
@@ -96,6 +99,7 @@ import {
9699
isStringLiteralLike,
97100
isValidTypeOnlyAliasUseSite,
98101
isVariableDeclaration,
102+
isVariableDeclarationInitializedToRequire,
99103
isVariableDeclarationList,
100104
isVariableStatement,
101105
LanguageServiceHost,
@@ -194,16 +198,15 @@ function error(notApplicableReason: string) {
194198

195199
function doChange(context: RefactorContext, oldFile: SourceFile, targetFile: string, program: Program, toMove: ToMove, changes: textChanges.ChangeTracker, host: LanguageServiceHost, preferences: UserPreferences): void {
196200
const checker = program.getTypeChecker();
197-
const usage = getUsageInfo(oldFile, toMove.all, checker);
198201
// For a new file
199202
if (!host.fileExists(targetFile)) {
200-
changes.createNewFile(oldFile, targetFile, getNewStatementsAndRemoveFromOldFile(oldFile, targetFile, usage, changes, toMove, program, host, preferences));
203+
changes.createNewFile(oldFile, targetFile, getNewStatementsAndRemoveFromOldFile(oldFile, targetFile, getUsageInfo(oldFile, toMove.all, checker), changes, toMove, program, host, preferences));
201204
addNewFileToTsconfig(program, changes, oldFile.fileName, targetFile, hostGetCanonicalFileName(host));
202205
}
203206
else {
204207
const targetSourceFile = Debug.checkDefined(program.getSourceFile(targetFile));
205208
const importAdder = codefix.createImportAdder(targetSourceFile, context.program, context.preferences, context.host);
206-
getNewStatementsAndRemoveFromOldFile(oldFile, targetSourceFile, usage, changes, toMove, program, host, preferences, importAdder);
209+
getNewStatementsAndRemoveFromOldFile(oldFile, targetSourceFile, getUsageInfo(oldFile, toMove.all, checker, getExistingImports(targetSourceFile, checker)), changes, toMove, program, host, preferences, importAdder);
207210
}
208211
}
209212

@@ -993,7 +996,7 @@ function isPureImport(node: Node): boolean {
993996
}
994997

995998
/** @internal */
996-
export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], checker: TypeChecker): UsageInfo {
999+
export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[], checker: TypeChecker, existingTargetImports: ReadonlySet<Symbol> = new Set()): UsageInfo {
9971000
const movedSymbols = new Set<Symbol>();
9981001
const oldImportsNeededByTargetFile = new Map<Symbol, /*isValidTypeOnlyUseSite*/ boolean>();
9991002
const targetFileImportsFromOldFile = new Set<Symbol>();
@@ -1010,9 +1013,17 @@ export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[],
10101013
movedSymbols.add(Debug.checkDefined(isExpressionStatement(decl) ? checker.getSymbolAtLocation(decl.expression.left) : decl.symbol, "Need a symbol here"));
10111014
});
10121015
}
1016+
1017+
const unusedImportsFromOldFile = new Set<Symbol>();
10131018
for (const statement of toMove) {
10141019
forEachReference(statement, checker, (symbol, isValidTypeOnlyUseSite) => {
1015-
if (!symbol.declarations) return;
1020+
if (!symbol.declarations) {
1021+
return;
1022+
}
1023+
if (existingTargetImports.has(skipAlias(symbol, checker))) {
1024+
unusedImportsFromOldFile.add(symbol);
1025+
return;
1026+
}
10161027
for (const decl of symbol.declarations) {
10171028
if (isInImport(decl)) {
10181029
const prevIsTypeOnly = oldImportsNeededByTargetFile.get(symbol);
@@ -1024,7 +1035,10 @@ export function getUsageInfo(oldFile: SourceFile, toMove: readonly Statement[],
10241035
}
10251036
});
10261037
}
1027-
const unusedImportsFromOldFile = new Set(oldImportsNeededByTargetFile.keys());
1038+
1039+
for (const unusedImport of oldImportsNeededByTargetFile.keys()) {
1040+
unusedImportsFromOldFile.add(unusedImport);
1041+
}
10281042

10291043
const oldFileImportsFromTargetFile = new Set<Symbol>();
10301044
for (const statement of oldFile.statements) {
@@ -1228,3 +1242,30 @@ function getOverloadRangeToMove(sourceFile: SourceFile, statement: Statement) {
12281242
}
12291243
return undefined;
12301244
}
1245+
1246+
function getExistingImports(sourceFile: SourceFile, checker: TypeChecker) {
1247+
const imports = new Set<Symbol>();
1248+
for (const moduleSpecifier of sourceFile.imports) {
1249+
const declaration = importFromModuleSpecifier(moduleSpecifier);
1250+
if (
1251+
isImportDeclaration(declaration) && declaration.importClause &&
1252+
declaration.importClause.namedBindings && isNamedImports(declaration.importClause.namedBindings)
1253+
) {
1254+
for (const e of declaration.importClause.namedBindings.elements) {
1255+
const symbol = checker.getSymbolAtLocation(e.propertyName || e.name);
1256+
if (symbol) {
1257+
imports.add(skipAlias(symbol, checker));
1258+
}
1259+
}
1260+
}
1261+
if (isVariableDeclarationInitializedToRequire(declaration.parent) && isObjectBindingPattern(declaration.parent.name)) {
1262+
for (const e of declaration.parent.name.elements) {
1263+
const symbol = checker.getSymbolAtLocation(e.propertyName || e.name);
1264+
if (symbol) {
1265+
imports.add(skipAlias(symbol, checker));
1266+
}
1267+
}
1268+
}
1269+
}
1270+
return imports;
1271+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @filename: /common.ts
4+
////export const x = 1;
5+
6+
// @filename: /a.ts
7+
////import { x } from "./common";
8+
////[|export const bar = x;|]
9+
10+
// @filename: /b.ts
11+
////import { x } from "./common";
12+
////export const foo = x;
13+
14+
verify.moveToFile({
15+
newFileContents: {
16+
"/a.ts": "",
17+
"/b.ts":
18+
`import { x } from "./common";
19+
export const foo = x;
20+
export const bar = x;
21+
`,
22+
},
23+
interactiveRefactorArguments: { targetFile: "/b.ts" },
24+
});
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @filename: /common.ts
4+
////export const x = 1;
5+
6+
// @filename: /a.ts
7+
////import { x } from "./common";
8+
////export const a = x;
9+
////[|export const b = x;|]
10+
11+
// @filename: /b.ts
12+
////import { x } from "./common";
13+
////export const a = x;
14+
15+
verify.moveToFile({
16+
newFileContents: {
17+
"/a.ts":
18+
`import { x } from "./common";
19+
export const a = x;
20+
`,
21+
"/b.ts":
22+
`import { x } from "./common";
23+
export const a = x;
24+
export const b = x;
25+
`,
26+
},
27+
interactiveRefactorArguments: { targetFile: "/b.ts" },
28+
});
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @module: commonjs
5+
// @filename: /common.js
6+
////export const x = 1;
7+
8+
// @filename: /a.js
9+
////const { x } = require("./common");
10+
////[|module.exports.b = x;|]
11+
12+
// @filename: /b.js
13+
////const { x } = require("./common");
14+
////module.exports.a = x;
15+
16+
verify.moveToFile({
17+
newFileContents: {
18+
"/a.js": "",
19+
"/b.js":
20+
`const { x } = require("./common");
21+
module.exports.a = x;
22+
module.exports.b = x;
23+
`,
24+
},
25+
interactiveRefactorArguments: { targetFile: "/b.js" },
26+
});
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @module: commonjs
5+
// @filename: /common.js
6+
////export const x = 1;
7+
8+
// @filename: /a.js
9+
////const { x } = require("./common");
10+
////module.exports.a = x;
11+
////[|module.exports.b = x;|]
12+
13+
// @filename: /b.js
14+
////const { x } = require("./common");
15+
////module.exports.a = x;
16+
17+
verify.moveToFile({
18+
newFileContents: {
19+
"/a.js":
20+
`const { x } = require("./common");
21+
module.exports.a = x;
22+
`,
23+
"/b.js":
24+
`const { x } = require("./common");
25+
module.exports.a = x;
26+
module.exports.b = x;
27+
`,
28+
},
29+
interactiveRefactorArguments: { targetFile: "/b.js" },
30+
});

0 commit comments

Comments
 (0)