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

Always handle "completion/resolve" requests #4463

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 9, 2024

Resolves the LSP "completion/resolve" requests, even if there is nothing more to resolve.
If there is nothing to resolve, we simply return the initial completion item.

If we reject "completion/resolve" requests, then some LSP clients show our rejection as pop-ups which are hugely distracting.

We change completion tests to always resolve completions, regardless of the existence of _data_, as this seems to be the behaviour of VSCode.

Closes #4451

Resolves the LSP "completion/resolve" requests, even if there is nothing
more to resolve.
If there is nothing to resolve, we simply return the initial completion
item.

If we reject "completion/resolve" requests, then some LSP clients show
our rejection as pop-ups which are hugely distracting.

We change completion tests to always resolve completions, regardless of
the existence of `_data_`, as this seems to be the behaviour of VSCode.
@michaelpj
Copy link
Collaborator

Hmmm. So what's happening? We advertise support for resolve but don't actually resolve it? Or we advertise resolve because some plugins support resolve, but not all of them do? And so when we hit that case we get a rejection?

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2024

We advertise "completion/resolve", and the completion plugin supports it, but for some completion items, there is simply no more info to resolve. But vscode still asks for "completion/resolve" which so far, we simply rejected if it was for a completion that didn't have any more info. E.g., documentation for local variables, such as in where blocks.

@michaelpj
Copy link
Collaborator

Okay, I guess I'm worried that we have the same problem in other plugins for other reasons, but I guess we can tackle that later!

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2024

We do, not with resolve in particular, but in other instances, e.g. semantic-token plugin rejects requests by default as well, since the plugin is disabled. Cabal plugin also rejects certain requests, such as Goto Type Definition, etc...

I will open an issue tracking this in general.

Add explanations and justification for 'NothingToResolve' constructor.
Also, include historical note to explain how we end up with this
'CompletionResolveData' version.
Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@soulomoon
Copy link
Collaborator

We do, not with resolve in particular, but in other instances, e.g. semantic-token plugin rejects requests by default as well, since the plugin is disabled. Cabal plugin also rejects certain requests, such as Goto Type Definition, etc...

I will open an issue tracking this in general.

Emmm, so we might need to respond with a dummy respond to these request as well ?

@fendor
Copy link
Collaborator Author

fendor commented Dec 13, 2024

Emmm, so we might need to respond with a dummy respond to these request as well ?

Yeah, that sounds like a reasonable solution. Maybe not so pretty, but solves the issue as well.

@fendor
Copy link
Collaborator Author

fendor commented Dec 20, 2024

We might be better off solving this issue by adding logic to the completion/resolve dispatch logic. For example, we could specify that by convention if the resolve request doesn't have any _data JSON, we simply return the CompletionItem.

Then the request wouldn't fail, and it would be handled at a central location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issues with CompletionItemResolve
3 participants