Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLS should always respond to resolve requests #4473

Open
fendor opened this issue Dec 28, 2024 · 3 comments
Open

HLS should always respond to resolve requests #4473

fendor opened this issue Dec 28, 2024 · 3 comments
Assignees
Labels
component: ghcide component: hls-plugin-api type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@fendor
Copy link
Collaborator

fendor commented Dec 28, 2024

We have multiple reports, where resolve requests (such as completion/resolve and codeAction/resolve) are rejected by HLS since the _data field of the respective LSP feature has not been populated by HLS. This makes sense, as we only support resolve for certain kinds of CodeAction/Completions, when they contain particularly expensive properties, such as documentation or non-local type signatures.

In #4463, I added a plaster for completions, deciding to always add the _data field, which allows our run-time system to resolve such requests to the completion plugin.
While this should work for completions, we have the same issue with completions provided by the hls-cabal-plugin, which serves completions for .cabal files.
Moreover, multiple plugins provide CodeActions but not necessarily CodeAction/resolve handlers, causing HLS to reject such requests.

This behaviour is troublesome, as vim clients (maybe emacs as well) seemingly show such rejections prominently in the UI, perhaps even outright not applying already present Codefixes.

The solution might be something like this:

If the _data field is Nothing we do not try to resolve a */resolve request to a specific plugin, but always return the request data unchanged. If the _data field is provided, we try to thread the */resolve request to the plugin which presumably handles the request.
We only reject the request, if the _data field is given, but no plugin owner can be determined.

This behaviour differs from the current behaviour only regarding how to handle _data = Nothing case, in which case we rejected the request.

Linked issues #4467, #4451

@fendor fendor added type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. status: needs triage component: ghcide component: hls-plugin-api and removed status: needs triage labels Dec 28, 2024
@fendor
Copy link
Collaborator Author

fendor commented Dec 28, 2024

cc @michaelpj, @joyfulmantis, do you think this makes sense?

@michaelpj
Copy link
Collaborator

Yes, I agree with something like this. We didn't completely think this through: while we have differing levels of support for resolve depending on the plugin, the capability we advertise to the client is all-or-nothing. So clients quite reasonably expect to be able to resolve any completion/code action/etc. The spec doesn't say what you do if a resolve request fails, but I think "complain loudly" is not unreasonable behaviour for the client.

So what to do? I can see two options:

  1. Be dumb and permissive: if no plugin wants to resolve a request, then just respond positively with the original item! Potentially this masks real issues, but I think it's not too bad: if a plugin thinks it can handle the request but it then fails to resolve it, we should still return a failure.
  2. Try and be smart: something like what you suggest, where we try and figure out requests that we're "supposed" to resolve (e.g. those with a data field), and fail if no plugin wants to handle those. I think this is possible since we set data. So as long as we maintain the invariant that only things which need resolving get data, then it could be okay.

I think the implementation looks pretty similar: in both cases we'll need a catch-all handler, it's just that for option 2 it will decide whether or not to return an error depending on whether or not the item has a data field. So maybe it's easy enough to be a bit smarter and do 2?

@fendor fendor self-assigned this Jan 5, 2025
@fendor
Copy link
Collaborator Author

fendor commented Jan 5, 2025

I have a POC for (2), will send the PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ghcide component: hls-plugin-api type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

2 participants