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

Feature: Inlay Hints #956

Closed
kjeremy opened this issue Apr 8, 2020 · 49 comments
Closed

Feature: Inlay Hints #956

kjeremy opened this issue Apr 8, 2020 · 49 comments
Labels
feature-request Request for new features or functionality new request
Milestone

Comments

@kjeremy
Copy link
Contributor

kjeremy commented Apr 8, 2020

I would like to upstream rust-analyzer's inlay hints feature. This allows an editor to place annotations inline with text to display parameter names, type hints etc.

See: microsoft/vscode-languageserver-node#609 for the proposal.

@rcjsuen
Copy link
Contributor

rcjsuen commented Apr 8, 2020

var someNumber = 123;

@kjeremy Given the above JavaScript code, the range in the returned InlayHint would be { start: { line: 0, position 4 }, end: { 0, position: 14 } } so that it encompasses the someNumber variable, correct?

Can you provide examples of a parameter hint and a chaining hint?

@Veetaha
Copy link

Veetaha commented Apr 8, 2020

What we do in rust-analyzer is:

  1. Send a request rust-analyzer/inlayHints on any source change (i.e. any character typed, any file moved/created/deleted etc.).
  2. As a parameter we pass { textDocument: TextDocumentIdentifier } for the particular file we need to get inlay hints for.
  3. The server responds with an array of InlayHint objects for the requested file.

Here is a simplified, stringly-typed version of it (we use a discriminated union at our repo instead):

import * as lc from "vscode-languageclient";

export interface InlayHint {
    range: lc.Range;
    label: string;
    kind: string;
}

export interface InlayHintsParams {
    textDocument: lc.TextDocumentIdentifier;
}

const inlayHints = new lc.RequestType<InlayHintsParams, InlayHint[], unknown>(
    "rust-analyzer/inlayHints"
);

The client-side implementation for this feature is very straight-forward and actually fits into < 250 lines of code. It currently uses the vscode text decorations feature (more precise inlay hints are after and before css pseudoelements), but this feature is very limited.
Current limitations:

  1. It feels unnatural when the cursor goes through the inlay hint, it's hard to describe it better be seen:
    inlays_limitations
    The cursor kinda jumps through the hint and the nearby token. IntelliJ, for example, emulates the inlay hint being a real holistic token in the editor.
  2. As you can see on the .gif these after and before pseudoelements inherit the styling from the token they are attached to. This means whenever that token (e.g. the variable name, a paren, a coma etc.) changes its style (e.g. is highlighted, outlined, etc) the inlay hint changes its style along with it, which is unwanted...
  3. Text decorations don't let you tweak too much of their style. For example, we cannot change their font size to make them a bit smaller and vertically center them in the middle of the line of code.

Our current implementation can be very easily generalized and integrated into LSP:

  1. The inlay hint kind should not be a fixed enum (I propose to change it in this PR @kjeremy), but just some string identifier for which the user can customize display styling. This gives lsp server developers the flexibility to add new kinds of inlay hints that they can think of without changing the protocol, and the editor should be abstract and not provide any ad hoc implementation (but we might consider the protocol to specify the well-known inlay hint kinds where special support for them may be implemented (e.g. by clicking on the type hint the editor invokes goto-definition of the type, or by clicking on the parameter name hint it invokes goto-definition of the parameter on the function that is invoked, etc.)
  2. The server itself should decide to which token and on which side (after/before) of the token the editor should place the inlay hint. So we might modify the InlayHint interface:
    interface InlayHint {
        range: lc.Range;
        kind: string;
        label: string;
        position: "after" | "before";
    }
  3. The strategy for invalidating the inlay hints should also be tweakable. For example, the server should register the inlay hints capability with the following options for invalidation.
    Just a naive scratch of invalidation events:
    enum InlayHintsInvalidationEvents {
        TextDocumentContentChanged = 0b0001,
        WorkspaceFileRemoved = 0b0010,
        WorkspaceFileAdded = 0b0100,
        WorkspaceFileMoved = 0b1000
    }  

So this describes the pull-based implementation of inlay hints that is currently used at rust-analyzer.
We might think of a push-based implementation instead, where the server can decide whether inlay hints are invalidated or not, but I guess we cannot easily implement it via only LSP protocol right now, this might require some more changes to the protocol to let the server always know which text documents are opened in the editor at any point of time.

Maybe @matklad can elaborate on Intelij where they've natively implemented this feature and which model/API they've considered best to be used.

@Veetaha
Copy link

Veetaha commented Apr 8, 2020

@rcjsuen the chaining hint is a type hint that is displayed after each chain of the multiline method call.

Example

image

@Trass3r
Copy link

Trass3r commented Jul 10, 2020

Linking this to microsoft/vscode#16221.
Edit: Looks like it didn't work. Locking the thread even disables the 'mentioned this' links 🙄

@mickaelistria
Copy link

Isn't it a dup of #405 ?

@Trass3r
Copy link

Trass3r commented Jul 10, 2020

Well they designed it to be separate from CodeLens but there is indeed an interesting overlap.
Like this proposal is lacking commands that could be useful for click actions.

ccls for example implemented a custom solution for inline CodeLens as well.
Maybe extending codelens instead with an inline parameter could do the job.

@mickaelistria
Copy link

And isn't it also a dup of #745 ?
The current proposal seems quite complex to me while the protocol seems to already have something very similar that can be enriched.

@matklad
Copy link
Contributor

matklad commented Jul 10, 2020

Yup, seems like there are at least four threads of discussion about LSP:

It makes sense to do some issue garbage collection at least :-)

