Code fix should prefer import type (microsoft#54255)

Author: andrewbranch

src/compiler/diagnosticMessages.json

@@ -1188,14 +1188,6 @@
         "category": "Error",
         "code": 1371
     },
-    "Convert to type-only import": {
-        "category": "Message",
-        "code": 1373
-    },
-    "Convert all imports not used as a value to type-only imports": {
-        "category": "Message",
-        "code": 1374
-    },
     "'await' expressions are only allowed at the top level of a file when that file is a module, but this file has no imports or exports. Consider adding an empty 'export {}' to make this file a module.": {
         "category": "Error",
         "code": 1375
@@ -7616,6 +7608,18 @@
         "category": "Message",
         "code": 95179
     },
+    "Use 'import type'": {
+        "category": "Message",
+        "code": 95180
+    },
+    "Use 'type {0}'": {
+        "category": "Message",
+        "code": 95181
+    },
+    "Fix all with type-only imports": {
+        "category": "Message",
+        "code": 95182
+    },
 
     "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
         "category": "Error",

src/services/codefixes/convertToTypeOnlyImport.ts

@@ -1,6 +1,7 @@
 import {
     Diagnostics,
     factory,
+    FindAllReferences,
     getSynthesizedDeepClone,
     getSynthesizedDeepClones,
     getTokenAtPosition,
@@ -9,12 +10,18 @@ import {
     ImportSpecifier,
     isImportDeclaration,
     isImportSpecifier,
+    isValidTypeOnlyAliasUseSite,
+    Program,
+    sameMap,
+    some,
     SourceFile,
+    SyntaxKind,
     textChanges,
 } from "../_namespaces/ts";
 import {
     codeFixAll,
     createCodeFixAction,
+    createCodeFixActionWithoutFixAll,
     registerCodeFix,
 } from "../_namespaces/ts.codefix";
 
@@ -30,16 +37,46 @@ registerCodeFix({
         const declaration = getDeclaration(context.sourceFile, context.span.start);
         if (declaration) {
             const changes = textChanges.ChangeTracker.with(context, t => doChange(t, context.sourceFile, declaration));
-            return [createCodeFixAction(fixId, changes, Diagnostics.Convert_to_type_only_import, fixId, Diagnostics.Convert_all_imports_not_used_as_a_value_to_type_only_imports)];
+            const importDeclarationChanges = declaration.kind === SyntaxKind.ImportSpecifier && canConvertImportDeclarationForSpecifier(declaration, context.sourceFile, context.program)
+                ? textChanges.ChangeTracker.with(context, t => doChange(t, context.sourceFile, declaration.parent.parent.parent))
+                : undefined;
+            const mainAction = createCodeFixAction(
+                fixId,
+                changes,
+                declaration.kind === SyntaxKind.ImportSpecifier
+                    ? [Diagnostics.Use_type_0, declaration.propertyName?.text ?? declaration.name.text]
+                    : Diagnostics.Use_import_type,
+                fixId,
+                Diagnostics.Fix_all_with_type_only_imports);
+
+            if (some(importDeclarationChanges)) {
+                return [
+                    createCodeFixActionWithoutFixAll(fixId, importDeclarationChanges, Diagnostics.Use_import_type),
+                    mainAction
+                ];
+            }
+            return [mainAction];
         }
         return undefined;
     },
     fixIds: [fixId],
     getAllCodeActions: function getAllCodeActionsToConvertToTypeOnlyImport(context) {
+        const fixedImportDeclarations: Set<ImportDeclaration> = new Set();
         return codeFixAll(context, errorCodes, (changes, diag) => {
-            const declaration = getDeclaration(diag.file, diag.start);
-            if (declaration) {
-                doChange(changes, diag.file, declaration);
+            const errorDeclaration = getDeclaration(diag.file, diag.start);
+            if (errorDeclaration?.kind === SyntaxKind.ImportDeclaration && !fixedImportDeclarations.has(errorDeclaration)) {
+                doChange(changes, diag.file, errorDeclaration);
+                fixedImportDeclarations.add(errorDeclaration);
+            }
+            else if (errorDeclaration?.kind === SyntaxKind.ImportSpecifier
+                && !fixedImportDeclarations.has(errorDeclaration.parent.parent.parent)
+                && canConvertImportDeclarationForSpecifier(errorDeclaration, diag.file, context.program)
+            ) {
+                doChange(changes, diag.file, errorDeclaration.parent.parent.parent);
+                fixedImportDeclarations.add(errorDeclaration.parent.parent.parent);
+            }
+            else if (errorDeclaration?.kind === SyntaxKind.ImportSpecifier) {
+                doChange(changes, diag.file, errorDeclaration);
             }
         });
     }
@@ -50,6 +87,31 @@ function getDeclaration(sourceFile: SourceFile, pos: number) {
     return isImportSpecifier(parent) || isImportDeclaration(parent) && parent.importClause ? parent : undefined;
 }
 
+
+function canConvertImportDeclarationForSpecifier(specifier: ImportSpecifier, sourceFile: SourceFile, program: Program): boolean {
+    if (specifier.parent.parent.name) {
+        // An import declaration with a default import and named bindings can't be type-only
+        return false;
+    }
+    const nonTypeOnlySpecifiers = specifier.parent.elements.filter(e => !e.isTypeOnly);
+    if (nonTypeOnlySpecifiers.length === 1) {
+        // If the error specifier is on the only non-type-only specifier, we can convert the whole import
+        return true;
+    }
+    // Otherwise, we need to check the usage of the other specifiers
+    const checker = program.getTypeChecker();
+    for (const specifier of nonTypeOnlySpecifiers) {
+        const isUsedAsValue = FindAllReferences.Core.eachSymbolReferenceInFile(specifier.name, checker, sourceFile, usage => {
+            return !isValidTypeOnlyAliasUseSite(usage);
+        });
+        if (isUsedAsValue) {
+            return false;
+        }
+    }
+    // No other specifiers are used as values, so we can convert the whole import
+    return true;
+}
+
 function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: ImportDeclaration | ImportSpecifier) {
     if (isImportSpecifier(declaration)) {
         changes.replaceNode(sourceFile, declaration, factory.updateImportSpecifier(declaration, /*isTypeOnly*/ true, declaration.propertyName, declaration.name));
@@ -73,8 +135,13 @@ function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, de
             ]);
         }
         else {
+            const newNamedBindings = importClause.namedBindings?.kind === SyntaxKind.NamedImports
+                ? factory.updateNamedImports(
+                    importClause.namedBindings,
+                    sameMap(importClause.namedBindings.elements, e => factory.updateImportSpecifier(e, /*isTypeOnly*/ false, e.propertyName, e.name)))
+                : importClause.namedBindings;
             const importDeclaration = factory.updateImportDeclaration(declaration, declaration.modifiers,
-                factory.updateImportClause(importClause, /*isTypeOnly*/ true, importClause.name, importClause.namedBindings), declaration.moduleSpecifier, declaration.assertClause);
+                factory.updateImportClause(importClause, /*isTypeOnly*/ true, importClause.name, newNamedBindings), declaration.moduleSpecifier, declaration.assertClause);
             changes.replaceNode(sourceFile, declaration, importDeclaration);
         }
     }

