Codefix: Don’t return a fixId if there’s definitely nothing else that can be fixed (#35765)

* Start fixing fixId

* Fix tests

* Add comment

* Fix unit tests, remove fixAllDescription when unavailable

* Add codeFixAllAvailable to fourslash harness

src/harness/fourslashImpl.ts

@@ -2544,8 +2544,8 @@ namespace FourSlash {
 
         public verifyCodeFixAll({ fixId, fixAllDescription, newFileContent, commands: expectedCommands }: FourSlashInterface.VerifyCodeFixAllOptions): void {
             const fixWithId = ts.find(this.getCodeFixes(this.activeFile.fileName), a => a.fixId === fixId);
-            ts.Debug.assert(fixWithId !== undefined, "No available code fix has that group id.", () =>
-                `Expected '${fixId}'. Available action ids: ${ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => a.fixId)}`);
+            ts.Debug.assert(fixWithId !== undefined, "No available code fix has the expected id. Fix All is not available if there is only one potentially fixable diagnostic present.", () =>
+                `Expected '${fixId}'. Available actions:\n${ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => `${a.fixName} (${a.fixId || "no fix id"})`).join("\n")}`);
             ts.Debug.assertEqual(fixWithId!.fixAllDescription, fixAllDescription);
 
             const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings, ts.emptyOptions);
@@ -2681,7 +2681,7 @@ namespace FourSlash {
             }
             const range = ts.firstOrUndefined(ranges);
 
-            const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixId === ts.codefix.importFixId);
+            const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixName === ts.codefix.importFixName);
 
             if (codeFixes.length === 0) {
                 if (expectedTextArray.length !== 0) {
@@ -2717,7 +2717,7 @@ namespace FourSlash {
             const codeFixes = this.getCodeFixes(marker.fileName, ts.Diagnostics.Cannot_find_name_0.code, {
                 includeCompletionsForModuleExports: true,
                 includeCompletionsWithInsertText: true
-            }, marker.position).filter(f => f.fixId === ts.codefix.importFixId);
+            }, marker.position).filter(f => f.fixName === ts.codefix.importFixName);
 
             const actualModuleSpecifiers = ts.mapDefined(codeFixes, fix => {
                 return ts.forEach(ts.flatMap(fix.changes, c => c.textChanges), c => {
@@ -3044,6 +3044,26 @@ namespace FourSlash {
             }
         }
 
+        public verifyCodeFixAllAvailable(negative: boolean, fixName: string) {
+            const availableFixes = this.getCodeFixes(this.activeFile.fileName);
+            const hasFix = availableFixes.some(fix => fix.fixName === fixName && fix.fixId);
+            if (negative && hasFix) {
+                this.raiseError(`Expected not to find a fix with the name '${fixName}', but one exists.`);
+            }
+            else if (!negative && !hasFix) {
+                if (availableFixes.some(fix => fix.fixName === fixName)) {
+                    this.raiseError(`Found a fix with the name '${fixName}', but fix-all is not available.`);
+                }
+
+                this.raiseError(
+                    `Expected to find a fix with the name '${fixName}', but none exists.` +
+                    availableFixes.length
+                        ? ` Available fixes: ${availableFixes.map(fix => `${fix.fixName} (${fix.fixId ? "with" : "without"} fix-all)`).join(", ")}`
+                        : ""
+                );
+            }
+        }
+
         public verifyApplicableRefactorAvailableAtMarker(negative: boolean, markerName: string) {
             const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName)).length > 0;
             if (negative && isAvailable) {

src/harness/fourslashInterfaceImpl.ts

@@ -191,6 +191,10 @@ namespace FourSlashInterface {
             this.state.verifyCodeFixAvailable(this.negative, options);
         }
 
+        public codeFixAllAvailable(fixName: string) {
+            this.state.verifyCodeFixAllAvailable(this.negative, fixName);
+        }
+
         public applicableRefactorAvailableAtMarker(markerName: string) {
             this.state.verifyApplicableRefactorAvailableAtMarker(this.negative, markerName);
         }

src/services/codeFixProvider.ts

@@ -10,7 +10,7 @@ namespace ts.codefix {
             : getLocaleSpecificMessage(diag);
     }
 
-    export function createCodeFixActionNoFixId(fixName: string, changes: FileTextChanges[], description: DiagnosticAndArguments) {
+    export function createCodeFixActionWithoutFixAll(fixName: string, changes: FileTextChanges[], description: DiagnosticAndArguments) {
         return createCodeFixActionWorker(fixName, diagnosticToString(description), changes, /*fixId*/ undefined, /*fixAllDescription*/ undefined);
     }
 
@@ -38,8 +38,24 @@ namespace ts.codefix {
         return arrayFrom(errorCodeToFixes.keys());
     }
 
+    function removeFixIdIfFixAllUnavailable(registration: CodeFixRegistration, diagnostics: Diagnostic[]) {
+        const { errorCodes } = registration;
+        let maybeFixableDiagnostics = 0;
+        for (const diag of diagnostics) {
+            if (contains(errorCodes, diag.code)) maybeFixableDiagnostics++;
+            if (maybeFixableDiagnostics > 1) break;
+        }
+
+        const fixAllUnavailable = maybeFixableDiagnostics < 2;
+        return ({ fixId, fixAllDescription, ...action }: CodeFixAction): CodeFixAction => {
+            return fixAllUnavailable ? action : { ...action, fixId, fixAllDescription };
+        };
+    }
+
     export function getFixes(context: CodeFixContext): readonly CodeFixAction[] {
-        return flatMap(errorCodeToFixes.get(String(context.errorCode)) || emptyArray, f => f.getCodeActions(context));
+        const diagnostics = getDiagnostics(context);
+        const registrations = errorCodeToFixes.get(String(context.errorCode));
+        return flatMap(registrations, f => map(f.getCodeActions(context), removeFixIdIfFixAllUnavailable(f, diagnostics)));
     }
 
     export function getAllFixes(context: CodeFixAllContext): CombinedCodeActions {
@@ -65,11 +81,15 @@ namespace ts.codefix {
         return createCombinedCodeActions(changes, commands.length === 0 ? undefined : commands);
     }
 
-    export function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: readonly number[], cb: (diag: DiagnosticWithLocation) => void): void {
-        for (const diag of program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken))) {
+    export function eachDiagnostic(context: CodeFixAllContext, errorCodes: readonly number[], cb: (diag: DiagnosticWithLocation) => void): void {
+        for (const diag of getDiagnostics(context)) {
             if (contains(errorCodes, diag.code)) {
                 cb(diag as DiagnosticWithLocation);
             }
         }
     }
+
+    function getDiagnostics({ program, sourceFile, cancellationToken }: CodeFixContextBase) {
+        return program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken));
+    }
 }

src/services/codefixes/addMissingAwait.ts

@@ -68,7 +68,9 @@ namespace ts.codefix {
                     makeChange(t, errorCode, sourceFile, checker, expression, fixedDeclarations);
                 }
             });
