Skip to content

Commit 43bf039

Browse files
author
Andy
authored
Add refactor to convert namespace to named imports and back (#24469)
* Add refactor to convert namespace to named imports and back * Add tests and comments * Code review * Handle shorthand property assignment and re-export * Don't use forEachFreeIdentifier * Fix rename after "."
1 parent 7737167 commit 43bf039

23 files changed

+302
-57
lines changed

‎src/compiler/diagnosticMessages.json

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4293,5 +4293,13 @@
42934293
"Convert '{0}' to mapped object type": {
42944294
"category": "Message",
42954295
"code": 95055
4296+
},
4297+
"Convert namespace import to named imports": {
4298+
"category": "Message",
4299+
"code": 95056
4300+
},
4301+
"Convert named imports to namespace import": {
4302+
"category": "Message",
4303+
"code": 95057
42964304
}
42974305
}

‎src/compiler/utilities.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,11 +223,11 @@ namespace ts {
223223
}
224224

225225
/**
226-
* Returns a value indicating whether a name is unique globally or within the current file
226+
* Returns a value indicating whether a name is unique globally or within the current file.
227+
* Note: This does not consider whether a name appears as a free identifier or not, so at the expression `x.y` this includes both `x` and `y`.
227228
*/
228-
export function isFileLevelUniqueName(currentSourceFile: SourceFile, name: string, hasGlobalName?: PrintHandlers["hasGlobalName"]): boolean {
229-
return !(hasGlobalName && hasGlobalName(name))
230-
&& !currentSourceFile.identifiers.has(name);
229+
export function isFileLevelUniqueName(sourceFile: SourceFile, name: string, hasGlobalName?: PrintHandlers["hasGlobalName"]): boolean {
230+
return !(hasGlobalName && hasGlobalName(name)) && !sourceFile.identifiers.has(name);
231231
}
232232

233233
// Returns true if this node is missing from the actual source code. A 'missing' node is different

‎src/harness/tsconfig.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@
120120
"../services/codefixes/useDefaultImport.ts",
121121
"../services/codefixes/fixAddModuleReferTypeMissingTypeof.ts",
122122
"../services/codefixes/convertToMappedObjectType.ts",
123+
"../services/refactors/convertImport.ts",
123124
"../services/refactors/extractSymbol.ts",
124125
"../services/refactors/generateGetAccessorAndSetAccessor.ts",
125126
"../services/refactors/moveToNewFile.ts",

‎src/server/tsconfig.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@
116116
"../services/codefixes/useDefaultImport.ts",
117117
"../services/codefixes/fixAddModuleReferTypeMissingTypeof.ts",
118118
"../services/codefixes/convertToMappedObjectType.ts",
119+
"../services/refactors/convertImport.ts",
119120
"../services/refactors/extractSymbol.ts",
120121
"../services/refactors/generateGetAccessorAndSetAccessor.ts",
121122
"../services/refactors/moveToNewFile.ts",

‎src/server/tsconfig.library.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@
122122
"../services/codefixes/useDefaultImport.ts",
123123
"../services/codefixes/fixAddModuleReferTypeMissingTypeof.ts",
124124
"../services/codefixes/convertToMappedObjectType.ts",
125+
"../services/refactors/convertImport.ts",
125126
"../services/refactors/extractSymbol.ts",
126127
"../services/refactors/generateGetAccessorAndSetAccessor.ts",
127128
"../services/refactors/moveToNewFile.ts",

‎src/services/codefixes/convertToEs6Module.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -437,22 +437,28 @@ namespace ts.codefix {
437437
type FreeIdentifiers = ReadonlyMap<ReadonlyArray<Identifier>>;
438438
function collectFreeIdentifiers(file: SourceFile): FreeIdentifiers {
439439
const map = createMultiMap<Identifier>();
440-
file.forEachChild(function recur(node) {
441-
if (isIdentifier(node) && isFreeIdentifier(node)) {
442-
map.add(node.text, node);
443-
}
444-
node.forEachChild(recur);
445-
});
440+
forEachFreeIdentifier(file, id => map.add(id.text, id));
446441
return map;
447442
}
448443

444+
/**
445+
* A free identifier is an identifier that can be accessed through name lookup as a local variable.
446+
* In the expression `x.y`, `x` is a free identifier, but `y` is not.
447+
*/
448+
function forEachFreeIdentifier(node: Node, cb: (id: Identifier) => void): void {
449+
if (isIdentifier(node) && isFreeIdentifier(node)) cb(node);
450+
node.forEachChild(child => forEachFreeIdentifier(child, cb));
451+
}
452+
449453
function isFreeIdentifier(node: Identifier): boolean {
450454
const { parent } = node;
451455
switch (parent.kind) {
452456
case SyntaxKind.PropertyAccessExpression:
453457
return (parent as PropertyAccessExpression).name !== node;
454458
case SyntaxKind.BindingElement:
455459
return (parent as BindingElement).propertyName !== node;
460+
case SyntaxKind.ImportSpecifier:
461+
return (parent as ImportSpecifier).propertyName !== node;
456462
default:
457463
return true;
458464
}

‎src/services/findAllReferences.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -712,16 +712,23 @@ namespace ts.FindAllReferences.Core {
712712
}
713713

714714
/** Used as a quick check for whether a symbol is used at all in a file (besides its definition). */
715-
export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile) {
715+
export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile): boolean {
716+
return eachSymbolReferenceInFile(definition, checker, sourceFile, () => true) || false;
717+
}
718+
719+
export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T): T | undefined {
716720
const symbol = checker.getSymbolAtLocation(definition);
717-
if (!symbol) return true; // Be lenient with invalid code.
718-
return getPossibleSymbolReferenceNodes(sourceFile, symbol.name).some(token => {
719-
if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) return false;
720-
const referenceSymbol = checker.getSymbolAtLocation(token)!;
721-
return referenceSymbol === symbol
721+
if (!symbol) return undefined;
722+
for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name)) {
723+
if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) continue;
724+
const referenceSymbol: Symbol = checker.getSymbolAtLocation(token)!; // See GH#19955 for why the type annotation is necessary
725+
if (referenceSymbol === symbol
722726
|| checker.getShorthandAssignmentValueSymbol(token.parent) === symbol
723-
|| isExportSpecifier(token.parent) && getLocalSymbolForExportSpecifier(token, referenceSymbol, token.parent, checker) === symbol;
724-
});
727+
|| isExportSpecifier(token.parent) && getLocalSymbolForExportSpecifier(token, referenceSymbol, token.parent, checker) === symbol) {
728+
const res = cb(token);
729+
if (res) return res;
730+
}
731+
}
725732
}
726733

