Fix organizeImports with type-only imports (#36807)

Author: andrewbranch

src/services/organizeImports.ts

@@ -174,115 +174,134 @@ namespace ts.OrganizeImports {
             return importGroup;
         }
 
-        const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getCategorizedImports(importGroup);
+        const { importWithoutClause, typeOnlyImports, regularImports } = getCategorizedImports(importGroup);
 
         const coalescedImports: ImportDeclaration[] = [];
 
         if (importWithoutClause) {
             coalescedImports.push(importWithoutClause);
         }
 
-        // Normally, we don't combine default and namespace imports, but it would be silly to
-        // produce two import declarations in this special case.
-        if (defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
-            // Add the namespace import to the existing default ImportDeclaration.
-            const defaultImport = defaultImports[0];
-            coalescedImports.push(
-                updateImportDeclarationAndClause(defaultImport, defaultImport.importClause!.name, namespaceImports[0].importClause!.namedBindings)); // TODO: GH#18217
+        for (const group of [regularImports, typeOnlyImports]) {
+            const isTypeOnly = group === typeOnlyImports;
+            const { defaultImports, namespaceImports, namedImports } = group;
+            // Normally, we don't combine default and namespace imports, but it would be silly to
+            // produce two import declarations in this special case.
+            if (!isTypeOnly && defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
+                // Add the namespace import to the existing default ImportDeclaration.
+                const defaultImport = defaultImports[0];
+                coalescedImports.push(
+                    updateImportDeclarationAndClause(defaultImport, defaultImport.importClause!.name, namespaceImports[0].importClause!.namedBindings)); // TODO: GH#18217
 
-            return coalescedImports;
-        }
+                continue;
+            }
 
-        const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) =>
-            compareIdentifiers((i1.importClause!.namedBindings as NamespaceImport).name, (i2.importClause!.namedBindings as NamespaceImport).name)); // TODO: GH#18217
+            const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) =>
+                compareIdentifiers((i1.importClause!.namedBindings as NamespaceImport).name, (i2.importClause!.namedBindings as NamespaceImport).name)); // TODO: GH#18217
 
-        for (const namespaceImport of sortedNamespaceImports) {
-            // Drop the name, if any
-            coalescedImports.push(
-                updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause!.namedBindings)); // TODO: GH#18217
-        }
+            for (const namespaceImport of sortedNamespaceImports) {
+                // Drop the name, if any
+                coalescedImports.push(
+                    updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause!.namedBindings)); // TODO: GH#18217
+            }
 
-        if (defaultImports.length === 0 && namedImports.length === 0) {
-            return coalescedImports;
-        }
+            if (defaultImports.length === 0 && namedImports.length === 0) {
+                continue;
+            }
 
-        let newDefaultImport: Identifier | undefined;
-        const newImportSpecifiers: ImportSpecifier[] = [];
-        if (defaultImports.length === 1) {
-            newDefaultImport = defaultImports[0].importClause!.name;
-        }
-        else {
-            for (const defaultImport of defaultImports) {
-                newImportSpecifiers.push(
-                    createImportSpecifier(createIdentifier("default"), defaultImport.importClause!.name!)); // TODO: GH#18217
+            let newDefaultImport: Identifier | undefined;
+            const newImportSpecifiers: ImportSpecifier[] = [];
+            if (defaultImports.length === 1) {
+                newDefaultImport = defaultImports[0].importClause!.name;
+            }
+            else {
+                for (const defaultImport of defaultImports) {
+                    newImportSpecifiers.push(
+                        createImportSpecifier(createIdentifier("default"), defaultImport.importClause!.name!)); // TODO: GH#18217
+                }
             }
-        }
 
-        newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause!.namedBindings as NamedImports).elements)); // TODO: GH#18217
+            newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause!.namedBindings as NamedImports).elements)); // TODO: GH#18217
 
-        const sortedImportSpecifiers = sortSpecifiers(newImportSpecifiers);
+            const sortedImportSpecifiers = sortSpecifiers(newImportSpecifiers);
 
-        const importDecl = defaultImports.length > 0
-            ? defaultImports[0]
-            : namedImports[0];
+            const importDecl = defaultImports.length > 0
+                ? defaultImports[0]
+                : namedImports[0];
 
-        const newNamedImports = sortedImportSpecifiers.length === 0
-            ? newDefaultImport
-                ? undefined
-                : createNamedImports(emptyArray)
-            : namedImports.length === 0
-                ? createNamedImports(sortedImportSpecifiers)
-                : updateNamedImports(namedImports[0].importClause!.namedBindings as NamedImports, sortedImportSpecifiers); // TODO: GH#18217
+            const newNamedImports = sortedImportSpecifiers.length === 0
+                ? newDefaultImport
+                    ? undefined
+                    : createNamedImports(emptyArray)
+                : namedImports.length === 0
+                    ? createNamedImports(sortedImportSpecifiers)
+                    : updateNamedImports(namedImports[0].importClause!.namedBindings as NamedImports, sortedImportSpecifiers); // TODO: GH#18217
 
