Add textDocument/inlayHints

[sam-mccall]

First draft, several points left open, marked as TODO and feedback sought. I do plan to do a VSCode client implementation but wanted to get feedback on design choices first.

Fixes #956, and mostly inspired by rust-analyzer's inlayHints.

This retrieves annotations, it's structurally similar to diagnostics, semantic tokens, outline etc. It's a pull request rather than push because that seems to be the preference for recent LSP changes (semantic tokens, pull-diagnostics).

This is a separate request rather than combining with codeLens. The overlap doesn't seem huge to me and I'm worried about creating a very complicated feature that various editors support different "profiles" of.

The main UI idea is to display the annotations permanently in little "chips" that appear in the source code. But other presentations are possible, e.g. a small tooltip-like panel that appears when the cursor is in the target area. The separation of prefix/suffix is mostly to enable alternate presentations.

This does not support hints that replace existing text, as described in microsoft/vscode#113285 (comment). It IMO adds too much complexity for too little value, but could be revisited later.

• it's not supported in vscode's inlay hints impl now, and is challenging to implement in editors
• it essentially requires a different interaction (e.g. a hotkey to enable)
• servers will need to juggle client capabilities and user preferences in deciding how to expose hints

Hint Classes: I suspect the idea of hint classes is either too big or too small. It's meant to express the commonality between e.g. how type-chaining hints in Java should look. But probably either it should grow to something user-visible that can be toggled on/off, or disappear as a premature wire-size optimization.

Code actions: this isn't spelled out in the text, but I imagine we could support "code actions on hints" (e.g. spell out inferred type) in a lightweight way by triggering a codeAction request on the target range. This is a small part of the motivation for having the target range.

Invalidation: This doesn't support fine-grained invalidation of the kind @Veetaha proposed in #956, IME a coarse-grained server->client request like workspace/semanticTokens/refresh is enough when you really need this, despite the very coarse granularity (and you can get a long way without invalidating at all). But interested in practical experience here.

cc @dbaeumer @matklad @SomeoneToIgnore @kjeremy @Veetaha @HighCommander4

[ghost]

All CLA requirements met.

[SomeoneToIgnore]

Thanks for finding time for kicking-off the development process here!

I wonder, would it make sense to prepare some building blocks for the dynamic capabilities to arrive later?
I could think of two use cases:

• hint folding and unfolding
  In Rust, types can get lengthy and rust-analyzer server is able to send the shortened versions of those, but currently there's no way to expand those easily

• extra hint info/navigation to definition
  Another commonly used feature with rust-analyzer hints is to navigate to the definition of the type in the hint (in Rust, there can be many definitions in a single hint due to Beautiful<Rust<Generics>>)
  For now, we place such definitions and "go to actions" into the corresponding element's hover info, but to me it looks more like a workaround rather then a proper solution.

See intellij-rust/intellij-rust#4217 for inspiration on both from JetBrains, where a user can click the hint to unfold the shortened info and cmd/ctrl + click to navigate into hint definition under the caret.

[mickaelistria]

We have "inline annotations" implemented in Eclipse IDE and it would be quite easy to bind LSP provided inlayHints to those.
However, I have the impression that in the current form, the specifications makes things more complicated that it could be. I particularly think that the "Categories" are making things more complex for a 1st iteration and that the vast majority of cases could be covered without them. So I'd recommend leaving them aside them for the moment and maybe reintroduce them in further versions if feedback supports this addition.

Also, compared to what we have in Eclipse IDE and what can be done with CodeLens, it would be good to add support for a command (or even a codeAction) from now on to allow some of those annotations to react to user clicking them.

[mickaelistria]

Are those categories necessary for a 1st iteration? I don't see categories in "Hover" nor "CodeLens" for instance. I think a 1st iteration should take place without them.

[sam-mccall]

Hmm, good question. These are present in both VSCode's proposed API and rust-analyzer (though Rust's categories have some overlap with classes, too)

I see two main purposes:

• allow users to selectively turn on/off hints by category. (This could also/instead be done by hintClass, but it seems more complex).
• style hints differently (e.g. a subtle color tint on the chips). This seems useful but I haven't seen it in the wild, maybe it isn't?

I find the configurability argument quite compelling, as this is a highly invasive/visible feature.
Categories don't add a lot of spec complexity, and are optional for both client & server.
So I'm inclined to include them but not wedded to it, definitely want to hear more opinions.

[mickaelistria]

I find the configurability argument quite compelling, as this is a highly invasive/visible feature.

