Add more description to Diagnostic[] in CodeActionContext specification

[jzashch]

Hi!

I was looking into documentation for this request https://microsoft.github.io/language-server-protocol/specification#textDocument_codeAction and it is not really clear what should be inside the Diagnostic[] array in CodeActionContext. Which diagnostics should the client send? How detailed should be this diagnostics (like which fields does the server actually need)? What will happen if the client simply sends an empty array?

I've looked into https://github.com/Microsoft/TypeScript and if I'm not mistaken only errorCode is used there from the whole diagnostic (please correct me if I'm wrong), so sending all of it seems like too much.

Anyway, it would be really great if you could add more context to this part of specification.

[rcjsuen]

@jzashch Note that this is simply my interpretation.

1. The client should send diagnostics that have a range that overlaps/contains the current text cursor/selection.

2. Each server behaves differently and may use more or less information then what is included in the diagnostic. I feel like as-is the client should simply send back the original diagnostic in its entirety in the event that the server expects everything. Note that one could however imagine a world where the server declares what it needs and the client can consume this information and only send back what is needed to save space over-the-wire (presumably mostly taken up by the message and relatedInformation fields.

3. That is a valid request. There may be code actions that are not related to an error (such as the generation of getters and setters, extract interface, extract method/function, remove unused field, and so on and so on...).

[DanTup]

I've just hit this too. All the docs say are:

An array of diagnostics.

Each diagnostic has its own range - should we return fixes for all of them plus the one from CodeActionParams.range? Or only the ones relating to the diagnostics? Do we have any guarantees that the diagnostics are within the first range? What if there are diagnostics from a different text document - should we include code actions for them?

For now I'm going to completely ignore the diagnostics argument because it's not clear how to handle it, but I don't know if that's entirely a valid thing to do.

[kdvolder]

I'll second @DanTup that the spec is a bit fuzzy on the meaning of diagnostics and ranges and how they are supposed to interact in selecting 'matching' code actions.

What we ended up doing is to assume that if the list of Diagnostics is empty then only the range matters (this much seems obvious, as... what else could you do in that case). I assume this corresponds to a scenario where the client is querying for code actions in relation to an editor location. E.g. when we would press 'CTRL+1' in Eclipse to get codeactions / quickfixes for the current cursor position.

When the diagnostics list is not empty then we return just the quickfixes that directly associated with these specific diagnostics. We are assuming this kind of request arises in the context of something like a 'problems' view. In such a view, a user might click on or select a specific (set of) problem(s) and ask for a quickfix. So in this scenario we are really asking for fixes for a specific problem marker, not code actions for anything in a given range of text.

Our interpretation seems somewhat logical given those two expected use-cases, but none of these details are clear from reading the spec.

[dbaeumer]

Here is what I think we should specify:

• the primary parameter to compute code actions is the range.
• the diagnostics provided should be the once the client now about and should overlap with the range (unless someone has a use case for diagnostics outside the range)
• I think we should not propose to deduce the origin of the request from the provided diagnostics. VS Code for example sends them as well when the action is triggered in the editor. If we think we need an origin then we should add a corresponding property to the context.

Comments?

[DanTup]

Am I right in thinking the purpose of providing them is just for convenience/performance for the server (eg. to avoid the server needing to re-compute diagnostics if it doesn't have them)?

If so, should it be stated that if the client sends them, it must include them all (so the server can trust that it doesn't need to go and figure them out again)?

[dbaeumer]

the purpose of providing them is just for convenience/performance ..

Yes, that is my current thinking. I think that most servers will have the state internally anyways and will not use the provided information.

it must include them all (so the server can trust that it doesn't need to go and figure them out again)?

You mean all in the range ?

[DanTup]

You mean all in the range ?

Sorry, yes :)

For the server to use them (if it doesn't already have them) it will want to be sure that they're complete, so if the array is present, the client should ensure it includes all diagnostics that intersect with the range provided.

Also - an empty array should probably signify that there definitely weren't any (to avoid the server having to recompute them if there were actually zero). If the client doesn't want to send them, it should not included the diagnostics (eg. they're undefined, and not an empty array).

[keyboardDrummer]

If the client doesn't want to send them, it should not included the diagnostics (eg. they're undefined, and not an empty array).

The argument is not optional, so the client is forced to send them.

[DanTup]

Oh yeah 😔 Changing that now is a breaking change too.

[keyboardDrummer]

What's also confusing is that the diagnostics sent by the client with codeAction are in practice not the same as what was originally sent by the server. For example, VSCode will drop the 'severity' field when sending the diagnostics back.

[rcjsuen]

For example, VSCode will drop the 'severity' field when sending the diagnostics back.

Interesting. That sounds like a bug then that should be filed with microsoft/vscode-languageserver-node then.

[keyboardDrummer]

Interesting. That sounds like a bug then that should be filed with microsoft/vscode-languageserver-node then.

Sorry, I just noticed this is no longer the case for the latest vscode-languageserver-node