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

LSP 3.16: Consider implementing completionItem/resolve #6366

Closed
kjeremy opened this issue Oct 26, 2020 · 9 comments
Closed

LSP 3.16: Consider implementing completionItem/resolve #6366

kjeremy opened this issue Oct 26, 2020 · 9 comments

Comments

@kjeremy
Copy link
Contributor

kjeremy commented Oct 26, 2020

If the client supports it we should implementing completionItem/resolve

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Dec 1, 2020

If the client supports it

What is a good way to understand this?
I've checked ClientCapabilities but did not find anything solid.

I see this when RA starts and gets info from VSCode:

resolve_support: Some(
    CompletionItemCapabilityResolveSupport {
        properties: [
            "documentation",
            "detail",
            "additionalTextEdits",
        ],
    },
),

in the completion capabilities, in Rust code this is

pub struct CompletionItemCapabilityResolveSupport {
    /// The properties that a client can resolve lazily.
    pub properties: Vec<String>,
}

Is it all right to check this vec for additionalTextEdits presence when I want to be sure that I can propose auto imports?

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 1, 2020

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 1, 2020

Is it all right to check this vec for additionalTextEdits presence when I want to be sure that I can propose auto imports?

Yes. I would hang something off of our config infra that says what we can resolve lazily and check that when computing the completion. Then that could be deferred. We may need to make it generic for all completions (I haven't looked at our completion infrastructure in a while).

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 1, 2020

Oh... we also need to advertise server-side support by filling in CompletionOptions::resolve_provider so that the client knows it's supported.

@SomeoneToIgnore
Copy link
Contributor

Yep, the server side is covered, but the client side is a bit obscure tbh.

Thank you for the advices.

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 1, 2020

It is awkward. You have to read https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#textDocument_completion a bunch.

From my understanding it works like this (assuming that the server advertises resolve) on LSP < 3.16:

  1. The client makes a completion request
  2. The server sends back a completion item.
  3. The user selects the item in the list and if the completion item does not have details or documentation filled in it sends a resolve request with that completion item otherwise it uses that item.
  4. The server responds with the completion item with either details/documentation filled in.

On LSP 3.16+ step 3 is different IF the client fills in resolveSupport. In that case other items besides just details and documentation can be resolved lazily (additionalTextEdits being the big one). Note that details and documentation are specified in that list to be backwards compatible with 3.15.

bors bot added a commit that referenced this issue Dec 8, 2020
6706: Move import text edit calculation into a completion resolve request r=matklad a=SomeoneToIgnore

Part of #6612 (presumably fixing it)
Part of #6366 (does not cover all possible resolve capabilities we can do)
Closes #6594

Further improves imports on completion performance by deferring the computations for import inserts.

To use the new mode, you have to have the experimental completions enabled and use the LSP 3.16-compliant client that reports `additionalTextEdits` in its `CompletionItemCapabilityResolveSupport` field in the client capabilities.
rust-analyzer VSCode extension does this already hence picks up the changes completely.

Performance implications are descrbed in: #6633 (comment)

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>
@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 8, 2020

Thanks for doing this! I don't think we need to support documentation or detail to close this issue since they're cheap to compute. We can gradually move other completion items over to resolve as the need arises.

@kjeremy kjeremy closed this as completed Dec 8, 2020
@SomeoneToIgnore
Copy link
Contributor

I was just about to write a set of instructions on what to do next here, thanks for saving my time.

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 9, 2020

@SomeoneToIgnore feel free to open a new issue about enhancing this exciting and thrilling feature! If you have thoughts on improvements or specific instructions we probably shouldn't lose them.

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

No branches or pull requests

2 participants