Add client support for inlay hints

[HighCommander4]

No description provided.

[HighCommander4]

Corresponding server-side patch for reference: https://reviews.llvm.org/D98748. There's some discussion there about the choices made about the protocol as well.

[HighCommander4]

Sam, perhaps you would be interested in reviewing the client-side patch as well?

[sam-mccall]

Absolutely. Looks like there are three paths though:

1. a client impl based on decorations, as in rust-analyzer (current state in this patch)
2. a client impl based on vscode inlay hints (only works in insiders)
3. adding this to lsp + vscode-languageclient instead (we've been invited to do this in Feature: Inlay Hints microsoft/language-server-protocol#956)

We can take multiple of these paths, though of course that has costs.
It looks like the LSP bit may not happen soon unless we push on it, and we could speed it up a lot.

Do you want to stick with approach 1 for this patch, or try 2?
Do you want to push on LSP standardization, or should I, or should we wait and see?

[HighCommander4]

Do you want to stick with approach 1 for this patch, or try 2?

I'm not fully clear on how Insiders works. Are things that are in Insiders now going to be in the next vscode release, or some unspecified (possibly distant) future one?

In the first case (which, IIUC, would mean the API would arrive to vscode stable within the next ~4 weeks), I think it makes sense to go straight for (2). In the second case, my preference would be to stick to (1) so that we can make the feature available to users without too much delay.

Do you want to push on LSP standardization, or should I, or should we wait and see?

I'm open to this, but perhaps as a follow-up to whichever of the above options we choose?

[njames93]

I'd suggest sticking with option 1 right now. Some features stay in insiders for a long time before they are merged into the main branch.
Option 3 will require modifying the server side patch to be in line with the yet to be agreed LSP spec for this feature.

[sam-mccall]

Yeah, there's plenty of value in us being able to use this regularly now. At least it'll motivate & inform work on the server impl and LSP effort.

There's value in getting it to "real" users, but also costs: a custom protocol in client + released server (clangd 13?) needs to be supported for a long time (like semantic highlighting). Some costs start early: as soon as we add these options to the vscode plugin, they're going to cause confusion (for users whose clangd doesn't have the feature). And there are mostly no benefits until the next release.

So:

• I agree we should build something on top of stable VSCode (option 1)
• I think there's a big advantage in switching to a standard protocol soon, I can make time to push for this
• I think we should avoid advertising this feature to the bulk of users who are on stable clangd releases, and revisit this question closer to 13 release.

I don't see a good way to make the settings conditional on clangd version. What do you think about hiding the server capability behind a clangd flag, and having the client unconditionally show hints if the capability is present?

We'd lose the ability to toggle categories independently, but I'm not sure that's terribly important right now.

WDYT?

[HighCommander4]

I don't see a good way to make the settings conditional on clangd version. What do you think about hiding the server capability behind a clangd flag, and having the client unconditionally show hints if the capability is present?

We'd lose the ability to toggle categories independently, but I'm not sure that's terribly important right now.

WDYT?

I think that would be fine for now. (Presumably, we could bring back the client-side settings once clangd 13 is released?)

---

Your comment made me ponder something which is perhaps off-topic for this bug, but here goes: if most users are using the latest official clangd release, and clangd releases are tied to LLVM releases which are every 6 months, does that make the velocity at which new features are rolled out to clangd users a bit on the slow side (compared to, say, other components of the tooling ecosystem like VSCode which get a release every month)?

[sam-mccall]

I think that would be fine for now. (Presumably, we could bring back the client-side settings once clangd 13 is released?)

We could, there's a long-term cost to that (c.f. confusion about the semantic highlighting setting that doesn't control standard semantic highlighting)...

So options would include changing the server protocol to match the standard or likely standard before the 13 release, making this a public non-standard feature and maintaining it for a few release cycles, or leaving the feature flag off by default for 13 and hope to get it standardized by 14. I really hope the first option looks feasible by then...

if most users are using the latest official clangd release, and clangd releases are tied to LLVM releases which are every 6 months, does that make the velocity at which new features are rolled out to clangd users a bit on the slow side

My response got too long and is obviously a bit offtopic here. I dumped my thoughts into the wiki: https://github.com/clangd/clangd/wiki/Release-cycle. That's no good for discussion though - I've been meaning to try out https://github.com/clangd/clangd/discussions, feel free to start a thread there if we should get further into this...

[Curve]

This is a feature that I anticipated to see for some time now, is this useable in its current state (I'd like to test / use it)?

[HighCommander4]

I think that would be fine for now. (Presumably, we could bring back the client-side settings once clangd 13 is released?)

We could, there's a long-term cost to that (c.f. confusion about the semantic highlighting setting that doesn't control standard semantic highlighting)...

Good point.

So options would include changing the server protocol to match the standard or likely standard before the 13 release, making this a public non-standard feature and maintaining it for a few release cycles, or leaving the feature flag off by default for 13 and hope to get it standardized by 14. I really hope the first option looks feasible by then...

One thought is: if vscode support for inlay hints hits stable in time for 13, but the protocol piece isn't there yet, we could use vscode's setting for the feature (rather than introducing our own setting), and have the client and server negotiate which impl. they use via capabilities?

[HighCommander4]

if most users are using the latest official clangd release, and clangd releases are tied to LLVM releases which are every 6 months, does that make the velocity at which new features are rolled out to clangd users a bit on the slow side

My response got too long and is obviously a bit offtopic here. I dumped my thoughts into the wiki: https://github.com/clangd/clangd/wiki/Release-cycle.

Thanks for the detailed thoughts. I don't have much to add so I'll risk your ire by making two minor comments here:

• The out-of-tree option would also have the advantage that clangd developers could keep up with clangd head without having to rebuild all of LLVM all the time (but I also recognize the many advantages of being in-tree and on the whole we may well be better off this way).
• When I asked the question, I envisioned option 3 (periodic snapshots promoted to releases), but failed to consider clang parser stability, which I agree is important.

No easy answers, I suppose.

[HighCommander4]

This is a feature that I anticipated to see for some time now, is this useable in its current state (I'd like to test / use it)?

It is if you're willing to use a nightly or snapshot build of clangd, and apply this client-side PR locally.

[sam-mccall]

AIUI we have a plan at least for the near term, and this is waiting on updates from @HighCommander4 (earlier review comments & ripping out settings for now). Nathan please let me know if I'm mistaken!

if vscode support for inlay hints hits stable in time for 13, but the protocol piece isn't there yet, we could use vscode's setting for the feature

Yes, this sounds good!

I'll risk your ire by making two minor comments here
No ire, but I can't resist replying, so started clangd/clangd#757

[HighCommander4]

I've addressed the review comments so far, with this server-side patch adding the command-line flag.

[sam-mccall]

Looks good!

Possible this may require type tweaks after #178 (strict TS)

(I'm not good at github, but I think you have permissions to commit here right?!)

[sam-mccall]

I'd rather set up a map here from HintType -> HintStyle.
Otherwise we're adding afterHintStyle which is currently unused, and setting up an expectation that the only difference between kinds will be before vs after (I think we should do colors too, later)

[HighCommander4]

I went with a parameterHintStyle variable for now, to keep things simple.

[njames93]

Having used this, I'm noticing sometimes for whatever reason, edits aren't causing the server to recalculate and It's resulting in hints being rendered incorrectly.

Maybe we should consider changing the InlayHints at the protocol level and encode like we do for semanticTokens

struct InlayHint {
  /// Hint line number, relative to the previous hint
  unsigned deltaLine = 0;
  /// Hint start character, relative to the previous hint
  /// (relative to 0 or the previous hints start if they are on the same line)
  unsigned deltaStart = 0;
  /// the length of the source code the hint applies to. 
  unsigned length = 0;
  /// The type of hint.
  InlayHintKind kind;
  /// The label that is displayed in the editor.
  std::string label;
};

This was the editor will be better able handle cases where the server doesn't update the hints. It would also support sending deltas for the inlay hints like with semantic tokens.

Unfortunately this would diverge from rust-analysers implementation but in the long run its likely the direction we should go and makes more sense in LSP spec.

[sam-mccall]

That's clearly a real bug and needs to be tracked and fixed. Is it reproducible/minimizable?
It looks like the hints fall "further behind" as the file goes on.

However deltas won't fix this, neither delta-encoding nor requests. They may be good ideas, but we don't need to have them in the first patch. Reasoning:

This feature requires exactly two things of clients:

1. as edits happen, clients can make the labels move along with the text.
2. after sending an inlayHints request, client knows which version of the file the request (and therefore response) matches. If there are edits before the response, client either (good) adjusts the response for the edits or (bad)

Adding delta requests and/or encoding obviously doesn't fix #1, because we're not going to have fresh data for every keystroke.
Adding deltas doesn't fix #2 because the exact same thing is true for the delta response: it (indirectly) describes a set of inlay hints which is valid for one precise snapshot of the file, and only applies correctly to that snapshot.

Delta-encoding the inlay sequence, and supporting a /delta request variant, may help with efficiency in various ways, but optimizing and compressing incorrect data is just going to lead to confusion, so let's wait :-)

[njames93]

That's clearly a real bug and needs to be tracked and fixed. Is it reproducible/minimizable?

Honestly from what I can see things that cause it to get confused are basically any edit to a file I seem to make. My best guess is the hints are being generated from the previous version of the file, rather than the current version.

Hopefully this shows how they are one version behind

Final edit, that seems to be the case. The inlayHints request is indeed sent before the didChange request.

I[17:24:28.433] <-- clangd/inlayHints(10)
I[17:24:28.434] --> reply:clangd/inlayHints(10) 0 ms
I[17:24:28.434] --> textDocument/clangd.fileStatus
I[17:24:28.435] <-- textDocument/didChange
I[17:24:28.435] --> textDocument/clangd.fileStatus

[HighCommander4]

The inlayHints request is indeed sent before the didChange request.

I[17:24:28.433] <-- clangd/inlayHints(10)
I[17:24:28.434] --> reply:clangd/inlayHints(10) 0 ms
I[17:24:28.434] --> textDocument/clangd.fileStatus
I[17:24:28.435] <-- textDocument/didChange
I[17:24:28.435] --> textDocument/clangd.fileStatus

Turns out this was a bug I introduced while porting the rust-analyzer code to clangd. A close comparison of the two showed that I was registering the feature's onDidChangeTextDocument listener at feature creation time, while rust-analyzer was doing it at feature initialization time. The difference turns out to be important, as I guess adding it at creation time means it gets added before the built-in textDocument/didChange feature's onDidChangeTextDocument listener, causing the inlay-hints one to fire first (and thus send the clangd/inlayHints request) before the other one sends the textDocument/didChange request.

Should be fixed now.