The thing is that to properly handle it, it would require tools to dynamically generate some page which lists the available categories in the LS and allow to enable/disable some of them. The LS would require to allow more introspective than usual; categories/kinds are usually hardcoded pre-identified enums of things shared by many languages more than dynamically filled placeholders for LS-specific stuff.
If we need to distinguish different kinds, I think it's better to do it just like Completion and other do: the protocol defines a static list of supported kinds; that clients can then code a behavior for.

[sam-mccall]

Oh, that was definitely the intention here: clients could show fixed checkboxes for Params/Types/Other.
(In fact I thought about using classes for configuration, but it seems too complicated for exactly the reason you mention)

The reason to allow other values is just to make protocol versioning/extensions easier:

• avoid deadlock where neither clients/servers/spec feel they should be the first to support a new category
• avoid complicated capability negotiation (completionItemKind.valueSet) to ensure back-compatibility
  It's possible different servers use the same non-standard category name to mean different things, I don't think this is a large risk.

[sam-mccall]

(I've tweaked some comments and such, trying to make this clearer)

[sam-mccall]

Thanks for the great feedback! I won't rush to edit everything just yet, but some thoughts...

@SomeoneToIgnore

I wonder, would it make sense to prepare some building blocks for the dynamic capabilities to arrive later?

This is a really tough one, I agree we should know where we're going. The intellij-rust demo is really compelling. There are some challenges:

• there's a lot of structure here, we'll need an intricate spec, which will take a while and run some risk of derailing the effort. So there's value in deferring if we can.
• there are many hints in a doc, so inlining complex structure into them will produce huge responses which seem likely to cause issues. And needing to efficiently provide go-to-def targets for every hint is constraining. So some of this should be outside the main request I think.
• these interactions doesn't exist in the current ("proposed" but in-tree) VSCode inlay hint API. Not having a VSCode reference implementation is a practical obstacle, and not something I can personally work on.
• more generally, supporting structure is going to be a challenge for clients, and some won't. I think it's important the structure is clearly "layered" to encourage basic inlay hint support.

With this in mind, my best idea is:

• we define a basic and extended model for hints.
• The basic model is plain text e.g. Outer<...> with no links, as in this PR. (Enough for display only)
• The extended model adds a link target, an "expanded text" with anchored links, code actions (all optional)
• textDocument/inlayHints/resolve takes you from a basic hint to an extended hint, would typicaly be called on mouseover
• resolve support is optional for both client and server

Do you see a better design?

If this is what we're aiming for, I think resolve is best added later and I can't see that it needs any explicit preparation now. Deferring this will simplify landing the basic feature, force the dynamic bits to be truly optional, and buy time for VSCode support.

@mickaelistria

However, I have the impression that in the current form, the specifications makes things more complicated that it could be.

Point well taken, I'm going to leave some bits in for now not to be argumentative but to give others a chance to comment.

it would be good to add support for a command

I think this is a good idea, do you think it's important to have in scope for v1?
My feeling is this has a rich set of use cases even without action support. Code actions fits naturally into a "resolve" type request, and deferring all interactions is a good way to limit scope.
(My personal feeling is that codeLens is too tightly bound to commands. I think the eventual design of interactive hints can benefit from some experimentation with basic hints)

[sam-mccall]

Hmm, good question. These are present in both VSCode's proposed API and rust-analyzer (though Rust's categories have some overlap with classes, too)

I see two main purposes:

• allow users to selectively turn on/off hints by category. (This could also/instead be done by hintClass, but it seems more complex).
• style hints differently (e.g. a subtle color tint on the chips). This seems useful but I haven't seen it in the wild, maybe it isn't?

I find the configurability argument quite compelling, as this is a highly invasive/visible feature.
Categories don't add a lot of spec complexity, and are optional for both client & server.
So I'm inclined to include them but not wedded to it, definitely want to hear more opinions.

[SomeoneToIgnore]

