-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Implement fallback handler for */resolve
requests
#4478
base: master
Are you sure you want to change the base?
Conversation
We had 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. So what to do? We 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 may not be 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: we try to 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. 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. In 'fallbackResolveHandler', we implement the option (2).
SMethod_InlayHintResolve | ||
| noResolveData params -> Just params | ||
SMethod_CompletionItemResolve | ||
| noResolveData params -> Just params | ||
SMethod_CodeActionResolve | ||
| noResolveData params -> Just params | ||
SMethod_WorkspaceSymbolResolve | ||
| noResolveData params -> Just params | ||
SMethod_CodeLensResolve | ||
| noResolveData params -> Just params | ||
SMethod_DocumentLinkResolve | ||
| noResolveData params -> Just params | ||
_ -> Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern matching is bad for long-term maintenance. Anyone an Idea on how to improve this? If we do a string comparison ala isSuffixOf "/resolve" (someMethodToMethodString ...)
, then I don't see how to actually implement the check, as we need a proof for JL.HasData_ p (Maybe a)
where p ~ MessageResult s
.
I have an idea for DRY'ing up noResolveData params
via Dict
, but it introduces a bit of complexity I am not sure is worth it.
We had multiple reports, where
resolve
requests (such ascompletion/resolve
andcodeAction/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 ofCodeAction
/Completions
, when they contain particularly expensive properties, such as documentation or non-local type signatures.So what to do? We can see two options:
In 'fallbackResolveHandler', we implement the option (2).
Supersedes #4463 and presumably fixes #4451 and #4467