Don't use forEachFreeIdentifier

Author: Andy Hanson

src/compiler/utilities.ts

@@ -224,7 +224,7 @@ namespace ts {
 
     /**
      * Returns a value indicating whether a name is unique globally or within the current file.
-     * Note: This does not consider whether a name appears as a free identifier or not. For that, use `forEachFreeIdentifier`.
+     * 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`.
      */
     export function isFileLevelUniqueName(sourceFile: SourceFile, name: string, hasGlobalName?: PrintHandlers["hasGlobalName"]): boolean {
         return !(hasGlobalName && hasGlobalName(name)) && !sourceFile.identifiers.has(name);

src/services/codefixes/convertToEs6Module.ts

@@ -441,6 +441,29 @@ namespace ts.codefix {
         return map;
     }
 
+    /**
+     * A free identifier is an identifier that can be accessed through name lookup as a local variable.
+     * In the expression `x.y`, `x` is a free identifier, but `y` is not.
+     */
+    function forEachFreeIdentifier(node: Node, cb: (id: Identifier) => void): void {
+        if (isIdentifier(node) && isFreeIdentifier(node)) cb(node);
+        node.forEachChild(child => forEachFreeIdentifier(child, cb));
+    }
+
+    function isFreeIdentifier(node: Identifier): boolean {
+        const { parent } = node;
+        switch (parent.kind) {
+            case SyntaxKind.PropertyAccessExpression:
+                return (parent as PropertyAccessExpression).name !== node;
+            case SyntaxKind.BindingElement:
+                return (parent as BindingElement).propertyName !== node;
+            case SyntaxKind.ImportSpecifier:
+                return (parent as ImportSpecifier).propertyName !== node;
+            default:
+                return true;
+        }
+    }
+
     // Node helpers
 
     function functionExpressionToDeclaration(name: string | undefined, additionalModifiers: ReadonlyArray<Modifier>, fn: FunctionExpression | ArrowFunction | MethodDeclaration): FunctionDeclaration {

src/services/refactors/convertImport.ts

@@ -30,58 +30,72 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
     }
 
     function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void {
-        const usedIdentifiers = createMap<true>();
-        forEachFreeIdentifier(sourceFile, id => usedIdentifiers.set(id.text, true));
         const checker = program.getTypeChecker();
-
         if (toConvert.kind === SyntaxKind.NamespaceImport) {
-            doChangeNamespaceToNamed(sourceFile, checker, changes, toConvert, usedIdentifiers, getAllowSyntheticDefaultImports(program.getCompilerOptions()));
+            doChangeNamespaceToNamed(sourceFile, checker, changes, toConvert, getAllowSyntheticDefaultImports(program.getCompilerOptions()));
         }
         else {
-            doChangeNamedToNamespace(sourceFile, checker, changes, toConvert, usedIdentifiers);
+            doChangeNamedToNamespace(sourceFile, checker, changes, toConvert);
         }
     }
 
-    function doChangeNamespaceToNamed(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamespaceImport, usedIdentifiers: ReadonlyMap<true>, allowSyntheticDefaultImports: boolean): void {
-        // We may need to change `mod.x` to `_x` to avoid a name conflict.
-        const exportNameToImportName = createMap<string>();
+    function doChangeNamespaceToNamed(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamespaceImport, allowSyntheticDefaultImports: boolean): void {
         let usedAsNamespaceOrDefault = false;
 
+        const nodesToReplace: PropertyAccessExpression[] = [];
+        const conflictingNames = createMap<true>();
+
         FindAllReferences.Core.eachSymbolReferenceInFile(toConvert.name, checker, sourceFile, id => {
             if (!isPropertyAccessExpression(id.parent)) {
                 usedAsNamespaceOrDefault = true;
             }
             else {
                 const parent = cast(id.parent, isPropertyAccessExpression);
                 const exportName = parent.name.text;
-                let name = exportNameToImportName.get(exportName);
-                if (name === undefined) {
-                    exportNameToImportName.set(exportName, name = generateName(exportName, usedIdentifiers));
+                if (checker.resolveName(exportName, id, SymbolFlags.All, /*excludeGlobals*/ true)) {
+                    conflictingNames.set(exportName, true);
                 }
                 Debug.assert(parent.expression === id);
-                changes.replaceNode(sourceFile, parent, createIdentifier(name));
+                nodesToReplace.push(parent);
             }
         });
 
-        const elements: ImportSpecifier[] = [];
+        // We may need to change `mod.x` to `_x` to avoid a name conflict.
+        const exportNameToImportName = createMap<string>();
+
+        for (const propertyAccess of nodesToReplace) {
+            const exportName = propertyAccess.name.text;
+            let importName = exportNameToImportName.get(exportName);
+            if (importName === undefined) {
+                exportNameToImportName.set(exportName, importName = conflictingNames.has(exportName) ? getUniqueName(exportName, sourceFile) : exportName);
+            }
+            changes.replaceNode(sourceFile, propertyAccess, createIdentifier(importName));
+        }
+
+        const importSpecifiers: ImportSpecifier[] = [];
         exportNameToImportName.forEach((name, propertyName) => {
-            elements.push(createImportSpecifier(name === propertyName ? undefined : createIdentifier(propertyName), createIdentifier(name)));
+            importSpecifiers.push(createImportSpecifier(name === propertyName ? undefined : createIdentifier(propertyName), createIdentifier(name)));
         });
 
         const importDecl = toConvert.parent.parent;
         if (usedAsNamespaceOrDefault && !allowSyntheticDefaultImports) {
             // Need to leave the namespace import alone
-            changes.insertNodeAfter(sourceFile, importDecl, updateImport(importDecl, /*defaultImportName*/ undefined, elements));
+            changes.insertNodeAfter(sourceFile, importDecl, updateImport(importDecl, /*defaultImportName*/ undefined, importSpecifiers));
         }
         else {
-            changes.replaceNode(sourceFile, importDecl, updateImport(importDecl, usedAsNamespaceOrDefault ? createIdentifier(toConvert.name.text) : undefined, elements));
+            changes.replaceNode(sourceFile, importDecl, updateImport(importDecl, usedAsNamespaceOrDefault ? createIdentifier(toConvert.name.text) : undefined, importSpecifiers));
         }
     }
 
-    function doChangeNamedToNamespace(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamedImports, usedIdentifiers: ReadonlyMap<true>): void {
-        const { moduleSpecifier } = toConvert.parent.parent;
-        // We know the user is using at least ScriptTarget.ES6, and moduleSpecifierToValidIdentifier only cares if we're using ES5+, so just set ScriptTarget.ESNext
-        const namespaceImportName = generateName(moduleSpecifier && isStringLiteral(moduleSpecifier) ? codefix.moduleSpecifierToValidIdentifier(moduleSpecifier.text, ScriptTarget.ESNext) : "module", usedIdentifiers);
+    function doChangeNamedToNamespace(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamedImports): void {
+        const importDecl = toConvert.parent.parent;
+        const { moduleSpecifier } = importDecl;
+
+        const preferredName = moduleSpecifier && isStringLiteral(moduleSpecifier) ? codefix.moduleSpecifierToValidIdentifier(moduleSpecifier.text, ScriptTarget.ESNext) : "module";
+        const namespaceNameConflicts = toConvert.elements.some(element =>
+            FindAllReferences.Core.eachSymbolReferenceInFile(element.name, checker, sourceFile, id =>
+                !!checker.resolveName(preferredName, id, SymbolFlags.All, /*excludeGlobals*/ true)) || false);
+        const namespaceImportName = namespaceNameConflicts ? getUniqueName(preferredName, sourceFile) : preferredName;
 
         const neededNamedImports: ImportSpecifier[] = [];
 
@@ -105,7 +119,6 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
 
         changes.replaceNode(sourceFile, toConvert, createNamespaceImport(createIdentifier(namespaceImportName)));
         if (neededNamedImports.length) {
-            const importDecl = toConvert.parent.parent;
             changes.insertNodeAfter(sourceFile, toConvert.parent.parent, updateImport(importDecl, /*defaultImportName*/ undefined, neededNamedImports));
         }
     }
@@ -114,11 +127,4 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
         return createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined,
             createImportClause(defaultImportName, elements && elements.length ? createNamedImports(elements) : undefined), old.moduleSpecifier);
     }
-
-    function generateName(name: string, usedIdentifiers: ReadonlyMap<true>): string {
-        while (usedIdentifiers.has(name)) {
-            name = `_${name}`;
-        }
-        return name;
-    }
 }

src/services/refactors/extractSymbol.ts

@@ -718,7 +718,7 @@ namespace ts.refactor.extractSymbol {
 
         // Make a unique name for the extracted function
         const file = scope.getSourceFile();
-        const functionNameText = getUniqueName(isClassLike(scope) ? "newMethod" : "newFunction", file.text);
+        const functionNameText = getUniqueName(isClassLike(scope) ? "newMethod" : "newFunction", file);
         const isJS = isInJavaScriptFile(scope);
 
         const functionName = createIdentifier(functionNameText);
@@ -1005,7 +1005,7 @@ namespace ts.refactor.extractSymbol {
 
         // Make a unique name for the extracted variable
         const file = scope.getSourceFile();
-        const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file.text);
+        const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file);
         const isJS = isInJavaScriptFile(scope);
 
         const variableType = isJS || !checker.isContextSensitive(node)

src/services/refactors/generateGetAccessorAndSetAccessor.ts

@@ -129,8 +129,8 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
 
         const name = declaration.name.text;
         const startWithUnderscore = startsWithUnderscore(name);
-        const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file.text), declaration.name);
-        const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file.text) : name, declaration.name);
+        const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file), declaration.name);
+        const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file) : name, declaration.name);
         return {
             isStatic: hasStaticModifier(declaration),
             isReadonly: hasReadonlyModifier(declaration),

src/services/utilities.ts

@@ -1326,29 +1326,6 @@ namespace ts {
         return textSpanContainsPosition(span, node.getStart(file)) &&
             node.getEnd() <= textSpanEnd(span);
     }
-
-    /**
-     * A free identifier is an identifier that can be accessed through name lookup as a local variable.
-     * In the expression `x.y`, `x` is a free identifier, but `y` is not.
-     */
-    export function forEachFreeIdentifier(node: Node, cb: (id: Identifier) => void): void {
-        if (isIdentifier(node) && isFreeIdentifier(node)) cb(node);
-        node.forEachChild(child => forEachFreeIdentifier(child, cb));
-    }
-
-    function isFreeIdentifier(node: Identifier): boolean {
-        const { parent } = node;
-        switch (parent.kind) {
-            case SyntaxKind.PropertyAccessExpression:
-                return (parent as PropertyAccessExpression).name !== node;
-            case SyntaxKind.BindingElement:
-                return (parent as BindingElement).propertyName !== node;
-            case SyntaxKind.ImportSpecifier:
-                return (parent as ImportSpecifier).propertyName !== node;
-            default:
-                return true;
-        }
-    }
 }
 
 // Display-part writer helpers
