Fix namespace name conflict detection in "Convert named imports to namespace import" action (microsoft#45019)

* fix namespace conflict detection

Author: gabritto

src/services/refactors/convertImport.ts

@@ -138,13 +138,36 @@ namespace ts.refactor {
         const importDecl = toConvert.parent.parent;
         const { moduleSpecifier } = importDecl;
 
+        const toConvertSymbols: Set<Symbol> = new Set();
+        toConvert.elements.forEach(namedImport => {
+            const symbol = checker.getSymbolAtLocation(namedImport.name);
+            if (symbol) {
+                toConvertSymbols.add(symbol);
+            }
+        });
         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);
+        function hasNamespaceNameConflict(namedImport: ImportSpecifier): boolean {
+            // We need to check if the preferred namespace name (`preferredName`) we'd like to use in the refactored code will present a name conflict.
+            // A name conflict means that, in a scope where we would like to use the preferred namespace name, there already exists a symbol with that name in that scope.
+            // We are going to use the namespace name in the scopes the named imports being refactored are referenced,
+            // so we look for conflicts by looking at every reference to those named imports.
+            return !!FindAllReferences.Core.eachSymbolReferenceInFile(namedImport.name, checker, sourceFile, id => {
+                const symbol = checker.resolveName(preferredName, id, SymbolFlags.All, /*excludeGlobals*/ true);
+                if (symbol) { // There already is a symbol with the same name as the preferred namespace name.
+                    if (toConvertSymbols.has(symbol)) { // `preferredName` resolves to a symbol for one of the named import references we are going to transform into namespace import references...
+                        return isExportSpecifier(id.parent); // ...but if this reference is an export specifier, it will not be transformed, so it is a conflict; otherwise, it will be renamed and is not a conflict.
+                    }
+                    return true; // `preferredName` resolves to any other symbol, which will be present in the refactored code and so poses a name conflict.
+                }
+                return false; // There is no symbol with the same name as the preferred namespace name, so no conflict.
+            });
+        }
+        const namespaceNameConflicts = toConvert.elements.some(hasNamespaceNameConflict);
         const namespaceImportName = namespaceNameConflicts ? getUniqueName(preferredName, sourceFile) : preferredName;
 
-        const neededNamedImports: ImportSpecifier[] = [];
+        // Imports that need to be kept as named imports in the refactored code, to avoid changing the semantics.
+        // More specifically, those are named imports that appear in named exports in the original code, e.g. `a` in `import { a } from "m"; export { a }`.
+        const neededNamedImports: Set<ImportSpecifier> = new Set();
 
         for (const element of toConvert.elements) {
             const propertyName = (element.propertyName || element.name).text;
@@ -153,10 +176,8 @@ namespace ts.refactor {
                 if (isShorthandPropertyAssignment(id.parent)) {
                     changes.replaceNode(sourceFile, id.parent, factory.createPropertyAssignment(id.text, access));
                 }
-                else if (isExportSpecifier(id.parent) && !id.parent.propertyName) {
-                    if (!neededNamedImports.some(n => n.name === element.name)) {
-                        neededNamedImports.push(factory.createImportSpecifier(element.propertyName && factory.createIdentifier(element.propertyName.text), factory.createIdentifier(element.name.text)));
-                    }
+                else if (isExportSpecifier(id.parent)) {
+                    neededNamedImports.add(element);
                 }
                 else {
                     changes.replaceNode(sourceFile, id, access);
@@ -165,8 +186,10 @@ namespace ts.refactor {
         }
 
         changes.replaceNode(sourceFile, toConvert, factory.createNamespaceImport(factory.createIdentifier(namespaceImportName)));
-        if (neededNamedImports.length) {
-            changes.insertNodeAfter(sourceFile, toConvert.parent.parent, updateImport(importDecl, /*defaultImportName*/ undefined, neededNamedImports));
+        if (neededNamedImports.size) {
+            const newNamedImports: ImportSpecifier[] = arrayFrom(neededNamedImports.values()).map(element =>
+                factory.createImportSpecifier(element.propertyName && factory.createIdentifier(element.propertyName.text), factory.createIdentifier(element.name.text)));
+            changes.insertNodeAfter(sourceFile, toConvert.parent.parent, updateImport(importDecl, /*defaultImportName*/ undefined, newNamedImports));
         }
     }
 

tests/cases/fourslash/refactorConvertImport_namedToNamespace1.ts

@@ -0,0 +1,19 @@
+/// <reference path='fourslash.ts' />
+
+/////*a*/import { a, b, foo } from "./foo";/*b*/
+////a;
+////b;
+////foo;
+
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert import",
+    actionName: "Convert named imports to namespace import",
+    actionDescription: "Convert named imports to namespace import",
+    newContent:
+`import * as foo from "./foo";
+foo.a;
+foo.b;
+foo.foo;`,
+});

tests/cases/fourslash/refactorConvertImport_namedToNamespace2.ts

@@ -0,0 +1,31 @@
+/// <reference path='fourslash.ts' />
+
+////import foo, /*a*/{ a, b, c as d }/*b*/ from "./foo";
+////a;
+////b;
+////d;
+////foo;
+////export default a;
+////export { b };
+////export { d };
+////export { foo };
+
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert import",
+    actionName: "Convert named imports to namespace import",
+    actionDescription: "Convert named imports to namespace import",
+    triggerReason: "invoked",
+    newContent:
+`import foo, * as foo_1 from "./foo";
+import { b, c as d } from "./foo";
+foo_1.a;
+foo_1.b;
+foo_1.c;
+foo;
+export default foo_1.a;
+export { b };
+export { d };
+export { foo };`,
+});

tests/cases/fourslash/refactorConvertImport_namedToNamespace3.ts

@@ -0,0 +1,30 @@
+/// <reference path='fourslash.ts' />
+
+/////*a*/import { a, b } from "./foo"/*b*/;
+////a;
+////b;
+////function newScope() {
+////    const foo = "foo";
+////    foo;
+////    return a;
+////}
+////newScope();
+
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert import",
+    actionName: "Convert named imports to namespace import",
+    actionDescription: "Convert named imports to namespace import",
+    triggerReason: "invoked",
+    newContent:
+`import * as foo_1 from "./foo";
+foo_1.a;
+foo_1.b;
+function newScope() {
+    const foo = "foo";
+    foo;
+    return foo_1.a;
+}
+newScope();`,
+});

