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

Declare labelDetailsSupport client capability #4644

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Dec 17, 2024

While #3495 claimed labelDetails was implemented, it really wasn't because the client capability was never declared, so AFAICT, servers do not send them down.

Servers tested against:

  • pyright (server does not support labelDetails)
  • typescript-language-server (server does not support labelDetails)
  • gopls (server does not support labelDetails)
  • jdtls (server supports labelDetails fully, spec compliant)
  • clangd (server supports labelDetails, but the CompletionItemLabelDetails#description property is never filled in, so it's a noop as far as lsp-mode is concerned, but still spec compliant)
  • rust-analyzer (server supports labelDetails, but the signature is in CompletionItemLabelDetails#description, not spec compliant)

P.S. For jdtls, the annotation function will return a really long partly qualified method signature for CompletionItem#detail, concatenated to the return type, which is the CompletionItemLabelDetails#description. To avoid this verbosity and duplication of information, the users are advised to do this:

;; or java-ts-mode for emacs 29
(add-hook 'java-mode (lambda () (setq-local lsp-completion-show-detail nil)))

P.P.S rust-analyzer's implementation for labelDetails has always been wrong in all channels as of 2024-12-17, but wrong in a way that incidentally can alleviate #4591. If and when the R-A team decides to collectively sit down, study and implement the spec properly, we can see if there's any adjustment we need to do, but otherwise I assume the user can do their own adjustments similar to jdtls.

@wyuenho wyuenho changed the title Declare labelDetailSupport client capability Declare labelDetailsSupport client capability Dec 17, 2024
@wyuenho wyuenho force-pushed the labelDetailsSupport branch from ac31b09 to 650e8c9 Compare December 17, 2024 19:08
@wyuenho wyuenho force-pushed the labelDetailsSupport branch from 650e8c9 to 965e81e Compare December 20, 2024 14:33
@jcs090218 jcs090218 merged commit ffc0f56 into emacs-lsp:master Dec 20, 2024
7 of 13 checks passed
@jcs090218
Copy link
Member

Thanks!

@dgutov
Copy link
Contributor

dgutov commented Dec 21, 2024

Here's an example of rust-analyzer's completions output:

    {
      "label": "abort()",
      "labelDetails": {
        "detail": " (use std::intrinsics::abort)",
        "description": "fn() -> !"
      },
      "kind": 3,
      "detail": "fn() -> !",

The corresponding Company popup looks like this:

image

Here's the same in VS Code:

image

Could labelDetails override detail in a way that the annotation is computed as, to simplify,

(when label-details?
  (concat
   " "
   (lsp:label-details-detail label-details?)
   (and (lsp:label-details-detail label-details?)
        " ")
   (lsp:label-details-description label-details?)))

That leaves the issue of not having the last part aligned to the right, but other than that it seems the author's intent that the most prominent note is about the package not having been imported yet. And when you pick that completion, the use statement gets added, and the labelDetails.detail field goes away for that completion, leaving labelDetail.description more prominent.

@wyuenho
Copy link
Contributor Author

wyuenho commented Dec 21, 2024

the author's intent that the most prominent note is about the package not having been imported yet

That's a bug in R-A under discussion in the zulip thread. They've reversed the detail and the description.

On R-A stable, I guess you can turn off lsp-completion-show-detail or lsp-completion-show-label-description for now.

@dgutov
Copy link
Contributor

dgutov commented Dec 21, 2024

That's a bug in R-A under discussion in the zulip thread. They've reversed the detail and the description.

That might be true, but do you disagree that the RA popup's behavior in VS Code seems to make sense? The hint about the use declaration to be added seems helpful.

@wyuenho
Copy link
Contributor Author

wyuenho commented Dec 21, 2024

Yes and no. I see what they are trying to do, but the constraints of the LSP spec doesn't allow them cramp everything into the completion item structure. They are going to have to figure out some combination of appending to the label, detail and label details for the kind of things they want to achieve. Besides the types, they have package FQN, doc alias, traits, and snippet descriptions. There are probably more that I'm missing but trying to fit them all into 3 - 4 properties is going to be a challenge and they will have to make some tradeoffs.

@dgutov
Copy link
Contributor

dgutov commented Dec 21, 2024

Yes and no. I see what they are trying to do, but the constraints of the LSP spec doesn't allow them cramp everything into the completion item structure.

Sorry, I don't understand.

Besides the types, they have package FQN, doc alias, traits, and snippet descriptions. There are probably more that I'm missing but trying to fit them all into 3 - 4 properties is going to be a challenge and they will have to make some tradeoffs.

They use the extra two properties documented in CompletionItemLabelDetails, which VS Code picks up and shows.

Why shouldn't we show them too? As of now, we never show the value contained in lsp:label-details-detail.

@wyuenho
Copy link
Contributor Author

wyuenho commented Dec 21, 2024

we never show the value contained in lsp:label-details-detail

I've updated to #4625 to show it just a couple of hours ago. I'm testing with various servers, bear with me :)

In short, even with the latest in #4625, it's not enough to show everything they want to show. They'll have to make some sacrifices in R-A. That's story for another day, or you can look at their code if you've got the time.

@dgutov
Copy link
Contributor

dgutov commented Dec 22, 2024

I've updated to #4625 to show it just a couple of hours ago. I'm testing with various servers, bear with me :)

Great! Thanks.

@ensc
Copy link

ensc commented Jan 1, 2025

not sure whether this is this change or something else, but at least for rust this is degression in usability.

E.g. now, I see

image

After partly reverting this patch (the lsp-mode.el change) I get the original

image

which lists all possible Result types with a hint from where they are. Now, I see only three Result candidates.

Inspecting IO log shows that LSP server returns a full list:

[Trace - 12:23:06 PM] Received response 'textDocument/completion - (478)' in 22ms.
Result: {
  "isIncomplete": true,
  "items": [
    {
      "label": "Result<…>",
      "labelDetails": {
        "detail": "(use clap::error::Result)"
      },
    ...
    {
      "label": "Result",
      "labelDetails": {
        "detail": "(use std::fmt::Result)"
      },
    ...
    {
      "label": "Result<…>",
      "labelDetails": {
        "detail": "(use std::io::Result)"
      },
    ...
    {
      "label": "Result<…>",
      "labelDetails": {
        "detail": "(use std::thread::Result)"
      },
    ...
    {
      "label": "Result<…>",
      "labelDetails": {
        "detail": "(use crate::Result)"
      },
    
  • lsp-mode 20241231.1934, Emacs 29.4, gnu/linux
  • rust-analyzer 1.85.0-nightly (d117b7f 2024-12-31)

@wyuenho
Copy link
Contributor Author

wyuenho commented Jan 1, 2025

@kiennq can we merge #4625 yet? The spec conformant fix for labelDetails is in there

@ensc please give #4625 a try

@ensc
Copy link

ensc commented Jan 2, 2025

@ensc please give #4625 a try

thx, looks good with this PR...

image

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.

5 participants