@matklad
Copy link
Contributor

matklad commented Jul 10, 2020

Enhancing code lens request seems fine by me! We need to:

  • add a label so text can be drawn without command
  • add a render_position: 'above' | 'before' | 'after' | 'lineEnd' to control where, relative to the range, the annotation should be rendered
  • add a kind such that the client can, eg, show parameter hints but not type hints.

@mickaelistria
Copy link

add a label so text can be drawn without command

+1

add a render_position: 'above' | 'before' | 'after' | 'lineEnd' to control where, relative to the range, the annotation should be rendered

I think we only need "above" and "before". "after" is actually "before" the next character; "lineEnd" is before line break. For the case of the lineEnd or the last line, we can just say that if position reaches file end, it's the lineEnd of last line.

add a kind such that the client can, eg, show parameter hints but not type hints.

I don't think filtering should happen on client side. Instead, clients should send "didChangeConfiguration" request to inform the LS about which annotations it's interested in, so the LS can then avoid some useless computation and send smaller messages.

@HighCommander4
Copy link

If this is shoehorned into code lens, it would be really good for the user to have a way to configure, that only this kind of code lens is shown. For example, I find that render_position: above code lens breaks the vertical spacing of lines and would prefer to never show those.

@matklad
Copy link
Contributor

matklad commented Jul 10, 2020

I think we only need "above" and "before". "after" is actually "before" the next character;

So I think there are two ways we can spin this. First, the range might mean "where to draw the annotation", and then indeed we don't need many positions. We don't need the range even, as we only need to point to a single position between chars. Second, the range might be semantic (the piece of source code that is being annotated), and then we do need position to specify where we render the annotation, relative to the bit of code.

I lean towards the second approach, as it unlocks interesting possibilities (for example, the client can render the annotation only when the cursor is on an annotate element) and just seems more proper, as it doesn't encode to fine representation details into the protocol.

I don't think filtering should happen on client side. Instead, clients should send "didChangeConfiguration" request to inform the LS about which annotations it's interested in, so the LS can then avoid some useless computation and send smaller messages.

Leaning on the server-specific configuration would prevent users from enabling/disabling parameter hints for all languages with a single config. I feel we should do the same pattern we use for code actions -- client & server negotiate supported kinds, and then client can send only in the request. This still allows server to save some work.

More generally, while implemented the server I've noticed that it generally is a good idea to just pass configuration as a parameter to request, instead of relying on some ambient state. This is primarily because state makes everything painful, but I also see concrete tangible benefits with per-request filtering. For example, one of the biggest problem with inlay hints is that they are "annoying". I can imagine a client implementing "flash hits" action, which would show all hints momentarily, and dismiss them afterwards. With only, the client only needs to request all the hints at a point in time. With config, the client needs to set config, send a request, and reset config back (possible interfering with requests for other files).

@sam-mccall
Copy link
Contributor

VSCode has a proposed API for this now: https://github.com/microsoft/vscode/blob/main/src/vs/vscode.proposed.d.ts. @dbaeumer does this mean anything for the timeline of having this in LSP?

@HighCommander4 is working on this for clangd in https://reviews.llvm.org/D98748, mostly following the rust-analyzer protocol. Our hope is to converge with whatever gets standardized.


