moveToNewFile: Update namespace imports (#24612) (#24657)

* moveToNewFile: Update namespace imports (#24612) * Port needed changes from #24469
microsoft · Jun 4, 2018 · 4e17e77 · 4e17e77
1 parent 3bf21be
commit 4e17e77
Show file tree

Hide file tree

Showing 12 changed files with 159 additions and 27 deletions.
diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts
@@ -710,16 +710,23 @@ namespace ts.FindAllReferences.Core {
     }
 
     /** Used as a quick check for whether a symbol is used at all in a file (besides its definition). */
-    export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile) {
+    export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile): boolean {
+        return eachSymbolReferenceInFile(definition, checker, sourceFile, () => true) || false;
+    }
+
+    export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T): T | undefined {
         const symbol = checker.getSymbolAtLocation(definition);
-        if (!symbol) return true; // Be lenient with invalid code.
-        return getPossibleSymbolReferenceNodes(sourceFile, symbol.name).some(token => {
-            if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) return false;
-            const referenceSymbol = checker.getSymbolAtLocation(token);
-            return referenceSymbol === symbol
+        if (!symbol) return undefined;
+        for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name)) {
+            if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) continue;
+            const referenceSymbol: Symbol = checker.getSymbolAtLocation(token)!; // See GH#19955 for why the type annotation is necessary
+            if (referenceSymbol === symbol
                 || checker.getShorthandAssignmentValueSymbol(token.parent) === symbol
-                || isExportSpecifier(token.parent) && getLocalSymbolForExportSpecifier(token, referenceSymbol, token.parent, checker) === symbol;
-        });
+                || isExportSpecifier(token.parent) && getLocalSymbolForExportSpecifier(token, referenceSymbol, token.parent, checker) === symbol) {
+                const res = cb(token);
+                if (res) return res;
+            }
+        }
     }
 
     function getPossibleSymbolReferenceNodes(sourceFile: SourceFile, symbolName: string, container: Node = sourceFile): ReadonlyArray<Node> {

diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
@@ -714,7 +714,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);
@@ -1001,7 +1001,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)

diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/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),

diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts
@@ -153,6 +153,8 @@ namespace ts.refactor {
             if (sourceFile === oldFile) continue;
             for (const statement of sourceFile.statements) {
                 forEachImportInStatement(statement, importNode => {
+                    if (checker.getSymbolAtLocation(moduleSpecifierFromImport(importNode)) !== oldFile.symbol) return;
+
                     const shouldMove = (name: Identifier): boolean => {
                         const symbol = isBindingElement(name.parent)
                             ? getPropertySymbolFromBindingElement(checker, name.parent as BindingElement & { name: Identifier })
@@ -163,11 +165,76 @@ namespace ts.refactor {
                     const newModuleSpecifier = combinePaths(getDirectoryPath(moduleSpecifierFromImport(importNode).text), newModuleName);
                     const newImportDeclaration = filterImport(importNode, createLiteral(newModuleSpecifier), shouldMove);
                     if (newImportDeclaration) changes.insertNodeAfter(sourceFile, statement, newImportDeclaration);
+
+                    const ns = getNamespaceLikeImport(importNode);
+                    if (ns) updateNamespaceLikeImport(changes, sourceFile, checker, movedSymbols, newModuleName, newModuleSpecifier, ns, importNode);
                 });
             }
         }
     }
 
+    function getNamespaceLikeImport(node: SupportedImport): Identifier | undefined {
+        switch (node.kind) {
+            case SyntaxKind.ImportDeclaration:
+                return node.importClause && node.importClause.namedBindings && node.importClause.namedBindings.kind === SyntaxKind.NamespaceImport ?
+                    node.importClause.namedBindings.name : undefined;
+            case SyntaxKind.ImportEqualsDeclaration:
+                return node.name;
+            case SyntaxKind.VariableDeclaration:
+                return tryCast(node.name, isIdentifier);
+            default:
+                return Debug.assertNever(node);
+        }
+    }
+
+    function updateNamespaceLikeImport(
+        changes: textChanges.ChangeTracker,
+        sourceFile: SourceFile,
+        checker: TypeChecker,
+        movedSymbols: ReadonlySymbolSet,
+        newModuleName: string,
+        newModuleSpecifier: string,
+        oldImportId: Identifier,
+        oldImportNode: SupportedImport,
+    ): void {
+        const preferredNewNamespaceName = codefix.moduleSpecifierToValidIdentifier(newModuleName, ScriptTarget.ESNext);
+        let needUniqueName = false;
+        const toChange: Identifier[] = [];
+        FindAllReferences.Core.eachSymbolReferenceInFile(oldImportId, checker, sourceFile, ref => {
+            if (!isPropertyAccessExpression(ref.parent)) return;
+            needUniqueName = needUniqueName || !!checker.resolveName(preferredNewNamespaceName, ref, SymbolFlags.All, /*excludeGlobals*/ true);
+            if (movedSymbols.has(checker.getSymbolAtLocation(ref.parent.name)!)) {
+                toChange.push(ref);
+            }
+        });
+
+        if (toChange.length) {
+            const newNamespaceName = needUniqueName ? getUniqueName(preferredNewNamespaceName, sourceFile) : preferredNewNamespaceName;
+            for (const ref of toChange) {
+                changes.replaceNode(sourceFile, ref, createIdentifier(newNamespaceName));
+            }
+            changes.insertNodeAfter(sourceFile, oldImportNode, updateNamespaceLikeImportNode(oldImportNode, newModuleName, newModuleSpecifier));
+        }
+    }
+
+    function updateNamespaceLikeImportNode(node: SupportedImport, newNamespaceName: string, newModuleSpecifier: string): Node {
+        const newNamespaceId = createIdentifier(newNamespaceName);
+        const newModuleString = createLiteral(newModuleSpecifier);
+        switch (node.kind) {
+            case SyntaxKind.ImportDeclaration:
+                return createImportDeclaration(
+                    /*decorators*/ undefined, /*modifiers*/ undefined,
+                    createImportClause(/*name*/ undefined, createNamespaceImport(newNamespaceId)),
+                    newModuleString);
+            case SyntaxKind.ImportEqualsDeclaration:
+                return createImportEqualsDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, newNamespaceId, createExternalModuleReference(newModuleString));
+            case SyntaxKind.VariableDeclaration:
+                return createVariableDeclaration(newNamespaceId, /*type*/ undefined, createRequireCall(newModuleString));
+            default:
+                return Debug.assertNever(node);
+        }
+    }
+
     function moduleSpecifierFromImport(i: SupportedImport): StringLiteralLike {
         return (i.kind === SyntaxKind.ImportDeclaration ? i.moduleSpecifier
             : i.kind === SyntaxKind.ImportEqualsDeclaration ? i.moduleReference.expression

diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -1596,9 +1596,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;
@@ -1617,7 +1617,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;
 
@@ -1635,4 +1635,13 @@ 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);
+        if (idx === -1) idx = change.indexOf('"' + name);
+        return idx === -1 ? -1 : idx + 1;
+    }
 }
diff --git a/tests/cases/fourslash/extract-method-uniqueName.ts b/tests/cases/fourslash/extract-method-uniqueName.ts
@@ -1,6 +1,6 @@
 /// <reference path='fourslash.ts' />
 
-////// newFunction
+////const newFunction = 0;
 /////*start*/1 + 1/*end*/;
 
 goTo.select('start', 'end')
@@ -9,7 +9,7 @@ edit.applyRefactor({
     actionName: "function_scope_0",
     actionDescription: "Extract to function in global scope",
     newContent:
-`// newFunction
+`const newFunction = 0;
 /*RENAME*/newFunction_1();
 
 function newFunction_1() {

diff --git a/tests/cases/fourslash/moveToNewFile_namespaceImport.ts b/tests/cases/fourslash/moveToNewFile_namespaceImport.ts
@@ -0,0 +1,49 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+
+// @Filename: /a.ts
+////[|export const x = 0;|]
+////export const y = 0;
+
+// @Filename: /b.ts
+////import * as a from "./a";
+////a.x;
+////a.y;
+
+// @Filename: /c.ts
+////import a = require("./a");
+////a.x;
+////a.y;
+
+// @Filename: /d.js
+////const a = require("./a");
+////a.x;
+////a.y;
+
+verify.moveToNewFile({
+    newFileContents: {
+        "/a.ts":
+`export const y = 0;`,
+
+        "/x.ts":
+`export const x = 0;`,
+
+        "/b.ts":
+`import * as a from "./a";
+import * as x from "./x";
+x.x;
+a.y;`,
+
+        "/c.ts":
+`import a = require("./a");
+import x = require("./x");
+x.x;
+a.y;`,
+
+        "/d.js":
+`const a = require("./a"), x = require("./x");
+x.x;
+a.y;`,
+    },
+});
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess23.ts 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";

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess4.ts b/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;
     }
 }`,

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess5.ts b/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;
     }
 }`,

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess6.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess6.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;
     }
 }`,

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess7.ts b/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;
     }
 }`,