@sam-mccall the resolve approach totally makes sense to me and deferring feels the right way for now too.
I would imagine, it could be worthwhile to mention those plans in the docs for the future implementators?
(but feel free to skip that too, if it's not a common practice here)

[matklad]

For categories, the primary value is ability to share configurations between different languages. The user can configure "show parameter hints, but not type hints" once, and that would work both for C++ and Rust. Without categories, each extension would have it's dedicated config.

[mickaelistria]

command [...] I think this is a good idea, do you think it's important to have in scope for v1?

You're right, it's ok to consider it later.

My personal feeling is that codeLens is too tightly bound to commands.

+1, and I think it's also one more reason I now agree we shouldn't rush into specifying interactions, as it's a tricky enough topic, and there is already enough trickiness to deal for the moment with just showing inlayHints.

[sam-mccall]

@dbaeumer Friendly ping on this - it'd be great to get your rough feedback.

If the direction looks right then I can start on a VSCode binding and hacking up clangd to emit this format.

[dbaeumer]

@sam-mccall sorry for the delay but I am pretty busy with non LSP things right now. I will have a look tomorrow but it will not be a full review.

I think you can certainly start on an implementation for VS Code given that there is some proposed API in VS Code for this. But I do know that that API will very likely change as well.

[dbaeumer]

I discussed the proposal with @jrieken and here is a first round of feedback which will also have impact on the VS Code API itself

• we propose to separate InlayHints from OverlayHints. This will make it more clear how certain properties should be interpreted. For example a InlayHint would then only have a positions and the position itself can be used to place the text correct (before or after a range). A OverLayHint will have a range denoting the range to be overlayed. Such a result wouldn't need a position.
• we strongly opt to only support InlayHints on a range (not making the range optional). For semantic tokens we have two different requests (e.g. range and full) because only full supports incremental updates. What would be the use case for requesting inlay hints for the whole document?
• if we have InlayHints with only a position and a text is it still worth having InlayHintClass. I would argue that the saving is so minimal that it will not make a difference in real life.

I am out of office next week. So please don't expect a response before the week of May 10th.

[sam-mccall]

Thanks Dirk - no problem at all for the delay, I know you're busy.
I just wanted to plan my own time for this and make sure it doesn't get lost.

Thanks for the feedback. I'll update the design & also work on a VSCode impl - the simplifications make that less daunting :-)

---

Inserting vs replacing text

we propose to separate InlayHints from OverlayHints

If I understand right, this means hints that replace text are entirely out of scope (they'd be a different feature). That certainly simplifies things, I'll update.

---

Range-restricted requests

we strongly opt to only support InlayHints on a range (not making the range optional)... What would be the use case for requesting inlay hints for the whole document?

Happy with whatever you decide, but here are the reasons I lean towards optional

Client-side:

• the editor's extension model may not support fine-grained viewport tracking (quickly & accurately)
• it's significantly more complicated to implement and debug. Being able to speak a simplified version of the protocol has ecosystem benefits.
• if you scroll quickly + server isn't instant, inlays must "pop" into view. Full-file fetch mitigates this.

(Clients can of course simply send the full range explicitly. This feels a little inconsistent with the rest of LSP to me.)

Server-side:

• If the server can't handle range requests more quickly than partial requests, then full requests will yield better client responsiveness and lower server load when scrolling. (clangd implements semanticTokens/full and /full/delta but not /range for this reason - scheduling and monolithic parsing dominate)

Misc/protocol:

• maybe we want a delta request here too? This does have similarities with semanticTokens. (I didn't define one, or a packed encoding, to keep things simple)

---

if we have InlayHints with only a position and a text is it still worth having InlayHintClass

I think you're right, InlayHintClass is complex and no longer justified. The non-size things we lose:

• alternate UI presentations (e.g. popup) aren't quite as clean, as punctuation will be "baked in". But most UI ideas I can think of also require ranges.
• ability to use classes as finer-grained language-specific toggleable categories.
  I'll remove it

[matklad]

Range-restricted requests

My thoughts here:

• I don't think that mandatory ranges necessary make the thing more complicated -- a simple client can always ask a full range, a simple server can always compute the full range and do the filtering at the end. Though, I do see how folks might rush doing the complex thing first, instead of starting with a simple one and checking if itis fast enough.
• I think "scrolling" issue is a bit more complicated. Imo, range is most useful when the programmer invokes "goto definition" and lands into a new file with cold caches. Here, it's important to hightlight&hint the viewport with as low latency as possible. For the "steady state", like typing or navigating within the same file, I'd expect the editor to just always issues a full query after an edit, and use adjusted, slightly stale results between the edit and the arrival of the response. Not sure if any clients actually implement this strategy though :-)
• Depending on the language, I think viewport latency wins might be substantional. I think viewport optimization indeed won't be to benefitial for C++ (completely offtopic, but, @sam-mccall, I wonder if my description of how C++ IDEs work from this article is close to the mark?). For Rust, I think we should be able to take advantage of it quite easily. We already type-check functions lazily. In the future, we might be able to push laziness even further, such that we don't even parse functions outside of the viewport. Sadly, I can't give any hard numbers here, as for us optimizations of the constant are a pipe dream still, and we are in the "let's maybe not be O(N^2) her, okay?" phase :)

[HighCommander4]

• we propose to separate InlayHints from OverlayHints. This will make it more clear how certain properties should be interpreted. For example a InlayHint would then only have a positions and the position itself can be used to place the text correct (before or after a range).

A range can still be useful for inlay hints. For example, an editor can choose not to show a hint all the time, but only when the cursor is over the range to which the hint pertains (for example, for a parameter hint, when the cursor is over the argument expression).

[sam-mccall]

Sorry for the long delay everyone - I had an unexpected personal situation come up.

I've pushed most of the requested simplifications, and will now try to get VSCode support working.

(I've made range-restrict support mandatory for servers, but allow it to be omitted in the request as shorthand for "full doc". This seems pretty cheap & consistent with the rest of LSP, but I'm not going to die on this hill either...)

I agree some of the cut features are useful, but simplicity is a feature too.

@matklad Re architecture: you're exactly right for clangd & libclang at least. However the in-file analysis isn't trivially fast because it can instantiate templates, so we still go to some trouble to prebuild & reuse ASTs. And implementation decisions in clang mean ASTs can't be shared between reader threads, so we end up serializing requests to achieve this reuse...

[sam-mccall]

microsoft/vscode-languageserver-node#772 implements this in vscode-languageclient as a "proposed" feature.
(i.e. extensions need to call client.registerProposedFeatures(), to enableProposedApi in package.json, and to be run in vscode insiders under the debugger)

[dbaeumer]

I was pretty busy with other stuff in Mai. I will try to spare some time for this in June.

[rcjsuen]

For categories, the primary value is ability to share configurations between different languages. The user can configure "show parameter hints, but not type hints" once, and that would work both for C++ and Rust. Without categories, each extension would have it's dedicated config.

Does it make sense for the client to declare what categories it supports and for the server to respond with theirs? This would be similar to what happens with code actions right now?

[sam-mccall]

I think this PR is obsolete per se as the spec will first go as "proposed" into vscode-languageserver-node in microsoft/vscode-languageserver-node#772.
A bit awkward that the discussion is in two places, sorry about that! I'll leave it to @dbaeumer to close this when appropriate.

Does it make sense for the client to declare what categories it supports and for the server to respond with theirs? This would be similar to what happens with code actions right now?

It makes sense but I'm not sure it's terribly useful. What would you expect a client/server to do differently based on knowing what categories its peer understands?
I'm inclined to leave out this complexity at least initially if it's only marginally useful, but maybe I'm lacking imagination.

[rcjsuen]

Does it make sense for the client to declare what categories it supports and for the server to respond with theirs? This would be similar to what happens with code actions right now?

It makes sense but I'm not sure it's terribly useful. What would you expect a client/server to do differently based on knowing what categories its peer understands?
I'm inclined to leave out this complexity at least initially if it's only marginally useful, but maybe I'm lacking imagination.

@sam-mccall I'm just suggesting it so that LSP clients can render a UI with checkboxes of what inlay hints to show or not show. That was all I was going for with this suggestion. But I also understand why we'd want to keep things simple initially.

[sam-mccall]

@sam-mccall I'm just suggesting it so that LSP clients can render a UI with checkboxes of what inlay hints to show or not show.

They can do this anyway for the categories specified in LSP and "other". There's some value in more fine-grained/dynamic config, but dynamic config options based on querying the server are complicated (not sure if VSCode supports them).

The alternative of using server-specific config for server-specific customizations seems like it might be better in practice.

[mickaelistria]

I still believe that this "categories" part is not a requirement from now on and isn't a requirement to enable inlayHints. Actually categorization of content can apply to the vast majority of the operations (completion, hover, diagnostics). I think this problem of categories would better be considered separately to give more chances of inlayHints to get in soon.

[dbaeumer]

A first draft version of the inlay hint support is now available here: https://github.com/microsoft/vscode-languageserver-node/blob/582ea52183d8a7c56fa617974de5cce8a322f0f7/protocol/src/common/proposed.InlayHints.ts#L6

[KamasamaK]

The first draft was added to the spec in c63b5f5

[KamasamaK]

@dbaeumer Since you already committed this feature yourself, this PR is no longer needed.

[dbaeumer]

Correct. Will close the PR