Skip to content

Commit 61bef3e

Browse files
author
Andy Hanson
committed
Code review
1 parent bcdec37 commit 61bef3e

File tree

4 files changed

+54
-40
lines changed

4 files changed

+54
-40
lines changed

‎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. For that, use `forEachFreeIdentifier`.
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/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: 19 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
1313
},
1414
getEditsForAction(context, actionName): RefactorEditInfo {
1515
Debug.assert(actionName === actionNameNamespaceToNamed || actionName === actionNameNamedToNamespace);
16-
const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program.getTypeChecker(), t, Debug.assertDefined(getImportToConvert(context))));
16+
const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program, t, Debug.assertDefined(getImportToConvert(context))));
1717
return { edits, renameFilename: undefined, renameLocation: undefined };
1818
}
1919
});
@@ -29,30 +29,27 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
2929
return importClause && importClause.namedBindings;
3030
}
3131

32-
function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void {
32+
function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void {
3333
const usedIdentifiers = createMap<true>();
3434
forEachFreeIdentifier(sourceFile, id => usedIdentifiers.set(id.text, true));
35+
const checker = program.getTypeChecker();
3536

3637
if (toConvert.kind === SyntaxKind.NamespaceImport) {
37-
doChangeNamespaceToNamed(sourceFile, checker, changes, toConvert, usedIdentifiers);
38+
doChangeNamespaceToNamed(sourceFile, checker, changes, toConvert, usedIdentifiers, getAllowSyntheticDefaultImports(program.getCompilerOptions()));
3839
}
3940
else {
4041
doChangeNamedToNamespace(sourceFile, checker, changes, toConvert, usedIdentifiers);
4142
}
4243
}
4344

44-
function doChangeNamespaceToNamed(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamespaceImport, usedIdentifiers: ReadonlyMap<true>): void {
45-
const namespaceSymbol = checker.getSymbolAtLocation(toConvert.name)!;
46-
45+
function doChangeNamespaceToNamed(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamespaceImport, usedIdentifiers: ReadonlyMap<true>, allowSyntheticDefaultImports: boolean): void {
4746
// We may need to change `mod.x` to `_x` to avoid a name conflict.
4847
const exportNameToImportName = createMap<string>();
49-
let usedAsNamespace = false;
50-
51-
forEachFreeIdentifier(sourceFile, id => {
52-
if (id === toConvert.name || checker.getSymbolAtLocation(id) !== namespaceSymbol) return;
48+
let usedAsNamespaceOrDefault = false;
5349

50+
FindAllReferences.Core.eachSymbolReferenceInFile(toConvert.name, checker, sourceFile, id => {
5451
if (!isPropertyAccessExpression(id.parent)) {
55-
usedAsNamespace = true;
52+
usedAsNamespaceOrDefault = true;
5653
}
5754
else {
5855
const parent = cast(id.parent, isPropertyAccessExpression);
@@ -70,14 +67,16 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
7067
exportNameToImportName.forEach((name, propertyName) => {
7168
elements.push(createImportSpecifier(name === propertyName ? undefined : createIdentifier(propertyName), createIdentifier(name)));
7269
});
73-
const namedImports = createNamedImports(elements);
70+
const makeImportDeclaration = (defaultImportName: Identifier | undefined) =>
71+
createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined,
72+
createImportClause(defaultImportName, elements.length ? createNamedImports(elements) : undefined),
73+
toConvert.parent.parent.moduleSpecifier);
7474

75-
if (usedAsNamespace) {
76-
changes.insertNodeAfter(sourceFile, toConvert.parent.parent,
77-
createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(/*name*/ undefined, namedImports), toConvert.parent.parent.moduleSpecifier));
75+
if (usedAsNamespaceOrDefault && !allowSyntheticDefaultImports) {
76+
changes.insertNodeAfter(sourceFile, toConvert.parent.parent, makeImportDeclaration(/*defaultImportName*/ undefined));
7877
}
7978
else {
80-
changes.replaceNode(sourceFile, toConvert, namedImports);
79+
changes.replaceNode(sourceFile, toConvert.parent.parent, makeImportDeclaration(usedAsNamespaceOrDefault ? createIdentifier(toConvert.name.text) : undefined));
8180
}
8281
}
8382

@@ -88,20 +87,12 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
8887

8988
changes.replaceNode(sourceFile, toConvert, createNamespaceImport(createIdentifier(namespaceImportName)));
9089

91-
const nameToPropertyName = createMap<string>();
9290
for (const element of toConvert.elements) {
93-
if (element.propertyName) {
94-
nameToPropertyName.set(element.name.text, element.propertyName.text);
95-
}
91+
const propertyName = (element.propertyName || element.name).text;
92+
FindAllReferences.Core.eachSymbolReferenceInFile(element.name, checker, sourceFile, id => {
93+
changes.replaceNode(sourceFile, id, createPropertyAccess(createIdentifier(namespaceImportName), propertyName));
94+
});
9695
}
97-
98-
const importedSymbols = toConvert.elements.map(e => checker.getSymbolAtLocation(e.name)!);
99-
100-
forEachFreeIdentifier(sourceFile, id => {
101-
if (!toConvert.elements.some(e => e.name === id) && contains(importedSymbols, checker.getSymbolAtLocation(id))) {
102-
changes.replaceNode(sourceFile, id, createPropertyAccess(createIdentifier(namespaceImportName), nameToPropertyName.get(id.text) || id.text));
103-
}
104-
});
10596
}
10697

10798
function generateName(name: string, usedIdentifiers: ReadonlyMap<true>): string {

‎tests/cases/fourslash/refactorConvertImport_useDefault.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowSyntheticDefaultImports: true
4+
5+
/////*a*/import * as m from "m";/*b*/
6+
////m();
7+
8+
goTo.select("a", "b");
9+
edit.applyRefactor({
10+
refactorName: "Convert import",
11+
actionName: "Convert namespace import to named imports",
12+
actionDescription: "Convert namespace import to named imports",
13+
newContent:
14+
`import m from "m";
15+
m();`,
16+
});

0 commit comments

Comments
 (0)