Skip to content

Commit 797c536

Browse files
andrewbranchandrewbranch
authored
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
1 parent eeff036 commit 797c536

23 files changed

+97
-58
lines changed

‎src/harness/fourslashImpl.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2544,8 +2544,8 @@ namespace FourSlash {
25442544

25452545
public verifyCodeFixAll({ fixId, fixAllDescription, newFileContent, commands: expectedCommands }: FourSlashInterface.VerifyCodeFixAllOptions): void {
25462546
const fixWithId = ts.find(this.getCodeFixes(this.activeFile.fileName), a => a.fixId === fixId);
2547-
ts.Debug.assert(fixWithId !== undefined, "No available code fix has that group id.", () =>
2548-
`Expected '${fixId}'. Available action ids: ${ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => a.fixId)}`);
2547+
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.", () =>
2548+
`Expected '${fixId}'. Available actions:\n${ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => `${a.fixName} (${a.fixId || "no fix id"})`).join("\n")}`);
25492549
ts.Debug.assertEqual(fixWithId!.fixAllDescription, fixAllDescription);
25502550

25512551
const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings, ts.emptyOptions);
@@ -2681,7 +2681,7 @@ namespace FourSlash {
26812681
}
26822682
const range = ts.firstOrUndefined(ranges);
26832683

2684-
const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixId === ts.codefix.importFixId);
2684+
const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixName === ts.codefix.importFixName);
26852685

26862686
if (codeFixes.length === 0) {
26872687
if (expectedTextArray.length !== 0) {
@@ -2717,7 +2717,7 @@ namespace FourSlash {
27172717
const codeFixes = this.getCodeFixes(marker.fileName, ts.Diagnostics.Cannot_find_name_0.code, {
27182718
includeCompletionsForModuleExports: true,
27192719
includeCompletionsWithInsertText: true
2720-
}, marker.position).filter(f => f.fixId === ts.codefix.importFixId);
2720+
}, marker.position).filter(f => f.fixName === ts.codefix.importFixName);
27212721

27222722
const actualModuleSpecifiers = ts.mapDefined(codeFixes, fix => {
27232723
return ts.forEach(ts.flatMap(fix.changes, c => c.textChanges), c => {
@@ -3044,6 +3044,26 @@ namespace FourSlash {
30443044
}
30453045
}
30463046

3047+
public verifyCodeFixAllAvailable(negative: boolean, fixName: string) {
3048+
const availableFixes = this.getCodeFixes(this.activeFile.fileName);
3049+
const hasFix = availableFixes.some(fix => fix.fixName === fixName && fix.fixId);
3050+
if (negative && hasFix) {
3051+
this.raiseError(`Expected not to find a fix with the name '${fixName}', but one exists.`);
3052+
}
3053+
else if (!negative && !hasFix) {
3054+
if (availableFixes.some(fix => fix.fixName === fixName)) {
3055+
this.raiseError(`Found a fix with the name '${fixName}', but fix-all is not available.`);
3056+
}
3057+
3058+
this.raiseError(
3059+
`Expected to find a fix with the name '${fixName}', but none exists.` +
3060+
availableFixes.length
3061+
? ` Available fixes: ${availableFixes.map(fix => `${fix.fixName} (${fix.fixId ? "with" : "without"} fix-all)`).join(", ")}`
3062+
: ""
3063+
);
3064+
}
3065+
}
3066+
30473067
public verifyApplicableRefactorAvailableAtMarker(negative: boolean, markerName: string) {
30483068
const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName)).length > 0;
30493069
if (negative && isAvailable) {

‎src/harness/fourslashInterfaceImpl.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,10 @@ namespace FourSlashInterface {
191191
this.state.verifyCodeFixAvailable(this.negative, options);
192192
}
193193

194+
public codeFixAllAvailable(fixName: string) {
195+
this.state.verifyCodeFixAllAvailable(this.negative, fixName);
196+
}
197+
194198
public applicableRefactorAvailableAtMarker(markerName: string) {
195199
this.state.verifyApplicableRefactorAvailableAtMarker(this.negative, markerName);
196200
}

‎src/services/codeFixProvider.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ namespace ts.codefix {
1010
: getLocaleSpecificMessage(diag);
1111
}
1212

13-
export function createCodeFixActionNoFixId(fixName: string, changes: FileTextChanges[], description: DiagnosticAndArguments) {
13+
export function createCodeFixActionWithoutFixAll(fixName: string, changes: FileTextChanges[], description: DiagnosticAndArguments) {
1414
return createCodeFixActionWorker(fixName, diagnosticToString(description), changes, /*fixId*/ undefined, /*fixAllDescription*/ undefined);
1515
}
1616

@@ -38,8 +38,24 @@ namespace ts.codefix {
3838
return arrayFrom(errorCodeToFixes.keys());
3939
}
4040

41+
function removeFixIdIfFixAllUnavailable(registration: CodeFixRegistration, diagnostics: Diagnostic[]) {
42+
const { errorCodes } = registration;
43+
let maybeFixableDiagnostics = 0;
44+
for (const diag of diagnostics) {
45+
if (contains(errorCodes, diag.code)) maybeFixableDiagnostics++;
46+
if (maybeFixableDiagnostics > 1) break;
47+
}
48+
49+
const fixAllUnavailable = maybeFixableDiagnostics < 2;
50+
return ({ fixId, fixAllDescription, ...action }: CodeFixAction): CodeFixAction => {
51+
return fixAllUnavailable ? action : { ...action, fixId, fixAllDescription };
52+
};
53+
}
54+
4155
export function getFixes(context: CodeFixContext): readonly CodeFixAction[] {
42-
return flatMap(errorCodeToFixes.get(String(context.errorCode)) || emptyArray, f => f.getCodeActions(context));
56+
const diagnostics = getDiagnostics(context);
57+
const registrations = errorCodeToFixes.get(String(context.errorCode));
58+
return flatMap(registrations, f => map(f.getCodeActions(context), removeFixIdIfFixAllUnavailable(f, diagnostics)));
4359
}
4460

4561
export function getAllFixes(context: CodeFixAllContext): CombinedCodeActions {
@@ -65,11 +81,15 @@ namespace ts.codefix {
6581
return createCombinedCodeActions(changes, commands.length === 0 ? undefined : commands);
6682
}
6783

68-
export function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: readonly number[], cb: (diag: DiagnosticWithLocation) => void): void {
69-
for (const diag of program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken))) {
84+
export function eachDiagnostic(context: CodeFixAllContext, errorCodes: readonly number[], cb: (diag: DiagnosticWithLocation) => void): void {
85+
for (const diag of getDiagnostics(context)) {
7086
if (contains(errorCodes, diag.code)) {
7187
cb(diag as DiagnosticWithLocation);
7288
}
7389
}
7490
}
91+
92+
function getDiagnostics({ program, sourceFile, cancellationToken }: CodeFixContextBase) {
93+
return program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken));
94+
}
7595
}

‎src/services/codefixes/addMissingAwait.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,9 @@ namespace ts.codefix {
6868
makeChange(t, errorCode, sourceFile, checker, expression, fixedDeclarations);
6969
}
7070
});
71-
return createCodeFixActionNoFixId(
71+
// No fix-all because it will already be included once with the use site fix,
72+
// and for simplicity the fix-all doesn‘t let the user choose between use-site and declaration-site fixes.
73+
return createCodeFixActionWithoutFixAll(
7274
"addMissingAwaitToInitializer",
7375
initializerChanges,
7476
awaitableInitializers.initializers.length === 1

‎src/services/codefixes/addMissingConst.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ namespace ts.codefix {
3030
if (forInitializer) return applyChange(changeTracker, forInitializer, sourceFile, fixedNodes);
3131

3232
const parent = token.parent;
33-
if (isBinaryExpression(parent) && isExpressionStatement(parent.parent)) {
33+
if (isBinaryExpression(parent) && parent.operatorToken.kind === SyntaxKind.EqualsToken && isExpressionStatement(parent.parent)) {
3434
return applyChange(changeTracker, token, sourceFile, fixedNodes);
3535
}
3636

@@ -104,6 +104,8 @@ namespace ts.codefix {
104104
return every([expression.left, expression.right], expression => expressionCouldBeVariableDeclaration(expression, checker));
105105
}
106106

107-
return isIdentifier(expression.left) && !checker.getSymbolAtLocation(expression.left);
107+
return expression.operatorToken.kind === SyntaxKind.EqualsToken
108+
&& isIdentifier(expression.left)
109+
&& !checker.getSymbolAtLocation(expression.left);
108110
}
109111
}

‎src/services/codefixes/convertToEs6Module.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ namespace ts.codefix {
1313
}
1414
});
1515
// No support for fix-all since this applies to the whole file at once anyway.
16-
return [createCodeFixActionNoFixId("convertToEs6Module", changes, Diagnostics.Convert_to_ES6_module)];
16+
return [createCodeFixActionWithoutFixAll("convertToEs6Module", changes, Diagnostics.Convert_to_ES6_module)];
1717
},
1818
});
1919

