Handling of Diagnostics in code action contexts is questionable

[michaelpj]

When we get a textDocument/codeAction request, the context contains some Diagnostics. These are (allegedly) diagnostics that are a) visible to the user and b) overlap with the requested range.

However, it's fairly unclear what the client is allowed to do in populating this field. In particular, it's not clear that they have to give it back to us as we sent it to them. The spec also indicates that this field is unreliable, the doc for it says

They are provided so that the server knows which errors are currently presented to the user for the given range. There is no guarantee that these accurately reflect the error state of the resource. The primary parameter to compute code actions is the provided range.

(emphasis mine)

We rely on the context Diagnostics quite a bit:

• Various code actions that trigger off diagnostics inspect them
  • This seems questionable since in most cases we want to trigger based on the objective fact about the code that triggers the diagnostic (e.g. "redundant import"), and not on whether this fact is presented to the user.
• In some places we compare them against existing diagnostics we know about
  • This can go wrong if the client messes with the diagnostic, see e.g. 'Remove redundant imports' code action not showing in neovim 0.9 #3857
  • If we actually care about this we should put identifying information in the data field.

Moreover, we don't really need to do this at all: we can just consult our diagnostic store and filter it based on the given range. This is more direct and simpler, and we should probably just do this in almost all cases. The exception would be cases where we actually do care about presentation: for example, the "suppress this warning" code action does care about whether the diagnostic it is proposing to suppress is in fact visible to the user. But this is an exceptional case.

[michaelpj]

See also #4051 (comment)

[keithfancher]

See also #4051 (comment)

To clarify/summarize my somewhat-rambly comment: I agree!

A process like this makes a lot of sense to me:

1. Filter all available server-side diagnostics based only on the incoming range from the client.
2. Based on the results of that filter, i.e. still using the server-side diags, decide which code actions to show. (The messy bits: regexes and other matching logic, etc.)

And I think there's potential to consolidate some of the logic for both of these steps to simplify things for plugin authors a bit.

As for edge cases/exceptions... I'll leave it to the experts!