727734
function getPossibleSymbolReferenceNodes(sourceFile: SourceFile, symbolName: string, container: Node = sourceFile): ReadonlyArray<Node> {

‎src/services/refactors/convertImport.ts

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
/* @internal */
2+
namespace ts.refactor.generateGetAccessorAndSetAccessor {
3+
const refactorName = "Convert import";
4+
const actionNameNamespaceToNamed = "Convert namespace import to named imports";
5+
const actionNameNamedToNamespace = "Convert named imports to namespace import";
6+
registerRefactor(refactorName, {
7+
getAvailableActions(context): ApplicableRefactorInfo[] | undefined {
8+
const i = getImportToConvert(context);
9+
if (!i) return undefined;
10+
const description = i.kind === SyntaxKind.NamespaceImport ? Diagnostics.Convert_namespace_import_to_named_imports.message : Diagnostics.Convert_named_imports_to_namespace_import.message;
11+
const actionName = i.kind === SyntaxKind.NamespaceImport ? actionNameNamespaceToNamed : actionNameNamedToNamespace;
12+
return [{ name: refactorName, description, actions: [{ name: actionName, description }] }];
13+
},
14+
getEditsForAction(context, actionName): RefactorEditInfo {
15+
Debug.assert(actionName === actionNameNamespaceToNamed || actionName === actionNameNamedToNamespace);
16+
const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program, t, Debug.assertDefined(getImportToConvert(context))));
17+
return { edits, renameFilename: undefined, renameLocation: undefined };
18+
}
19+
});
20+
21+
// Can convert imports of the form `import * as m from "m";` or `import d, { x, y } from "m";`.
22+
function getImportToConvert(context: RefactorContext): NamedImportBindings | undefined {
23+
const { file } = context;
24+
const span = getRefactorContextSpan(context);
25+
const token = getTokenAtPosition(file, span.start, /*includeJsDocComment*/ false);
26+
const importDecl = getParentNodeInSpan(token, file, span);
27+
if (!importDecl || !isImportDeclaration(importDecl)) return undefined;
28+
const { importClause } = importDecl;
29+
return importClause && importClause.namedBindings;
30+
}
31+
32+
function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void {
33+
const checker = program.getTypeChecker();
34+
if (toConvert.kind === SyntaxKind.NamespaceImport) {
35+
doChangeNamespaceToNamed(sourceFile, checker, changes, toConvert, getAllowSyntheticDefaultImports(program.getCompilerOptions()));
36+
}
37+
else {
38+
doChangeNamedToNamespace(sourceFile, checker, changes, toConvert);
39+
}
40+
}
41+
42+
function doChangeNamespaceToNamed(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamespaceImport, allowSyntheticDefaultImports: boolean): void {
43+
let usedAsNamespaceOrDefault = false;
44+
45+
const nodesToReplace: PropertyAccessExpression[] = [];
46+
const conflictingNames = createMap<true>();
47+
48+
FindAllReferences.Core.eachSymbolReferenceInFile(toConvert.name, checker, sourceFile, id => {
49+
if (!isPropertyAccessExpression(id.parent)) {
50+
usedAsNamespaceOrDefault = true;
51+
}
52+
else {
53+
const parent = cast(id.parent, isPropertyAccessExpression);
54+
const exportName = parent.name.text;
55+
if (checker.resolveName(exportName, id, SymbolFlags.All, /*excludeGlobals*/ true)) {
56+
conflictingNames.set(exportName, true);
57+
}
58+
Debug.assert(parent.expression === id);
59+
nodesToReplace.push(parent);
60+
}
61+
});
62+
63+
// We may need to change `mod.x` to `_x` to avoid a name conflict.
64+
const exportNameToImportName = createMap<string>();
65+
66+
for (const propertyAccess of nodesToReplace) {
67+
const exportName = propertyAccess.name.text;
68+
let importName = exportNameToImportName.get(exportName);
69+
if (importName === undefined) {
70+
exportNameToImportName.set(exportName, importName = conflictingNames.has(exportName) ? getUniqueName(exportName, sourceFile) : exportName);
71+
}
72+
changes.replaceNode(sourceFile, propertyAccess, createIdentifier(importName));
73+
}
74+
75+
const importSpecifiers: ImportSpecifier[] = [];
76+
exportNameToImportName.forEach((name, propertyName) => {
77+
importSpecifiers.push(createImportSpecifier(name === propertyName ? undefined : createIdentifier(propertyName), createIdentifier(name)));
78+
});
79+
80+
const importDecl = toConvert.parent.parent;
81+
if (usedAsNamespaceOrDefault && !allowSyntheticDefaultImports) {
82+
// Need to leave the namespace import alone
83+
changes.insertNodeAfter(sourceFile, importDecl, updateImport(importDecl, /*defaultImportName*/ undefined, importSpecifiers));
84+
}
85+
else {
86+
changes.replaceNode(sourceFile, importDecl, updateImport(importDecl, usedAsNamespaceOrDefault ? createIdentifier(toConvert.name.text) : undefined, importSpecifiers));
87+
}
88+
}
89+
90+
function doChangeNamedToNamespace(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamedImports): void {
91+
const importDecl = toConvert.parent.parent;
92+
const { moduleSpecifier } = importDecl;
93+
94+
const preferredName = moduleSpecifier && isStringLiteral(moduleSpecifier) ? codefix.moduleSpecifierToValidIdentifier(moduleSpecifier.text, ScriptTarget.ESNext) : "module";
95+
const namespaceNameConflicts = toConvert.elements.some(element =>
96+
FindAllReferences.Core.eachSymbolReferenceInFile(element.name, checker, sourceFile, id =>
97+
!!checker.resolveName(preferredName, id, SymbolFlags.All, /*excludeGlobals*/ true)) || false);
98+
const namespaceImportName = namespaceNameConflicts ? getUniqueName(preferredName, sourceFile) : preferredName;
99+
100+
const neededNamedImports: ImportSpecifier[] = [];
101+
102+
for (const element of toConvert.elements) {
103+
const propertyName = (element.propertyName || element.name).text;
104+
FindAllReferences.Core.eachSymbolReferenceInFile(element.name, checker, sourceFile, id => {
105+
const access = createPropertyAccess(createIdentifier(namespaceImportName), propertyName);
106+
if (isShorthandPropertyAssignment(id.parent)) {
107+
changes.replaceNode(sourceFile, id.parent, createPropertyAssignment(id.text, access));
108+
}
109+
else if (isExportSpecifier(id.parent) && !id.parent.propertyName) {
110+
if (!neededNamedImports.some(n => n.name === element.name)) {
111+
neededNamedImports.push(createImportSpecifier(element.propertyName && createIdentifier(element.propertyName.text), createIdentifier(element.name.text)));
112+
}
113+
}
114+
else {
115+
changes.replaceNode(sourceFile, id, access);
116+
}
117+
});
118+
}
119+
120+
changes.replaceNode(sourceFile, toConvert, createNamespaceImport(createIdentifier(namespaceImportName)));
121+
if (neededNamedImports.length) {
122+
changes.insertNodeAfter(sourceFile, toConvert.parent.parent, updateImport(importDecl, /*defaultImportName*/ undefined, neededNamedImports));
123+
}
124+
}
125+
126+
function updateImport(old: ImportDeclaration, defaultImportName: Identifier | undefined, elements: ReadonlyArray<ImportSpecifier> | undefined): ImportDeclaration {
127+
return createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined,
128+
createImportClause(defaultImportName, elements && elements.length ? createNamedImports(elements) : undefined), old.moduleSpecifier);
129+
}
130+
}