@@ -1650,9 +1627,9 @@ namespace ts {
     }
 
     /* @internal */
-    export function getUniqueName(baseName: string, fileText: string): string {
+    export function getUniqueName(baseName: string, sourceFile: SourceFile): string {
         let nameText = baseName;
-        for (let i = 1; stringContains(fileText, nameText); i++) {
+        for (let i = 1; !isFileLevelUniqueName(sourceFile, nameText); i++) {
             nameText = `${baseName}_${i}`;
         }
         return nameText;
@@ -1671,7 +1648,7 @@ namespace ts {
             Debug.assert(fileName === renameFilename);
             for (const change of textChanges) {
                 const { span, newText } = change;
-                const index = newText.indexOf(name);
+                const index = indexInTextChange(newText, name);
                 if (index !== -1) {
                     lastPos = span.start + delta + index;
 
@@ -1689,4 +1666,12 @@ namespace ts {
         Debug.assert(lastPos >= 0);
         return lastPos;
     }
+
+    function indexInTextChange(change: string, name: string): number {
+        if (startsWith(change, name)) return 0;
+        // Add a " " to avoid references inside words
+        let idx = change.indexOf(" " + name);
+        if (idx === -1) idx = change.indexOf('"' + name);
+        return idx === -1 ? -1 : idx + 1;
+    }
 }

tests/cases/fourslash/refactorConvertImport_namedToNamespace.ts

@@ -13,12 +13,12 @@ edit.applyRefactor({
     actionName: "Convert named imports to namespace import",
     actionDescription: "Convert named imports to namespace import",
     newContent:
-// TODO: GH#23781 (_m.y shouldn't be indented)
-`import * as _m from "m";
+// TODO: GH#23781 (m_1.y shouldn't be indented)
+`import * as m_1 from "m";
 import { x } from "m";
 const m = 0;
-const o = { x: _m.x };
+const o = { x: m_1.x };
 export { x }; // Need a named import for this
-    _m.y;
-const n: _m.T = 0;`,
+    m_1.y;
+const n: m_1.T = 0;`,
 });

tests/cases/fourslash/refactorConvertImport_namespaceToNamed.ts

@@ -11,8 +11,8 @@ edit.applyRefactor({
     actionName: "Convert namespace import to named imports",
     actionDescription: "Convert namespace import to named imports",
     newContent:
-`import { a as _a, b } from "m";
+`import { a as a_1, b } from "m";
 const a = 0;
-_a;
+a_1;
 b;`,
 });

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess23.ts

@@ -29,10 +29,10 @@ edit.applyRefactor({
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
     private _a: number = 1;
-    public get /*RENAME*/a_2(): number {
+    public get /*RENAME*/a_1(): number {
         return this._a;
     }
-    public set a_2(value: number) {
+    public set a_1(value: number) {
         this._a = value;
     }
     private _a_1: string = "foo";

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess4.ts

@@ -11,10 +11,10 @@ edit.applyRefactor({
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
     private _a: string;
-    public get /*RENAME*/a_1(): string {
+    public get /*RENAME*/a(): string {
         return this._a;
     }
-    public set a_1(value: string) {
+    public set a(value: string) {
         this._a = value;
     }
 }`,

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess5.ts

@@ -11,10 +11,10 @@ edit.applyRefactor({
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
     private _a: string;
-    public get /*RENAME*/a_1(): string {
+    public get /*RENAME*/a(): string {
         return this._a;
     }
-    public set a_1(value: string) {
+    public set a(value: string) {
         this._a = value;
     }
 }`,

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess6.ts

@@ -1,8 +1,8 @@
 /// <reference path='fourslash.ts' />
 
-//// class A {
-////     /*a*/public _a: string;/*b*/
-//// }
+////class A {
+////    /*a*/public _a: string;/*b*/
+////}
 
 goTo.select("a", "b");
 edit.applyRefactor({
@@ -11,10 +11,10 @@ edit.applyRefactor({
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
     private _a: string;
-    public get /*RENAME*/a_1(): string {
+    public get /*RENAME*/a(): string {
         return this._a;
     }
-    public set a_1(value: string) {
+    public set a(value: string) {
         this._a = value;
     }
 }`,

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess7.ts

@@ -11,10 +11,10 @@ edit.applyRefactor({
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
     private _a: string;
-    protected get /*RENAME*/a_1(): string {
+    protected get /*RENAME*/a(): string {
         return this._a;
     }
-    protected set a_1(value: string) {
+    protected set a(value: string) {
         this._a = value;
     }
 }`,