Vscode: [Api] Auto fix on save and fix all

[mjbvz]

(edit: see here for a more recent proposal. Original proposal below)

Problem

ES Lint, TS Lint, and other extensions have the concept of auto fix on save. On save, this goes through the errors file and fixes all simple ones, such as removing tailing whitespace or converting { x: string } to Array<{ x: string }>. This functionality is quite useful but is currently implemented per extension. It would be nice if we could:

• Control auto fix on save with a single setting.
• Indicate auto fixes in the code action context menu (since these are usually the preferred fixes)
• Bind auto fix to a keyboard shortcut

Related
Many extensions implement a fix all of this type code action / quick fix. This can be viewed as a special case of auto fix on save where the candidate fixes are only of a single type.

Sketches

(Note that the sketch below record some of the options being considered for this API. They are not yet concrete proposals)

---

Do nothing

Keep things as is. Let extensions handle this all individually

Benefits

• No work! (unless we maybe try to establish some sort of convention for extensions to follow)

Drawbacks

• Many different ways of implementing the same thing
• Cannot bring auto fix or fix all into UI in a consistent way
• Cannot support things that require standardization, such as keybindings or commands

---

CodeAction.autoFixable

Add an autoFixable flag to CodeAction

class CodeAction {
    ...

   autoFixable?: boolean;
}

Use this flag to determine which code actions should be applied on save.

Introduce a editor.autoFixOnSave setting to enable or disable auto fix on save

Implementing auto fix on save

1. For every diagnostic in a file
2. Request all code actions for that diagnostic.
3. If there are is only one code action marked autoFixable in the set, then apply that code action

Implementing auto fix all errors of type X

1. Assuming we have UI that shows a auto fix all errors of type X suggestion .
2. For each diagnostic of type X in the file, request code actions.
3. Filter to only those that CodeAction.diagnostics set to include the diagnostic of type X.
4. Then, if we only have one code action left for that diagnostic, apply it.

Maybe autoFixable here should actually be an array of diagnostics? Would that help with filtering.

Benefits

• Theoretically this lets us address all requirements (perhaps with a few api tweaks)

Drawbacks / Open questions

• Supper chatty api
• Not easy to handle recursive or overlapping fixes

---

CodeActionKind.SourceAutoFix

Add a new code action kind called source.autoFix. Update extensions to return a source code action that fixes all auto fixable errors in the file.

Use "editor.sourceActionsOnSave": ["source.autoFix"] to apply code actions on save

Implementing auto fix on save
Basically no work on VS Code side. Get extensions to adopt this new API.

Implementing auto fix all errors of type X
We could do something like have multiple source code actions for autofix, such as:

• source.autoFix.123
• source.autoFix.456

This may gets confusing though since requesting the code actions for source.autoFix would end up including the source code actions of source.autoFix.123 and source.autoFix.456 unless the extension is smart. Maybe we'd have to have a source.autoFix.all to prevent this for the autofix on save case? Or perhaps it doesn't matter? If editor.sourceActionsOnSave applies all actions of source.autoFix, presumably applying source.autoFix.123 and source.autoFix.456 should work?

Benefits

• Easy to implement
• Allows extensions to compute a batch of edits
• "editor.sourceActionsOnSave": ["source.autoFix"]

Drawbacks / Open questions

