Specify that an editor should fallback to unresolved code action if resolve request fails

[MariaSolOs]

VS Code, Helix and (soon) Neovim all fallback to the original code action when resolve fails.

To avoid issues like neovim/neovim#32757 where it's unclear what an editor should do in the scenario where the language server errors when resolving the code action, let's specify what the standard behavior should be.

[dbaeumer]

@mjbvz can you please comment on what the current default behavior in VS Code is in this sceanrio?

[puremourning]

This is a huge change in spec. And it's not even intuitive. If the resolve fails the code action failed.

[mjbvz]

Yeah I'm inclined to say that we should surface this error to the user so they can report it. Ia actually thought that's what we were doing on the VS Code side by calling onUnexpectedExternalError:

https://github.com/microsoft/vscode/blob/cb807ec6ae375343d9bffbaeb5ea9c1f45055ad3/src/vs/editor/contrib/codeAction/common/types.ts#L185

But it turns out we end up silently catching the error

@justschen Want to try changing our code to alert when resolve fails?

[mfussenegger]

I think part of the problem here is that while the client can announce which properties it can resolve via the textDocument.codeAction.resolveSupport capability, the server currently can't tell the client which properties it provides and which need to be resolved.

So if the client says it can resolve both edit and command, and the server provides actions with only command it is unclear if edit can be resolved or if it is simply not available.

Not having this fallback mechanism causes at least issues with the haskell-language-server (neovim/neovim#25464) and ruby-lsp (neovim/neovim#32757)

[mjbvz]

My expectation would be :

1. Client tells the server which properties it can resolve

2. The server then adjust its behavior based on the client's capabilities. If resolve isn't support for example, then the server would do all the work in the main call instead of deferring it to resolve.

3. The client is responsible for calling the server to resolve code actions as needed. The client should expect a response that contains a subset of the properties listed in step 1 (this can be an empty response if the code action was already resolved)

I don't think there should be fallbacks. Instead the states would be:

• The client doesn't support resolve so we get back complete code actions to start with

• The client can support resolve but we get back code actions that don't need to be resolved

• The client tries to resolve a code action but the server returns the same code action, indicating that resolve wasn't needed

• Resolve fails. At this point the code action is invalid and cannot be applied

[MariaSolOs]

@mjbvz that sounds reasonable to me. However, I think that we should apply the same logic to other requests that also have a resolve step. Take completions for example: Based on this snippet, it seems like VS Code uses the unresolved item when the resolve request fails. Shouldn't we extend this idea there too?

The main motivation behind this PR isn't about falling back or not, but to specify in the LSP what should be the correct editor behavior when resolve fails, as it's unclear whether language servers should handle a potentially unexpected code action argument in codeAction/resolve or if the editor should handle failure scenarios.

[dbaeumer]

I fully agree with @mjbvz comment here: #2112 (comment)

@mfussenegger the only scenario I can currently see for server's to announce what they can resolve is performance since we could save an additional server roundtrip. If this really becomes a problem I would instead add a tag/ property to the code action indicating that it is already fully resolved so that clients can avoid the round trip.

[mjbvz]

@MariaSolOs I think it needs thought on a case by case basis

For code actions, resolve was designed to be a core part of applying a code action. The server can choose to fail graceful if it runs into an error during resolve, but I don't think we want to make fallback behavior part of the spec

However for completions, resolve typically adds an additional edit or command (such as an edit adding imports at the top of the file). Again I'd lean towards having the same behavior as code actions (show an error on resolve failure with the server able to catch and handle this gracefully if it wants but) but we'd need to have another discussion for that. For completions you could argue it's better to always try applying the primary edit, even if resolve fails

[MariaSolOs]

@mjbvz sounds good. As long as we document when/how to fallback in the spec to have consistent experiences across editors, I'm fine with that.