User a looser comparison to match Diagnostic objects

[keithfancher]

This fixes #3857, and makes "remove all redundant imports" code action work again in neovim.

However, I don't know at all if this is an appropriate fix! Opening the PR anyway to start the discussion. (And also so people can build HLS from this branch manually, if they want, to work around an annoying bug.)

The root cause of this was neovim no longer sending the tags field in their Diagnostic objects. (See neovim/neovim#27318.) This was causing a lookup to be missed here.

However, it appears that according to the LSP docs, that's not a required field. And in fact, only message and range are required.

My fix is very local. If this issue exists in other plugins or other code actions, this will not affect them. (For better or worse.) Also, I don't know if there are better patterns to do this kind of thing in Haskell generally...

Like I said, just opening this to start the discussion. So... discuss! 😄

[michaelpj]

I think I'm still not quite following. Diagnostics come from the server, not the client. So I think what must be happening is:

1. We send them a diagnostic with a tag
2. They send it back to us without the tag when making a code action request for a range overlapping that diagnostic

If that's so, I think there are several things going on:

1. I'm not sure we should be relying on which diagnostic is sent by the client in order to decide what code actions to return. I'm not really sure why we do that at all. We could (and I think) should instead filter by the range in the params.
   • There are circumstances where this might be appropriate. For example "disable this warning". If for some reason the warning diagnostic isn't visible to the user, that would be a nonsensical code action to show. But in this case we are using the diagnostics as a proxy for an objective fact about the code, namely whether imports are redundant or not.
2. I do think neovim is being unhelpful here. The spec is of course totally silent on what you're supposed to do here, but I do think it's reasonable to expect that the diagnostics which the client sends in the code action context are exactly some of those that the server has sent, not ones that have been tinkered with.

If you like, you could try implementing my suggestion about filtering based on range instead of the diagnostics. I think that would be both simpler and more robust. For one thing, there is really no guarantee that clients even send the diagnostics field, so relying on that may just make some things not work...

[asivitz]

It works for me. (I reported the original issue.) Thank you!!

[michaelpj]

Looking at the spec more closely, I see the Diagnostic.data field, which specifically says that it's preserved between diagnostic publication and codeAction requests. So possibly what we're supposed to do to identify diagnostics is to give them a tag in data and then use that to match them up again. Tedious, but probably sensible.

[keithfancher]

Re: How things are now:

I definitely agree, it's a little strange. And more importantly, it seems quite brittle. That said, after doing some spot-checking of other HLS plugins, it seems like they're doing very similar operations. (i.e. doing a loose check of specific fields of the incoming CodeActionContext diagnostics.)

If there are larger questions at play of how code actions should work (in HLS or more generally), it might still be worth merging this change or something like it to un-break the feature in the meantime. (I really felt the pain when removing all these unused imports manually, for example 😅 .)

Re: How things should be:

Couple observations based on my limited understanding (of HLS, LSP, and Haskell, haha):

• As I said above, these checks are all pretty brittle. Regex checks against messages, checks against specific fields which aren't guaranteed to even exist, etc. A lot of that is probably inescapable, just based on the nature of how this all works, but then also...
• The checks are scattered. Seems like each plugin is doing very similar logic in its own particular way. Strikes me that there could be a central place to map from, say, Diagnostic data to a set of enums or something that represent categories of diagnostics that a given code-action-provider would care about. (Not sure whether that kind of metadata should/could go in the Diagnostic.data field, or what.) But that would simplify the specific plugin logic a bit and provide more consistency, I think. And discoverability for new plugin authors, maybe? (I don't feel strongly about this idea, and it's possible each plugin's needs are too specific to make that worth doing. But it's a thought!)
• I don't have much insight into the LSP spec, but perhaps one way to solidify some of these questions could be to look at the MS-authored stuff as reference implementations? Like, VS Code and whatever language server(s) they've built. (TypeScript, maybe?) Or just file issues, I guess, like the person on the neovim issue suggested 😄

Jeez, this kinda turned into an essay, heh. Apologies!

[keithfancher]

Oh awesome, I just saw they pushed a fix on the neovim side too! 🎉

[michaelpj]

Since the issue is hopefully fixed on the neovim side and doesn't present elsewhere, I'm inclined to skip the local fix. I've made #4056 to discuss the broader issue. I very much agree that our current handling is dodgy, hopefully we can figure out how we should be doing it and do that instead.

[keithfancher]

Since the issue is hopefully fixed on the neovim side and doesn't present elsewhere, I'm inclined to skip the local fix.

Makes sense to me! As long as the "remove all" feature comes back to life, I'm happy. I'm all for not merging a "temporary" fix if there's another path forward.

I'll close this PR but leave the branch around, at least until the next version of neovim is cut, just as a potential workaround for folks who might need it. (I assume it's easier for most HLS users to build HLS than neovim.)

I've made #4056 to discuss the broader issue. I very much agree that our current handling is dodgy, hopefully we can figure out how we should be doing it and do that instead.

Excellent! Subscribing with interest to the new issue 😄

Thanks!