-            return createCodeFixActionNoFixId(
+            // No fix-all because it will already be included once with the use site fix,
+            // and for simplicity the fix-all doesn‘t let the user choose between use-site and declaration-site fixes.
+            return createCodeFixActionWithoutFixAll(
                 "addMissingAwaitToInitializer",
                 initializerChanges,
                 awaitableInitializers.initializers.length === 1

src/services/codefixes/addMissingConst.ts

@@ -30,7 +30,7 @@ namespace ts.codefix {
         if (forInitializer) return applyChange(changeTracker, forInitializer, sourceFile, fixedNodes);
 
         const parent = token.parent;
-        if (isBinaryExpression(parent) && isExpressionStatement(parent.parent)) {
+        if (isBinaryExpression(parent) && parent.operatorToken.kind === SyntaxKind.EqualsToken && isExpressionStatement(parent.parent)) {
             return applyChange(changeTracker, token, sourceFile, fixedNodes);
         }
 
@@ -104,6 +104,8 @@ namespace ts.codefix {
             return every([expression.left, expression.right], expression => expressionCouldBeVariableDeclaration(expression, checker));
         }
 
-        return isIdentifier(expression.left) && !checker.getSymbolAtLocation(expression.left);
+        return expression.operatorToken.kind === SyntaxKind.EqualsToken
+            && isIdentifier(expression.left)
+            && !checker.getSymbolAtLocation(expression.left);
     }
 }

src/services/codefixes/convertToEs6Module.ts

@@ -13,7 +13,7 @@ namespace ts.codefix {
                 }
             });
             // No support for fix-all since this applies to the whole file at once anyway.
-            return [createCodeFixActionNoFixId("convertToEs6Module", changes, Diagnostics.Convert_to_ES6_module)];
+            return [createCodeFixActionWithoutFixAll("convertToEs6Module", changes, Diagnostics.Convert_to_ES6_module)];
         },
     });
 

src/services/codefixes/disableJsDiagnostics.ts