-        coalescedImports.push(
-            updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports));
+            // Type-only imports are not allowed to combine
+            if (isTypeOnly && newDefaultImport && newNamedImports) {
+                coalescedImports.push(
+                    updateImportDeclarationAndClause(importDecl, newDefaultImport, /*namedBindings*/ undefined));
+                coalescedImports.push(
+                    updateImportDeclarationAndClause(namedImports[0] ?? importDecl, /*name*/ undefined, newNamedImports));
+            }
+            else {
+                coalescedImports.push(
+                    updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports));
+            }
+        }
 
         return coalescedImports;
 
-        /*
-         * Returns entire import declarations because they may already have been rewritten and
-         * may lack parent pointers.  The desired parts can easily be recovered based on the
-         * categorization.
-         *
-         * NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`.
-         */
-        function getCategorizedImports(importGroup: readonly ImportDeclaration[]) {
-            let importWithoutClause: ImportDeclaration | undefined;
-            const defaultImports: ImportDeclaration[] = [];
-            const namespaceImports: ImportDeclaration[] = [];
-            const namedImports: ImportDeclaration[] = [];
-
-            for (const importDeclaration of importGroup) {
-                if (importDeclaration.importClause === undefined) {
-                    // Only the first such import is interesting - the others are redundant.
-                    // Note: Unfortunately, we will lose trivia that was on this node.
-                    importWithoutClause = importWithoutClause || importDeclaration;
-                    continue;
-                }
+    }
 
-                const { name, namedBindings } = importDeclaration.importClause;
+    interface ImportGroup {
+        defaultImports: ImportDeclaration[];
+        namespaceImports: ImportDeclaration[];
+        namedImports: ImportDeclaration[];
+    }
 
-                if (name) {
-                    defaultImports.push(importDeclaration);
-                }
+    /*
+     * Returns entire import declarations because they may already have been rewritten and
+     * may lack parent pointers.  The desired parts can easily be recovered based on the
+     * categorization.
+     *
+     * NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`.
+     */
+    function getCategorizedImports(importGroup: readonly ImportDeclaration[]) {
+        let importWithoutClause: ImportDeclaration | undefined;
+        const typeOnlyImports: ImportGroup = { defaultImports: [], namespaceImports: [], namedImports: [] };
+        const regularImports: ImportGroup = { defaultImports: [], namespaceImports: [], namedImports: [] };
+
+        for (const importDeclaration of importGroup) {
+            if (importDeclaration.importClause === undefined) {
+                // Only the first such import is interesting - the others are redundant.
+                // Note: Unfortunately, we will lose trivia that was on this node.
+                importWithoutClause = importWithoutClause || importDeclaration;
+                continue;
+            }
 
-                if (namedBindings) {
-                    if (isNamespaceImport(namedBindings)) {
-                        namespaceImports.push(importDeclaration);
-                    }
-                    else {
-                        namedImports.push(importDeclaration);
-                    }
-                }
+            const group = importDeclaration.importClause.isTypeOnly ? typeOnlyImports : regularImports;
+            const { name, namedBindings } = importDeclaration.importClause;
+
+            if (name) {
+                group.defaultImports.push(importDeclaration);
             }
 
-            return {
-                importWithoutClause,
-                defaultImports,
-                namespaceImports,
-                namedImports,
-            };
+            if (namedBindings) {
+                if (isNamespaceImport(namedBindings)) {
+                    group.namespaceImports.push(importDeclaration);
+                }
+                else {
+                    group.namedImports.push(importDeclaration);
+                }
+            }
         }
+
+        return {
+            importWithoutClause,
+            typeOnlyImports,
+            regularImports,
+        };
     }
 
     // Internal for testing
@@ -294,37 +313,38 @@ namespace ts.OrganizeImports {
             return exportGroup;
         }
 
-        const { exportWithoutClause, namedExports } = getCategorizedExports(exportGroup);
+        const { exportWithoutClause, namedExports, typeOnlyExports } = getCategorizedExports(exportGroup);
 
         const coalescedExports: ExportDeclaration[] = [];
 
         if (exportWithoutClause) {
             coalescedExports.push(exportWithoutClause);
         }
 
-        if (namedExports.length === 0) {
-            return coalescedExports;
+        for (const exportGroup of [namedExports, typeOnlyExports]) {
+            if (exportGroup.length === 0) {
+                continue;
+            }
+            const newExportSpecifiers: ExportSpecifier[] = [];
+            newExportSpecifiers.push(...flatMap(exportGroup, i => i.exportClause && isNamedExports(i.exportClause) ? i.exportClause.elements : emptyArray));
+
+            const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers);
+
+            const exportDecl = exportGroup[0];
+            coalescedExports.push(
+                updateExportDeclaration(
+                    exportDecl,
+                    exportDecl.decorators,
+                    exportDecl.modifiers,
+                    exportDecl.exportClause && (
+                        isNamedExports(exportDecl.exportClause) ?
+                            updateNamedExports(exportDecl.exportClause, sortedExportSpecifiers) :
+                            updateNamespaceExport(exportDecl.exportClause, exportDecl.exportClause.name)
+                    ),
+                    exportDecl.moduleSpecifier,
+                    exportDecl.isTypeOnly));
         }
 
-        const newExportSpecifiers: ExportSpecifier[] = [];
-        newExportSpecifiers.push(...flatMap(namedExports, i => i.exportClause && isNamedExports(i.exportClause) ? i.exportClause.elements : emptyArray));
-
-        const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers);
-
-        const exportDecl = namedExports[0];
-        coalescedExports.push(
-            updateExportDeclaration(
-                exportDecl,
-                exportDecl.decorators,
-                exportDecl.modifiers,
-                exportDecl.exportClause && (
-                    isNamedExports(exportDecl.exportClause) ?
-                        updateNamedExports(exportDecl.exportClause, sortedExportSpecifiers) :
-                        updateNamespaceExport(exportDecl.exportClause, exportDecl.exportClause.name)
-                ),
-                exportDecl.moduleSpecifier,
-                exportDecl.isTypeOnly));
-
         return coalescedExports;
 
         /*
@@ -335,13 +355,17 @@ namespace ts.OrganizeImports {
         function getCategorizedExports(exportGroup: readonly ExportDeclaration[]) {
             let exportWithoutClause: ExportDeclaration | undefined;
             const namedExports: ExportDeclaration[] = [];
+            const typeOnlyExports: ExportDeclaration[] = [];
 
             for (const exportDeclaration of exportGroup) {
                 if (exportDeclaration.exportClause === undefined) {
                     // Only the first such export is interesting - the others are redundant.
                     // Note: Unfortunately, we will lose trivia that was on this node.
                     exportWithoutClause = exportWithoutClause || exportDeclaration;
                 }
+                else if (exportDeclaration.isTypeOnly) {
+                    typeOnlyExports.push(exportDeclaration);
+                }
                 else {
                     namedExports.push(exportDeclaration);
                 }
@@ -350,6 +374,7 @@ namespace ts.OrganizeImports {
             return {
                 exportWithoutClause,
                 namedExports,
+                typeOnlyExports,
             };
         }
     }

src/testRunner/unittests/services/organizeImports.ts

@@ -168,6 +168,28 @@ namespace ts {
                 const expectedCoalescedImports = sortedImports;
                 assertListEqual(actualCoalescedImports, expectedCoalescedImports);
             });
+
+            it("Combine type-only imports separately from other imports", () => {
+                const sortedImports = parseImports(
+                    `import type { x } from "lib";`,
+                    `import type { y } from "lib";`,
+                    `import { z } from "lib";`);
+                const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports);
+                const expectedCoalescedImports = parseImports(
+                    `import { z } from "lib";`,
+                    `import type { x, y } from "lib";`);
+                assertListEqual(actualCoalescedImports, expectedCoalescedImports);
+            });
+
+            it("Do not combine type-only default, namespace, or named imports with each other", () => {
+                const sortedImports = parseImports(
+                    `import type { x } from "lib";`,
+                    `import type * as y from "lib";`,
+                    `import type z from "lib";`);
+                const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports);
+                const expectedCoalescedImports = actualCoalescedImports;
+                assertListEqual(actualCoalescedImports, expectedCoalescedImports);
+            });
         });
 
         describe("Coalesce exports", () => {
@@ -240,6 +262,25 @@ namespace ts {
                     `export { x as a, y, z as b } from "lib";`);
                 assertListEqual(actualCoalescedExports, expectedCoalescedExports);
             });
+
+            it("Keep type-only exports separate", () => {
+                const sortedExports = parseExports(
+                    `export { x };`,
+                    `export type { y };`);
+                const actualCoalescedExports = OrganizeImports.coalesceExports(sortedExports);
+                const expectedCoalescedExports = sortedExports;
+                assertListEqual(actualCoalescedExports, expectedCoalescedExports);
+            });
+
+            it("Combine type-only exports", () => {
+                const sortedExports = parseExports(
+                    `export type { x };`,
+                    `export type { y };`);
+                const actualCoalescedExports = OrganizeImports.coalesceExports(sortedExports);
+                const expectedCoalescedExports = parseExports(
+                    `export type { x, y };`);
+                assertListEqual(actualCoalescedExports, expectedCoalescedExports);
+            });
         });
 
         describe("Baselines", () => {
@@ -425,6 +466,18 @@ D();
                 libFile);
             /* eslint-enable no-template-curly-in-string */
 
+            testOrganizeImports("TypeOnly",
+                {
+                    path: "/test.ts",
+                    content: `
+import { X } from "lib";
+import type Y from "lib";
+import { Z } from "lib";
+import type { A, B } from "lib";
+
+export { A, B, X, Y, Z };`
+                });
+
             testOrganizeImports("CoalesceMultipleModules",
                 {
                     path: "/test.ts",

tests/baselines/reference/organizeImports/TypeOnly.ts

@@ -0,0 +1,15 @@
+// ==ORIGINAL==
+
+import { X } from "lib";
+import type Y from "lib";
+import { Z } from "lib";
+import type { A, B } from "lib";
+
+export { A, B, X, Y, Z };
+// ==ORGANIZED==
+
+import { X, Z } from "lib";
+import type Y from "lib";
+import type { A, B } from "lib";
+
+export { A, B, X, Y, Z };