Range is different between the two models:

  • The VSCode API seems to use "range" to mean "which text to replace with the hint", which would often be empty. Being able to do this seems useful: e.g. in C++ we may want to replace auto with the concrete type, or have parameter hints replace hand-written annotations like foo(/*bar=*/42)
  • The rust-analyzer API uses "range" to mean "which text the annotation applies to". This seems useful too! (@matklad's arguments above)

You could accommodate both by using rust-analyzer's range, adding position = "before"|"after"|"overwrite". (The "above" and "lineEnd" values described above aren't in VSCode currently)


It seems like it'd be useful at some point to be able to "target" these for code actions (like diagnostics). In particular, to be able to make some deduced info explicit in the source code (spell out deduced types, annotate parameter names). But this seems like something that can reasonably be bolted on later.

@KamasamaK
Copy link
Contributor

KamasamaK commented Mar 4, 2022

VS Code has shipped with a finalized native API for this.

@KamasamaK
Copy link
Contributor

@dbaeumer This is missing the 3.17 milestone

@dbaeumer dbaeumer modified the milestones: Backlog, 3.17 Apr 7, 2022
@dbaeumer
Copy link
Member

dbaeumer commented Apr 7, 2022

Correct and closing.

@DanTup
Copy link
Contributor

DanTup commented Jun 13, 2022

After waiting a while for this, I was disappointed to find (and surprised that I'd missed it before) the Kinds here are quite restrictive (type/parameter names). I'd been hoping to use it for these labels we show in Dart (to help relate closing parens to where they're opened):

image

Currently they're rendered using Decorations and custom messages from the LSP server, but I'd really like them to be something standard so other clients can use them without implementing custom messages (something that's unlikely to happen in editors that don't have a specific language extension and only generic LSP support).

I've filed an issue with VS Code - if anyone else was planning to use them for something more generic, please consider adding 👍 !

microsoft/vscode#151920

@thaliaarchi
Copy link

Like @DanTup, I find the InlayHintKind values restrictive for my use case. I am building a language server for Whitespace that will display the opcodes inline in the invisible code, to make editing easier. These aren't type or parameter annotations, but rather display the statements in an alternate syntax.

For example, here is roughly what a program would look like with the inlay hints interspersed:

push·1   	
label·C
   	    		
dup 
 printi	
 	push·10   	 	 
printc	
  push·1   	
add	   dup 
 push·11   	 		
sub	  	jz·E
	  	   	 	
jmp·C
 
 	    		
label·E
   	   	 	
drop 

end


I've also written a little more about it in a gist, which includes this program in the standard syntax, assembly syntax, and with inlay hints.

@matklad
Copy link
Contributor

matklad commented Jun 14, 2022

@DanTup note that you don't have to use custom decorators to do that, it's possible to achieve this using today's inlay hints API. rust-analyzer does this. We send kind to null for all custom kinds of inlays:

https://github.com/rust-lang/rust-analyzer/blob/7db73875ac0d9280ae93b14232249d9c1496583a/crates/rust-analyzer/src/to_proto.rs#L472

@DanTup
Copy link
Contributor

DanTup commented Jun 14, 2022

@matklad the decorations is the old implementation from before InlayHints. I wanted to migrate it to Inlay Hints. The wording of the kind suggests that the editor might assume type/param names of kind isn't provided, so the theming could be confusing:

Can be omitted in which case the client should fall back to a reasonable default.

Telling users to modify some "parameter names" color or similar to control this would be odd.

In the past I've shipped things that didn't line up with the spec and things have changed/been broken, so I'm trying to avoid using things for purposes they might not be intended for (at least without some reasonable clarification that it's a valid use).

@jasonjurotich
Copy link

Just wanted to know if there was any advances in the inlay hints in Helix. It's the only think keeping me from moving from VSCode...

@poliorcetics
Copy link

If there are two inlay hints request in flight for two different files, I see no way in the returned InlayHint data to see for which URI they apply (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_inlayHint), do I just hope the last-response is to be used for the currently focused file ?

@sam-mccall
Copy link
Contributor

sam-mccall commented Feb 11, 2023

@poliorcetics the client request specifies which document it is for and specifies a request ID. The server response includes the request ID it corresponds to. This applies to all request/response pairs.

Edit: this isn't described in each request/response message in the spec as it's part of the JSON-RPC layer that encloses them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality new request
Projects
None yet
Development

No branches or pull requests