Multiple code action kinds requests during code actions on save

[rchl]

Describe the bug

I've mentioned that (vscode interoperability) issue in some other places but I'm too lazy to find those now.

When asking servers for code actions during "code actions on save" task, we should create separate requests per code action kind and not include multiple code action kinds in a single request. Otherwise we might try to apply multiple text edits that are not compatible with each other (if server doesn't provide version for text edits).

[predragnikolic]

I've mentioned that (vscode interoperability) issue in some other place but I'm too lazy to find those now.

here it is #2421 (comment)

some additional info on how to reproduce the issue is to add:

"lsp_code_actions_on_save":
    {
        "source.fixAll.eslint": true,
        "source.fixAll.ts": true,
        "source.organizeImports.ts": true,
        "source.addMissingImports.ts": true,
        "source.sortImports.ts": true,
    },

[jwortmann]

To be fair, I think this has nothing really to do with VSCode interoperability, but the current design used here is just flawed. The server responds with a list of code actions for the current version of the document, which is totally expected. As soon as one of the code action gets applied, in general the document will change and the workspace edits of all other code actions are therefore invalidated. The server does not know beforehand whether the code actions will be applied, and if so in what order.

It would be the same situation with the regular code actions for the current cursor position; if you would try to apply all actions (if there are multiple) at the same time, the resulting code would probably be broken as well.

I think in the LSP-ruff example from sublimelsp/LSP-ruff#72, the server not sending a version in the edits could be considered as a bug and a misinterpretation of the specs. As far as I understand, version should only be null if the document is not opened in the editor:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#optionalVersionedTextDocumentIdentifier

         /* If an optional versioned text document
	 * identifier is sent from the server to the client and the file is not
	 * open in the editor (the server has not received an open notification
	 * before) the server can send `null` to indicate that the version is
	 * known and the content on disk is the master
	 */

But yes, the way how multiple code actions on save are handled in LSP is wrong too and should be fixed. Currently in the best case the version is included in the edits, which means that only one of the specified code actions on save will be applied and the others are ignored due to mismatched version after the first change.

[rchl]

Yes, it's probably more a bug than interoperability issue but since vscode is a spec, maybe you can call it either. :)

@predragnikolic I think to reproduce the biome issue you need the config from the thread you've linked. The one you've pasted is related to recently discussed on Discord LSP-typescript issue but I'm not convinced that that is related to this issue.

[travisstaloch]

I really hope this issue gets some attention. I found this issue through zigtools/zls#2076 and have been noticing this bug quite a bit lately when the zig language server applies multiple quick fixes at once.

I'm not sure I'd be able to fix anything, but would like to look into this if anyone could provide some guidance. It sounds like we need a way to apply multiple code actions to a single document version.

Does anyone have a plan yet for how this might work? Would a diff library be helpful?

[jwortmann]

It sounds like we need a way to apply multiple code actions to a single document version.

Does anyone have a plan yet for how this might work? Would a diff library be helpful?

I would not try to apply multiple code actions to the same document version. When the edits from a code action are applied, arbitrary lines of the file(s) can change and then the edit ranges of the next code action could become invalid. I assume that manually sorting beforehand or diffing and tracking changes would be very error prone or even infeasable if there are more than two code actions. Instead, I would only apply a single code action from the server response(s), and after that I would request code actions again if there was more than one code action in the responses. Doing that should automatically purge the applied changes. So at least this part should probably be rewritten:

LSP/plugin/code_actions.py

Lines 263 to 269 in 9be040d

	for config_name, code_actions in responses:
	session = self._task_runner.session_by_name(config_name, 'codeActionProvider')
	if session:
	tasks.extend([
	session.run_code_action_async(action, progress=False, view=view) for action in code_actions
	])
	Promise.all(tasks).then(lambda _: self._on_complete())

This code currently runs all actions from all specified kinds and from all servers at once.

As an optimization, perhaps it could be made more efficient by introducing some kind of queue for the action kinds and/or sessions, to limit the amount of total requests and to not request the same kinds over and over again.

[travisstaloch]

Thanks for the ideas! That makes a lot of sense. I'll see if I can remember how do plugin dev and then work toward this part.

apply a single code action from the server response(s), and after that I would request code actions again

Might not get started until next week or so. Will post updates.

[travisstaloch]

I haven't made any progress on this. Not sure when I'll find any time for it.

Will post back if I things change but if anyone else wants to work on this, go ahead.