‎src/services/codefixes/disableJsDiagnostics.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ namespace ts.codefix {
1818

1919
const fixes: CodeFixAction[] = [
2020
// fixId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file.
21-
createCodeFixActionNoFixId(
21+
createCodeFixActionWithoutFixAll(
2222
fixName,
2323
[createFileTextChanges(sourceFile.fileName, [
2424
createTextChange(sourceFile.checkJsDirective

‎src/services/codefixes/fixAddMissingMember.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ namespace ts.codefix {
255255

256256
const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(declSourceFile, classDeclaration, indexSignature));
257257
// No fixId here because code-fix-all currently only works on adding individual named properties.
258-
return createCodeFixActionNoFixId(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]);
258+
return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]);
259259
}
260260

261261
function getActionForMethodDeclaration(

‎src/services/codefixes/fixEnableExperimentalDecorators.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ namespace ts.codefix {
1313
}
1414

1515
const changes = textChanges.ChangeTracker.with(context, changeTracker => doChange(changeTracker, configFile));
16-
return [createCodeFixActionNoFixId(fixId, changes, Diagnostics.Enable_the_experimentalDecorators_option_in_your_configuration_file)];
16+
return [createCodeFixActionWithoutFixAll(fixId, changes, Diagnostics.Enable_the_experimentalDecorators_option_in_your_configuration_file)];
1717
},
1818
fixIds: [fixId],
1919
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes) => {

‎src/services/codefixes/fixEnableJsxFlag.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace ts.codefix {
1414
doChange(changeTracker, configFile)
1515
);
1616
return [
17-
createCodeFixActionNoFixId(fixID, changes, Diagnostics.Enable_the_jsx_flag_in_your_configuration_file)
17+
createCodeFixActionWithoutFixAll(fixID, changes, Diagnostics.Enable_the_jsx_flag_in_your_configuration_file)
1818
];
1919
},
2020
fixIds: [fixID],

0 commit comments

Comments
 (0)