src/services/codefixes/fixUnreferenceableDecoratorMetadata.ts

@@ -44,7 +44,7 @@ registerCodeFix({
             actions = append(actions, createCodeFixActionWithoutFixAll(fixId, namespaceChanges, Diagnostics.Convert_named_imports_to_namespace_import));
         }
         if (typeOnlyChanges.length) {
-            actions = append(actions, createCodeFixActionWithoutFixAll(fixId, typeOnlyChanges, Diagnostics.Convert_to_type_only_import));
+            actions = append(actions, createCodeFixActionWithoutFixAll(fixId, typeOnlyChanges, Diagnostics.Use_import_type));
         }
         return actions;
     },

tests/cases/fourslash/codeFixConvertToTypeOnlyImport1.ts

@@ -20,7 +20,7 @@
 goTo.file("imports.ts");
 verify.codeFix({
     index: 0,
-    description: ts.Diagnostics.Convert_to_type_only_import.message,
+    description: ts.Diagnostics.Use_import_type.message,
     newFileContent: `import type {
     B,
     C,

tests/cases/fourslash/codeFixConvertToTypeOnlyImport2.ts

@@ -18,7 +18,7 @@
 goTo.file("imports.ts");
 verify.codeFix({
     index: 0,
-    description: ts.Diagnostics.Convert_to_type_only_import.message,
+    description: ts.Diagnostics.Use_import_type.message,
     newFileContent: `import type A from './exports';
 import type { B, C } from './exports';
 

tests/cases/fourslash/codeFixConvertToTypeOnlyImport3.ts

@@ -26,7 +26,7 @@
 goTo.file("imports.ts");
 verify.codeFixAll({
     fixId: "convertToTypeOnlyImport",
-    fixAllDescription: ts.Diagnostics.Convert_all_imports_not_used_as_a_value_to_type_only_imports.message,
+    fixAllDescription: ts.Diagnostics.Fix_all_with_type_only_imports.message,
     newFileContent: `import type A from './exports1';
 import type { B, C } from './exports1';
 import type D from "./exports2";

tests/cases/fourslash/codeFixConvertToTypeOnlyImport4.ts

@@ -12,6 +12,11 @@
 goTo.file("/a.ts");
 verify.codeFix({
     index: 0,
-    description: ts.Diagnostics.Convert_to_type_only_import.message,
+    description: ts.Diagnostics.Use_import_type.message,
+    newFileContent: `import type { I, foo } from "./b";`
+});
+verify.codeFix({
+    index: 1,
+    description: `Use 'type I'`,
     newFileContent: `import { type I, foo } from "./b";`
 });

tests/cases/fourslash/codeFixConvertToTypeOnlyImport5.ts

@@ -0,0 +1,24 @@
+/// <reference path="fourslash.ts" />
+
+// @module: esnext
+// @verbatimModuleSyntax: true
+// @filename: /b.ts
+////export interface I {}
+////export const foo = {};
+
+// @filename: /a.ts
+////import { I, foo } from "./b";
+////foo;
+////export declare const i: I;
+
+// ^ usage prevents 'Remove unused declaration' fix,
+//   so that lack of `index` option asserts only one fix is available.
+//   Specifically, we ensure no `Use 'import type'` fix is offered.
+
+goTo.file("/a.ts");
+verify.codeFix({
+    description: `Use 'type I'`,
+    newFileContent: `import { type I, foo } from "./b";
+foo;
+export declare const i: I;`,
+});

tests/cases/fourslash/codeFixConvertToTypeOnlyImport6.ts

@@ -0,0 +1,26 @@
+/// <reference path="fourslash.ts" />
+
+// @module: esnext
+// @verbatimModuleSyntax: true
+// @filename: /b.ts
+////export interface I {}
+////export const foo = {};
+
+// @filename: /a.ts
+//// import { I, type foo } from "./b";
+//// export declare const x: typeof foo;
+
+goTo.file("/a.ts");
+verify.codeFix({
+    index: 0,
+    description: ts.Diagnostics.Use_import_type.message,
+    newFileContent: `import type { I, foo } from "./b";
+export declare const x: typeof foo;`,
+});
+
+verify.codeFix({
+    index: 1,
+    description: `Use 'type I'`,
+    newFileContent: `import { type I, type foo } from "./b";
+export declare const x: typeof foo;`,
+});

tests/cases/fourslash/codeFixConvertToTypeOnlyImport7.ts

@@ -0,0 +1,34 @@
+/// <reference path="fourslash.ts" />
+
+// @module: esnext
+// @verbatimModuleSyntax: true
+
+// @Filename: /b.ts
+////export interface I {}
+////export const foo = {};
+
+// @Filename: /c.ts
+//// export interface C {}
+
+// @Filename: /d.ts
+//// export interface D {}
+//// export function d() {}
+
+// @Filename: /a.ts
+//// import { I, type foo } from "./b";
+//// import { C } from "./c";
+//// import { D, d } from "./d";
+//// export declare const x: typeof foo;
+//// d();
+
+goTo.file("/a.ts");
+verify.codeFixAll({
+  fixId: "convertToTypeOnlyImport",
+  fixAllDescription: ts.Diagnostics.Fix_all_with_type_only_imports.message,
+  newFileContent:
+`import type { I, foo } from "./b";
+import type { C } from "./c";
+import { type D, d } from "./d";
+export declare const x: typeof foo;
+d();`,
+});