‎src/services/refactors/extractSymbol.ts

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,7 @@ namespace ts.refactor.extractSymbol {
718718

719719
// Make a unique name for the extracted function
720720
const file = scope.getSourceFile();
721-
const functionNameText = getUniqueName(isClassLike(scope) ? "newMethod" : "newFunction", file.text);
721+
const functionNameText = getUniqueName(isClassLike(scope) ? "newMethod" : "newFunction", file);
722722
const isJS = isInJavaScriptFile(scope);
723723

724724
const functionName = createIdentifier(functionNameText);
@@ -1005,7 +1005,7 @@ namespace ts.refactor.extractSymbol {
10051005

10061006
// Make a unique name for the extracted variable
10071007
const file = scope.getSourceFile();
1008-
const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file.text);
1008+
const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file);
10091009
const isJS = isInJavaScriptFile(scope);
10101010

10111011
const variableType = isJS || !checker.isContextSensitive(node)
@@ -1744,23 +1744,6 @@ namespace ts.refactor.extractSymbol {
17441744
}
17451745
}
17461746

1747-
function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined {
1748-
if (!node) return undefined;
1749-
1750-
while (node.parent) {
1751-
if (isSourceFile(node.parent) || !spanContainsNode(span, node.parent, file)) {
1752-
return node;
1753-
}
1754-
1755-
node = node.parent;
1756-
}
1757-
}
1758-
1759-
function spanContainsNode(span: TextSpan, node: Node, file: SourceFile): boolean {
1760-
return textSpanContainsPosition(span, node.getStart(file)) &&
1761-
node.getEnd() <= textSpanEnd(span);
1762-
}
1763-
17641747
/**
17651748
* Computes whether or not a node represents an expression in a position where it could
17661749
* be extracted.

‎src/services/refactors/generateGetAccessorAndSetAccessor.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
129129

130130
const name = declaration.name.text;
131131
const startWithUnderscore = startsWithUnderscore(name);
132-
const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file.text), declaration.name);
133-
const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file.text) : name, declaration.name);
132+
const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file), declaration.name);
133+
const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file) : name, declaration.name);
134134
return {
135135
isStatic: hasStaticModifier(declaration),
136136
isReadonly: hasReadonlyModifier(declaration),

‎src/services/tsconfig.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@
113113
"codefixes/useDefaultImport.ts",
114114
"codefixes/fixAddModuleReferTypeMissingTypeof.ts",
115115
"codefixes/convertToMappedObjectType.ts",
116+
"refactors/convertImport.ts",
116117
"refactors/extractSymbol.ts",
117118
"refactors/generateGetAccessorAndSetAccessor.ts",
118119
"refactors/moveToNewFile.ts",

‎src/services/utilities.ts

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,6 +1345,23 @@ namespace ts {
13451345
return forEachEntry(this.map, pred) || false;
13461346
}
13471347
}
1348+
1349+
export function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined {
1350+
if (!node) return undefined;
1351+
1352+
while (node.parent) {
1353+
if (isSourceFile(node.parent) || !spanContainsNode(span, node.parent, file)) {
1354+
return node;
1355+
}
1356+
1357+
node = node.parent;
1358+
}
1359+
}
1360+
1361+
function spanContainsNode(span: TextSpan, node: Node, file: SourceFile): boolean {
1362+
return textSpanContainsPosition(span, node.getStart(file)) &&
1363+
node.getEnd() <= textSpanEnd(span);
1364+
}
13481365
}
13491366

13501367
// Display-part writer helpers
@@ -1646,9 +1663,9 @@ namespace ts {
16461663
}
16471664

16481665
/* @internal */
1649-
export function getUniqueName(baseName: string, fileText: string): string {
1666+
export function getUniqueName(baseName: string, sourceFile: SourceFile): string {
16501667
let nameText = baseName;
1651-
for (let i = 1; stringContains(fileText, nameText); i++) {
1668+
for (let i = 1; !isFileLevelUniqueName(sourceFile, nameText); i++) {
16521669
nameText = `${baseName}_${i}`;
16531670
}
16541671
return nameText;
@@ -1667,7 +1684,7 @@ namespace ts {
16671684
Debug.assert(fileName === renameFilename);
16681685
for (const change of textChanges) {
16691686
const { span, newText } = change;
1670-
const index = newText.indexOf(name);
1687+
const index = indexInTextChange(newText, name);
16711688
if (index !== -1) {
16721689
lastPos = span.start + delta + index;
16731690

@@ -1685,4 +1702,13 @@ namespace ts {
16851702
Debug.assert(lastPos >= 0);
16861703
return lastPos;
16871704
}
1705+
1706+
function indexInTextChange(change: string, name: string): number {
1707+
if (startsWith(change, name)) return 0;
1708+
// Add a " " to avoid references inside words
1709+
let idx = change.indexOf(" " + name);
1710+
if (idx === -1) idx = change.indexOf("." + name);
1711+
if (idx === -1) idx = change.indexOf('"' + name);
1712+
return idx === -1 ? -1 : idx + 1;
1713+
}
16881714
}

‎tests/cases/fourslash/extract-method-uniqueName.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/// <reference path='fourslash.ts' />
22

3-
////// newFunction
3+
////const newFunction = 0;
44
/////*start*/1 + 1/*end*/;
55

66
goTo.select('start', 'end')
@@ -9,7 +9,7 @@ edit.applyRefactor({
99
actionName: "function_scope_0",
1010
actionDescription: "Extract to function in global scope",
1111
newContent:
12-
`// newFunction
12+
`const newFunction = 0;
1313
/*RENAME*/newFunction_1();
1414
1515
function newFunction_1() {

0 commit comments

Comments
 (0)