• What happens if multiple extensions contribute fix all actions? With current proposal, the source code menu would just show an fix all action for each extension. Would be nice if we just have one entry (autofix on save may still work though, at least as long as the fixes don't overlap)
• Can we actually implement auto fix all errors of type X?
• Is there any standard way to surface autofix in the UI with this proposal?

[mjbvz]

I'm in favor of the CodeActionKind.SourceAutoFix approach as it slots in nicely with existing apis. Still some open questions, specifically around how to present the fix all action in the UI

Also need to see if other languages/extensions have a use case for this.

/cc @dbaeumer and @jrieken

[jrieken]

Not sure that I understood all the implications outlined above but making auto-fix a property and not a kind sound more correct, e.g. I can have all kinds of code actions that can be auto-fixed/applied.

[dbaeumer]

Reading through it I prefer CodeActionKind.SourceAutoFix however I don't fully understand all implications either.

[mjbvz]

Documenting a few other proposals

CodeAction.autoFixable + CodeActionContext. autoFixable

(discussed with @jrieken )

This proposal would add a new CodeAction.autoFixable flag on code actions as originally proposed.

class CodeAction {
    ...

   autoFixable?: boolean;
}

In addition, in order to address the problems around efficiency and overlapping changes with first proposal, we would add a new autoFixable field to the CodeActionContext

class CodeActionContext {
    ...

   autoFixable?: boolean;
}

When requesting the fix all code actions, the context would set autoFixable: true and filter out any code actions that are not marked as autoFixable. This allows providers to compute overlapping changes and to return all code actions in a single batch

CodeAction.autoFixable + CodeActionKind.SourceAutoFix

(discussed with @kieferrm)

This proposal basically combines the first two proposals. The idea here is that there are two issues we are really interested in solving:

• Fix all in file source action. Using SourceAutoFix works nicely for this
• F8 through file and hit a button to fix an error. This is where the CodeAction.autoFixable property comes in

[mjbvz]

I'm in favor of the last proposal — CodeAction.autoFixable + CodeActionKind.SourceAutoFix — as it lets us address most of the original concerns (except the fix all errors of type X one) and the F8 case is pretty interesting too

@dbaeumer From the LSP point of view, CodeActionKind.SourceAutoFix would just be a new CodeActionKind. It would work just like organize imports. Providers that implement it would return a single code action that fixes all auto-fixable errors in the file (the provider and not VS Code would be responsible for identifying auto-fixable errors).

[mjbvz]

A more complete writeup on the last proposal:

Problem

ES Lint, TS Lint, and other extensions have the concept of auto fix on save. On save, this goes through the errors file and fixes all simple ones, such as removing tailing whitespace or converting { x: string } to Array<{ x: string }>. This functionality is quite useful but is currently implemented per extension. It would be nice if we could:

• Standardize auto fix on save: have with a single setting to enable or disable this, clarify expected behavior.
• Indicate auto fixes in the code action context menu (since these are usually the preferred fixes)
• Bind auto fix to a keyboard shortcut

Proposal

There are two related but separate problems with this proposal: an action that auto fixes problems in a file, and auto fixing individual errors.

Auto-fixing all auto-fixable errors in a file

Motivating example
You enable a new linter error in your project that bans semicolons. This introduce 1.21 gazillion errors in every file. Thankfully, your linter extension has implemented the fix all source action so removing those vile semicolons is simple!

You can even fix all linter errors when you save a file by setting editor.codeActionsOnSave in case you accidentally introduce a new semicolon

Proposal

1. Introduce a new CodeActionKind called SourceFixAll with the scope source.fixAll.

 Implementing this would be very similar to the `SourceOrganizeImports` actions. The implementing extension could compute a single edit that would fix all autofixable errors in the file. It would be up to the extension to determine what can be auto fixed and how to fix

1. Document that you can set "editor.codeActionsOnSave": ["source.fixAll'] to apply this code action on save

Auto fixing individual errors

Motivating example
You open the following TS file:

interface IFoo {
    bar(): void;
}

class Foo implements IFoo { }

new foo()

You hit F8 to start navigating through errors. While navigating, you can hit a single keyboard shortcut to auto implement the interface or to correct foo -> Foo

Proposal

1. Introduce a new isPreferred field on the CodeAction type:

 ```ts
 class CodeAction {
     ...
    /**
      * Indicates that this quick fix can be automatically applied as a correct fix for the error
      *
      * `implement interface` for example would be marked `canAutoApply` while a error suppression
      * would not be.
      */
    isPreferred?: boolean;
 }
 ```

1. Introduce editor.action.autoFix command with a keyboard shortcut, something like cmd+option+. which is similar to the regular quick fix shortcut. This action would:

   • Request code actions for the current diagnostic.
   • If only a single code action marked canAutoApply is returned, then apply it .
   • Otherwise show a list of the canAutoApply code actions that were returned

2. (longer term) Use the canAutoApply actions in the UI to indicated preferred code fixes.

[dbaeumer]

@mjbvz regarding Auto fixing individual errors:

How would we deal with a case where there are two errors at the cursor position (from the same or different providers). How would the code action provider know which diagnostic to fix.

IMO the VS Code API and the LSP is unclear when in comes to diagnostics in code action requests. The primary input IMO was always the range. In my language servers I never limited the code actions to the provided diagnostics.

Instead of having autofixable on CodeAction would it make more sense to have a property fixes of type Diagnostic | Diagnostic[] to indicate that this code action fixes the provided diagnostics.

[mjbvz]

@dbaeumer Good points

• There are a few different cases for overlap; the most common for js/ts is that one diagnostic is inside another. I don't think that presents any problems as F8 should take us to one and then into other so we would get different code actions requests each time

  If there are two diagnostics with the exact same ranges, it is likely that we cannot fix both at the same time as the fix changes will conflict with each other. I think in these cases we should show the code action context menu and let the user select which one to apply. This would also be how we handle a single diagnostic that has multiple code actions marked autoFixable

• The CodeAction class has a diagnostics property already. The problem with using that for auto fixing is that error suppression—such as adding // @ts-ignore—also set diagnostics but we likely do not want to auto apply those. We can probably leverage the diagnostics property to implement auto fix though

[mjbvz]

Quick status update:

• 1904cd8d84c5aa615cb7730fd367d3ffae1fd0db adds a canAutoApply proposed property to code actions and enables it for TS spelling quick fixes. This is hooked up to an auto fix command that is triggered using cmdoption.. The current behavior applies the fix if there is only a single fix available, otherwise it shows a list of fixes. The flow is actually really nice with F8 (except the new keyboard shortcut is awkward on mac where you need to press fnf8)

• 66521 Adds the Source.AutoFix code action kind to the proposed API. It is hooked up to fix spelling errors and implement interface errors in TS.

Todo

• Try reaching out to some extension authors to get feedback on these proposed apis
• See if there is TypeScript buy in for these features.
• Polishing UI
• Add an autoApplyOnly property to the CodeActionContext so that providers know which code actions will be discarded?
• Add an providesAutoApplyActions property to CodeActionProviderMetadata so we can avoid calling providers that will not return auto fixes in the first place?

[DavidAnson]

Thanks for reaching out!

Some of my concerns about implementing this on my own are already captured in
https://github.com/DavidAnson/markdownlint/issues/80.

I trust you folks to come up with the right API; I’m more worried about how this works in practice. In addition to the topics already under discussion, something I don’t see captured for fix-all is that applying one fix could obviate another -or- create new things to fix (especially cross-extension). Is the process iterative? What if two fixes conflict with each other or create a feedback loop forever?

So far, this seems straightforward to prototype. If you want me to do so in a private branch for experimentation, let me know. That may be enlightening. :)

[mjbvz]

Thanks for the feedback.

You bring up some good points. For fix-all, we leave most of this up to extensions. Overlaps and changes that generate more auto fixable issues could be handled by your extension when it computes the changes.

If two separate extensions return overlapping fix-all edits, we would currently try to apply both sets of edits together. We may switch to a sequential or iterative approach instead if this proves to be a common problem in real world usage

[rbiggs]

I don't mind the concept of fix on save, and I think it's a good thing overall. However, I would not want it to be something that happens by default unless I turn it on. Reason, many times the default fixes can be problematic for what I'm actually doing. I most case I would just rather see the lint error and then deal with that myself because depending on the situation, I may prefer to change the settings to remedy the lint error. I don't always agree with every linter rule and don't like being forced to use them whether I want to or not.

[mjbvz]

@rbiggs Fix all would not be enabled on save by default. Like organize imports it would have a command you could manually run and you could optionally enable it on save per language

[mjbvz]

Both api components are now merged into VS Code as proposed apis.

---

We may want to rename source.autoFix to source.cleanup or something else:

• source.autoFix is confusing if we have an auto fix action in the editor that fixes individual errors

• Roslyn is using the term "cleanup": http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/CodeCleanup/ICodeCleanupService.cs,f5c8fe504cd2b3ad,references

However "cleanup" may be too generic of a term since organize imports and format could also be considered under "cleanups"

[dbaeumer]

@mjbvz I read through the API again and I am still puzzled how we ensure that the provided fix is for a given diagnostic. Or is this based on trust?

[mjbvz]

CodeActions.diagnostics lists the diagnostics that a quick fix addresses. How diagnostics is used is up to the editor. We currently only use it for sorting in the lightbulb menu and don't use it to filter code actions.

Similarly, the auto fix action requests all preferred quick fixes at a given location (which more likely than not would be a diagnostic) but in the editor we do not use diagnostics to filter out other returned fixes

[mjbvz]

Ok, I changed the name for the action from Source.autoFix to Source.fixAll. The intent of this action is to address errors in the code and cleanup is too generic sounding for that

[dbaeumer]

@mjbvz thanks for the clarification. One additinal question: how would you (if necessary) determine that two diagnostics are equal. Object identity? In LSP we create new diagnostic objects.

[mjbvz]

@dbaeumer In the VS Code source, we have a IMarkerData.makeKey function that converts a diagnostic into a key. I think we could compare keys to check if two diagnostics are equal (even if they are different objects)

[mjbvz]

CodeAction.isPreferred and CodeActionKind.SourceFixAll apis are scheduled to be proposed APIs for the January release.

Closing this exploration issue. We are tracking finalizing CodeAction.isPreferred with #67144 and SourceFixAll with #67145. Please share any further feedback on these api proposals on those issues.

[dbaeumer]

@mjbvz thanks. That makes live in LSP easier.