Merge pull request #29451 from amcasey/GH27937

Consider JSX namespace imports when moving statements between files

Author: amcasey

src/services/refactors/moveToNewFile.ts

@@ -460,6 +460,12 @@ namespace ts.refactor {
         const oldImportsNeededByNewFile = new SymbolSet();
         const newFileImportsFromOldFile = new SymbolSet();
 
+        const containsJsx = find(toMove, statement => !!(statement.transformFlags & TransformFlags.ContainsJsx));
+        const jsxNamespaceSymbol = getJsxNamespaceSymbol(containsJsx);
+        if (jsxNamespaceSymbol) { // Might not exist (e.g. in non-compiling code)
+            oldImportsNeededByNewFile.add(jsxNamespaceSymbol);
+        }
+
         for (const statement of toMove) {
             forEachTopLevelDeclaration(statement, decl => {
                 movedSymbols.add(Debug.assertDefined(isExpressionStatement(decl) ? checker.getSymbolAtLocation(decl.expression.left) : decl.symbol));
@@ -485,13 +491,35 @@ namespace ts.refactor {
         for (const statement of oldFile.statements) {
             if (contains(toMove, statement)) continue;
 
+            // jsxNamespaceSymbol will only be set iff it is in oldImportsNeededByNewFile.
+            if (jsxNamespaceSymbol && !!(statement.transformFlags & TransformFlags.ContainsJsx)) {
+                unusedImportsFromOldFile.delete(jsxNamespaceSymbol);
+            }
+
             forEachReference(statement, checker, symbol => {
                 if (movedSymbols.has(symbol)) oldFileImportsFromNewFile.add(symbol);
                 unusedImportsFromOldFile.delete(symbol);
             });
         }
 
         return { movedSymbols, newFileImportsFromOldFile, oldFileImportsFromNewFile, oldImportsNeededByNewFile, unusedImportsFromOldFile };
+
+        function getJsxNamespaceSymbol(containsJsx: Node | undefined) {
+            if (containsJsx === undefined) {
+                return undefined;
+            }
+
+            const jsxNamespace = checker.getJsxNamespace(containsJsx);
+
+            // Strictly speaking, this could resolve to a symbol other than the JSX namespace.
+            // This will produce erroneous output (probably, an incorrectly copied import) but
+            // is expected to be very rare and easily reversible.
+            const jsxNamespaceSymbol = checker.resolveName(jsxNamespace, containsJsx, SymbolFlags.Namespace, /*excludeGlobals*/ true);
+
+            return !!jsxNamespaceSymbol && some(jsxNamespaceSymbol.declarations, isInImport)
+                ? jsxNamespaceSymbol
+                : undefined;
+        }
     }
 
     // Below should all be utilities
@@ -512,7 +540,7 @@ namespace ts.refactor {
     }
     function isVariableDeclarationInImport(decl: VariableDeclaration) {
         return isSourceFile(decl.parent.parent.parent) &&
-            decl.initializer && isRequireCall(decl.initializer, /*checkArgumentIsStringLiteralLike*/ true);
+            !!decl.initializer && isRequireCall(decl.initializer, /*checkArgumentIsStringLiteralLike*/ true);
     }
 
     function filterImport(i: SupportedImport, moduleSpecifier: StringLiteralLike, keep: (name: Identifier) => boolean): SupportedImportStatement | undefined {

tests/cases/fourslash/moveToNewFile_moveJsxImport1.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @noLib: true
+// @libFiles: react.d.ts,lib.d.ts
+
+// @Filename: file.tsx
+//// import React = require('react');
+//// [|<div/>;|]
+//// 1;
+
+verify.moveToNewFile({
+    newFileContents: {
+        "/tests/cases/fourslash/file.tsx":
+`1;`,
+        "/tests/cases/fourslash/newFile.tsx":
+`import React = require('react');
+<div />;
+`,
+    }
+});

tests/cases/fourslash/moveToNewFile_moveJsxImport2.ts

@@ -0,0 +1,22 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @noLib: true
+// @libFiles: react.d.ts,lib.d.ts
+
+// @Filename: file.tsx
+//// import React = require('react');
+//// [|<div/>;|]
+//// <div/>;
+
+verify.moveToNewFile({
+    newFileContents: {
+        "/tests/cases/fourslash/file.tsx":
+`import React = require('react');
+<div/>;`,
+        "/tests/cases/fourslash/newFile.tsx":
+`import React = require('react');
+<div />;
+`,
+    }
+});

tests/cases/fourslash/moveToNewFile_moveJsxImport3.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @noLib: true
+// @libFiles: react.d.ts,lib.d.ts
+
+// @Filename: file.tsx
+//// import React = require('react');
+//// [|1;|]
+//// <div/>;
+
+verify.moveToNewFile({
+    newFileContents: {
+        "/tests/cases/fourslash/file.tsx":
+`import React = require('react');
+<div/>;`,
+        "/tests/cases/fourslash/newFile.tsx":
+`1;
+`,
+    }
+});

tests/cases/fourslash/moveToNewFile_moveJsxImport4.ts

@@ -0,0 +1,29 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @noLib: true
+// @libFiles: react.d.ts,lib.d.ts,leftpad.d.ts
+
+// @Filename: file.tsx
+//// import React = require('leftpad');
+//// [|function F() {
+////     const React = import("react");
+////     <div/>;
+//// }|]
+//// React;
+
+verify.moveToNewFile({
+    newFileContents: {
+        "/tests/cases/fourslash/file.tsx":
+`import React = require('leftpad');
+React;`,
+        // NB: A perfect implementation would not copy over the import
+        "/tests/cases/fourslash/F.tsx":
+`import React = require('leftpad');
+function F() {
+    const React = import("react");
+    <div />;
+}
+`,
+    }
+});