@@ -18,7 +18,7 @@ namespace ts.codefix {
 
             const fixes: CodeFixAction[] = [
                 // fixId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file.
-                createCodeFixActionNoFixId(
+                createCodeFixActionWithoutFixAll(
                     fixName,
                     [createFileTextChanges(sourceFile.fileName, [
                         createTextChange(sourceFile.checkJsDirective

src/services/codefixes/fixAddMissingMember.ts

@@ -255,7 +255,7 @@ namespace ts.codefix {
 
         const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(declSourceFile, classDeclaration, indexSignature));
         // No fixId here because code-fix-all currently only works on adding individual named properties.
-        return createCodeFixActionNoFixId(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]);
+        return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]);
     }
 
     function getActionForMethodDeclaration(

src/services/codefixes/fixEnableExperimentalDecorators.ts

@@ -13,7 +13,7 @@ namespace ts.codefix {
             }
 
             const changes = textChanges.ChangeTracker.with(context, changeTracker => doChange(changeTracker, configFile));
-            return [createCodeFixActionNoFixId(fixId, changes, Diagnostics.Enable_the_experimentalDecorators_option_in_your_configuration_file)];
+            return [createCodeFixActionWithoutFixAll(fixId, changes, Diagnostics.Enable_the_experimentalDecorators_option_in_your_configuration_file)];
         },
         fixIds: [fixId],
         getAllCodeActions: context => codeFixAll(context, errorCodes, (changes) => {

src/services/codefixes/fixEnableJsxFlag.ts

@@ -14,7 +14,7 @@ namespace ts.codefix {
                 doChange(changeTracker, configFile)
             );
             return [
-                createCodeFixActionNoFixId(fixID, changes, Diagnostics.Enable_the_jsx_flag_in_your_configuration_file)
+                createCodeFixActionWithoutFixAll(fixID, changes, Diagnostics.Enable_the_jsx_flag_in_your_configuration_file)
             ];
         },
         fixIds: [fixID],

src/services/codefixes/fixInvalidImportSyntax.ts

@@ -26,7 +26,7 @@ namespace ts.codefix {
 
     function createAction(context: CodeFixContext, sourceFile: SourceFile, node: Node, replacement: Node): CodeFixAction {
         const changes = textChanges.ChangeTracker.with(context, t => t.replaceNode(sourceFile, node, replacement));
-        return createCodeFixActionNoFixId(fixName, changes, [Diagnostics.Replace_import_with_0, changes[0].textChanges[0].newText]);
+        return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Replace_import_with_0, changes[0].textChanges[0].newText]);
     }
 
     registerCodeFix({
@@ -89,7 +89,7 @@ namespace ts.codefix {
         if (isExpression(expr) && !(isNamedDeclaration(expr.parent) && expr.parent.name === expr)) {
             const sourceFile = context.sourceFile;
             const changes = textChanges.ChangeTracker.with(context, t => t.replaceNode(sourceFile, expr, createPropertyAccess(expr, "default"), {}));
-            fixes.push(createCodeFixActionNoFixId(fixName, changes, Diagnostics.Use_synthetic_default_member));
+            fixes.push(createCodeFixActionWithoutFixAll(fixName, changes, Diagnostics.Use_synthetic_default_member));
         }
         return fixes;
     }

src/services/codefixes/importFixes.ts

@@ -1,6 +1,7 @@
 /* @internal */
 namespace ts.codefix {
-    export const importFixId = "fixMissingImport";
+    export const importFixName = "import";
+    const importFixId = "fixMissingImport";
     const errorCodes: readonly number[] = [
         Diagnostics.Cannot_find_name_0.code,
         Diagnostics.Cannot_find_name_0_Did_you_mean_1.code,
@@ -542,7 +543,7 @@ namespace ts.codefix {
         const changes = textChanges.ChangeTracker.with(context, tracker => {
             diag = codeActionForFixWorker(tracker, sourceFile, symbolName, fix, quotePreference);
         });
-        return createCodeFixAction("import", changes, diag, importFixId, Diagnostics.Add_all_missing_imports);
+        return createCodeFixAction(importFixName, changes, diag, importFixId, Diagnostics.Add_all_missing_imports);
     }
     function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbolName: string, fix: ImportFix, quotePreference: QuotePreference): DiagnosticAndArguments {
         switch (fix.kind) {

src/testRunner/unittests/tsserver/duplicatePackages.ts

@@ -35,8 +35,6 @@ namespace ts.projectSystem {
                     {
                         description: `Import 'foo' from module "foo"`,
                         fixName: "import",
-                        fixId: "fixMissingImport",
-                        fixAllDescription: "Add all missing imports",
                         changes: [{
                             fileName: user.path,
                             textChanges: [{
@@ -46,6 +44,8 @@ namespace ts.projectSystem {
                             }],
                         }],
                         commands: undefined,
+                        fixId: undefined,
+                        fixAllDescription: undefined
                     },
                 ]);
             }

src/testRunner/unittests/tsserver/untitledFiles.ts

@@ -26,8 +26,6 @@ namespace ts.projectSystem {
             assert.deepEqual<readonly protocol.CodeFixAction[] | undefined>(response, [
                 {
                     description: "Change spelling to 'foo'",
-                    fixAllDescription: "Fix all detected spelling errors",
-                    fixId: "fixSpelling",
                     fixName: "spelling",
                     changes: [{
                         fileName: untitledFile,
@@ -38,6 +36,8 @@ namespace ts.projectSystem {
                         }],
                     }],
                     commands: undefined,
+                    fixId: undefined,
+                    fixAllDescription: undefined
                 },
             ]);
         });

tests/cases/fourslash/codeFixAddMissingAwait_initializer4.ts

@@ -16,13 +16,4 @@ verify.codeFix({
 }`
 });
 
-verify.codeFixAll({
-  fixAllDescription: ts.Diagnostics.Fix_all_expressions_possibly_missing_await.message,
-  fixId: "addMissingAwait",
-  newFileContent:
-`async function fn(a: string, b: Promise<string>) {
-  const x = await b;
-  const y = await b;
-  x + y;
-}`
-});
+verify.not.codeFixAllAvailable("addMissingAwait");

tests/cases/fourslash/codeFixAddMissingMember10.ts

@@ -3,9 +3,9 @@
 //// class C {}
 //// const n: number = new C().add(1, 2);
 
-verify.codeFixAll({
-    fixId: "addMissingMember",
-    fixAllDescription: "Add all missing members",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Declare_method_0),
     newFileContent:
 `class C {
     add(arg0: number, arg1: number): number {

tests/cases/fourslash/codeFixAddMissingMember11.ts

@@ -4,9 +4,9 @@
 //// function f(v: number): void { }
 //// f(new C().add(1, 2))
 
-verify.codeFixAll({
-    fixId: "addMissingMember",
-    fixAllDescription: "Add all missing members",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Declare_method_0),
     newFileContent:
 `class C {
     add(arg0: number, arg1: number): number {

tests/cases/fourslash/codeFixAddMissingMember9.ts

@@ -8,9 +8,9 @@
 ////    }
 ////}
 
-verify.codeFixAll({
-    fixId: "addMissingMember",
-    fixAllDescription: "Add all missing members",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Declare_method_0),
     newFileContent:
 `class C {
     z: boolean = true;

tests/cases/fourslash/codeFixAddMissingMember_generator_function.ts

@@ -6,9 +6,9 @@
 ////    }
 ////}
 
-verify.codeFixAll({
-    fixId: "addMissingMember",
-    fixAllDescription: "Add all missing members",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Declare_method_0),
     newFileContent:
         `class C {
     *method() {

tests/cases/fourslash/codeFixAddMissingMember_non_generator_function.ts

@@ -6,9 +6,9 @@
 ////    }
 ////}
 
-verify.codeFixAll({
-    fixId: "addMissingMember",
-    fixAllDescription: "Add all missing members",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Declare_method_0),
     newFileContent:
         `class C {
     method() {

tests/cases/fourslash/codeFixInferFromUsageCallJS.ts

@@ -8,9 +8,9 @@
 ////    b();
 ////}
 
-verify.codeFixAll({
-    fixId: "inferFromUsage",
-    fixAllDescription: "Infer all types from usage",
+verify.codeFix({
+    index: 0,
+    description: ignoreInterpolations(ts.Diagnostics.Infer_parameter_types_from_usage),
     newFileContent:
 `/**
  * @param {() => void} b

tests/cases/fourslash/fourslash.ts

@@ -236,6 +236,7 @@ declare namespace FourSlashInterface {
             commands?: {}[],
         });
         codeFixAvailable(options?: ReadonlyArray<VerifyCodeFixAvailableOptions> | string): void;
+        codeFixAllAvailable(fixName: string): void;
         applicableRefactorAvailableAtMarker(markerName: string): void;
         codeFixDiagnosticsAvailableAtMarkers(markerNames: string[], diagnosticCode?: number): void;
         applicableRefactorAvailableForRange(): void;

tests/cases/fourslash/importNameCodeFixWithPrologue.ts

@@ -17,9 +17,8 @@
 ////export class B extends A { }
 
 goTo.file("/b.ts");
-verify.codeFixAll({
-    fixId: "fixMissingImport",
-    fixAllDescription: "Add all missing imports",
+verify.codeFix({
+    description: ignoreInterpolations(ts.Diagnostics.Import_0_from_module_1),
     newFileContent:
 `"use strict";
 
@@ -29,9 +28,8 @@ export class B extends A { }`,
 });
 
 goTo.file("/c.ts");
-verify.codeFixAll({
-    fixId: "fixMissingImport",
-    fixAllDescription: "Add all missing imports",
+verify.codeFix({
+    description: ignoreInterpolations(ts.Diagnostics.Import_0_from_module_1),
     newFileContent:
 `/*---------------------------------------------------------------------------------------------
  *  Copyright (c) Microsoft Corporation. All rights reserved.