tests/cases/fourslash/refactorConvertImport_namedToNamespace4.ts

@@ -0,0 +1,30 @@
+/// <reference path='fourslash.ts' />
+
+/////*a*/import { a, b, c as d } from "./foo"/*b*/;
+////a;
+////b;
+////d;
+////export default a;
+////export { b };
+////export { d };
+////export { d as e };
+////export { b as f };
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert import",
+    actionName: "Convert named imports to namespace import",
+    actionDescription: "Convert named imports to namespace import",
+    triggerReason: "invoked",
+    newContent:
+`import * as foo from "./foo";
+import { b, c as d } from "./foo";
+foo.a;
+foo.b;
+foo.c;
+export default foo.a;
+export { b };
+export { d };
+export { d as e };
+export { b as f };`,
+});

tests/cases/fourslash/refactorConvertImport_namedToNamespace5.ts

@@ -0,0 +1,24 @@
+/// <reference path='fourslash.ts' />
+
+/////*a*/import { a, b, foo } from "./foo";/*b*/
+////a;
+////b;
+////foo;
+////export { foo };
+////export { foo as bar };
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert import",
+    actionName: "Convert named imports to namespace import",
+    actionDescription: "Convert named imports to namespace import",
+    triggerReason: "invoked",
+    newContent:
+`import * as foo_1 from "./foo";
+import { foo } from "./foo";
+foo_1.a;
+foo_1.b;
+foo_1.foo;
+export { foo };
+export { foo as bar };`,
+});