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

Provide better guidance on the use of CompletionItem.detail and CompletionItem.documentation #1115

Open
DanTup opened this issue Oct 26, 2020 · 19 comments

Comments

@DanTup
Copy link
Contributor

DanTup commented Oct 26, 2020

There are two fields on CompletionItem for type/docs:

	/**
	 * A human-readable string with additional information
	 * about this item, like type or symbol information.
	 */
	detail?: string;

	/**
	 * A human-readable string that represents a doc-comment.
	 */
	documentation?: string | MarkupContent;

These comments make it seem like detail is a good place to put function signatures, but this seems to not be the case because they appear quite poorly in VS Code (no highlighting - see microsoft/vscode#106862) and the recommendation is to use documentation and leave detail blank.

It's important for servers and clients to all be on the same page to ensure things are rendered well across clients, so I think the LSP spec should have clearer guidance here. For example, if a server puts signature information in detail for VS Code it's important it is not also at the top of documentation. I know LSP doesn't want to get into UI, but if it's just silent on this, clients will behave differently and then servers are stuck picking between "some information not being visible in some editors" and "some information being duplicated in some editors".

@kjeremy
Copy link
Contributor

kjeremy commented Oct 26, 2020

the recommendation is to use documentation and leave detail blank.

Wow that's the first I've heard of this.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 26, 2020

To clarify - that was the suggestion in order to resolve the issue of not getting syntax highlighting (see this comment).

However there was a further comment here:

Also, we have plans for richer labels that allow to show things like label, signature, qualifier, and type (microsoft/vscode#39441). With that details would only be shown in the details panels and when omitted it isn't shown.

So if that's implemented and then added to LSP, it may provide a better solution all-round.

@dbaeumer
Copy link
Member

I am still the opinion that LSP itself shouldn't go into how editors should render this. That simply blocks innovation on the client side.

I do agree that we should be a little bit more specific of what should go into the details property. I think things are clear for documentation.

Any suggestions on how to improve this.

@dbaeumer dbaeumer added this to the On Deck milestone Oct 27, 2020
@rwols
Copy link
Contributor

rwols commented Oct 27, 2020

A lot of servers present completion items for functions/callables in such a way that the client/editor immediately requests signatureHelp when the completion item is inserted. e.g. a snippet like foo($0) would trigger signatureHelp if one of the trigger chars for signatureHelp is (. That would help render the signature information in a nice way, no?

@DanTup
Copy link
Contributor Author

DanTup commented Oct 27, 2020

I am still the opinion that LSP itself shouldn't go into how editors should render this

I understand this and I don't think LSP should prescribe the UI, but I think it doesn't need to provide some general guidance. Leaving clients to do their own thing will lead to them diverging and that makes it difficult for servers to know what information to put into what fields.

To get syntax highlighting of the signature (which TypeScript wants too), VS Code is recommending servers put everything in documentation and not use details. This means servers now have to decide whether to populate data based on what the LSP spec suggests, or the specific UI implementations of VS Code. This doesn't seem like an ideal situation and may ultimately lead to servers using the client information to put data into different fields to try and get the optimal user experience in each client - and that seems like a bad path to go down (and somewhat against the idea of LSP).

A lot of servers present completion items for functions/callables in such a way that the client/editor immediately requests signatureHelp when the completion item is inserted

That happens after completing, but this issue is about the information visible before you complete (eg. while you're moving through the items in the completion list, reading their arguments to see which function/method is the one that you need).

@nbfalcon
Copy link

nbfalcon commented Nov 5, 2020

This is related to #1130. We need more syntax highlighting in the various detail? fields. IMHO only relying on documentation? is not the way forward, as an editor should be able to add small signature documentation to completion items.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 6, 2020

I do agree with @nbfalcon that a better way out is to have syntax highlighting in the detailed field. We could even add this to LSP and VS Code simply sets the capability to false.

The only thing I can imagine to add to the spec is something like:

  • details: This information is usually presented in the same view as the completion item
  • documentation: this information is usually presented in a separate view closely attached to the completion list.

@rwols
Copy link
Contributor

rwols commented Nov 6, 2020

But it is not guaranteed that the detail field is tokenizable by a syntax highlighter. At this point it only says

A human-readable string with additional information about this item

which seems to suggest it should not be a parseable code snippet?

like type or symbol information.

but this does suggest it should be parseable as a code snippet :)

@dbaeumer
Copy link
Member

dbaeumer commented Nov 6, 2020

@Rowls things that are tokenizable are of type MarkedString. A normal string is never tokenizable. We even discussed if we allow servers to tokenize the string using some markup content based on semantic tokens.

@nbfalcon
Copy link

nbfalcon commented Nov 6, 2020

@rwols type or symbol information need not be parsable: imagine you have a class

class foo: public std::string {};

the server might provide the following as a detail: "(class) foo: public std::string".

This doesn't mean though that some parts of it cannot be colorized: the "std::string" part could be colorized using semantic tokens, .... However, I do agree that MarkupContent is the better way to go about this.

@nbfalcon
Copy link

nbfalcon commented Nov 6, 2020

@dbaeumer I don't see a MarkedString interface in the spec. Did you mean MarkupContent?

@dbaeumer
Copy link
Member

dbaeumer commented Nov 6, 2020

Sorry, I meant MarkupContent

@DanTup
Copy link
Contributor Author

DanTup commented Nov 6, 2020

I do agree with @nbfalcon that a better way out is to have syntax highlighting in the detailed field.

I completely agree - but I requested that in the issue linked above (microsoft/vscode#106862) and it was rejected and advised to use documentation 🤷‍♂️

@rwols
Copy link
Contributor

rwols commented Nov 6, 2020

I would be okay with the possibility to have detail be a MarkupContent. In the same way that the response to textDocument/hover can be a plaintext string or a markdown string. But it should be stated in the protocol that it should not span more than 1 line.

That said, I also noticed major slowdowns when rendering all docs upfront for all completion items. And since detail has to be provided upfront for sublime text, I fear that that would force me to use plaintext (the docs can be rendered lazily which works around the performance problems). But performance may also be adequate enough if the markdown strings are short. I'd have to experiment :)

@nbfalcon
Copy link

nbfalcon commented Nov 6, 2020

Yup, we need to try and see :). If the performance is too bad, we can incorporate my lazy :children proposal. I am personally interested in getting syntax-highlighting for lsp-treemacs-symbols (Emacs, lsp-mode).

@rchl
Copy link
Contributor

rchl commented Nov 6, 2020

the server might provide the following as a detail: "(class) foo: public std::string".

A separate conversation I guess, but that convention adopted by MS to prefix symbols with (foo) is problematic. In ST, in some syntaxes, it triggers invalid highlighting. In other cases it affects scoping and highlighting (also in VSCode).

One example from ST (python syntax):
Screenshot 2020-11-06 at 13 15 28

@nbfalcon
Copy link

nbfalcon commented Nov 6, 2020

True. However, enforcing syntactic-validity is probably the wrong idea, because it prevents nicer output, decoupled from the language syntax. I would personally prefer something like

(class) foo: public std::string

over

class foo: public std::string {}

@nbfalcon
Copy link

nbfalcon commented Nov 6, 2020

Of course, (class) should be left out for DocumentSymbol's detail?, because that's what kind is for.

@DanTup
Copy link
Contributor Author

DanTup commented Apr 26, 2022

Another reason to clarify/spec this.. VS Code is extracting hex codes from label/detail/documentation to render color previews in the completion list. This only works of the hex code is the only thing in those fields. This means (today) that some other information may need to be dropped from an existing field to get the color preview.

However, in the latest VS Code, if you use the new label.description to override the old detail to avoid that:

const newDesc = new CompletionItem({ label: "Colors.blueAccent", description: "MaterialAccentColor" });
newDesc.detail = "#448AFF"; // this is hidden by "description" above but still causes the color preview in VS Code
newDesc.documentation = "docs for this color";

It's a little wonky for servers to be relying on this unspecified behaviour, so having some guidelines/hints in LSP would be helpful. Without them, servers either have to not have this functionality, or just assume VS Code is the spec instead of the LSP spec and I don't think that's a great thing.

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

No branches or pull requests

6 participants