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

Upstream inlay hints #2797

Closed
kjeremy opened this issue Jan 11, 2020 · 29 comments · Fixed by #11445 or #11658
Closed

Upstream inlay hints #2797

kjeremy opened this issue Jan 11, 2020 · 29 comments · Fixed by #11445 or #11658
Labels
A-inlay-hints inlay/inline hints E-hard fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now

Comments

@kjeremy
Copy link
Contributor

kjeremy commented Jan 11, 2020

Should we upstream this as a proposed api?

@kjeremy
Copy link
Contributor Author

kjeremy commented Mar 2, 2020

We should look into this after #3394 is resolved. It may be a stepping stone to push microsoft/vscode#16221.

@matklad
Copy link
Member

matklad commented Mar 2, 2020

I don't necessary think we should wait for VS Code to provide API here. I think it might make sens to prep a proposed API before that, it can be implemented, albeit in a hacky way, in today's Emacs and VS Code. Might make sense to create an issue on the LSP repo and ask if they have capacity to review protocol changes.

@matklad matklad added E-hard fun A technically challenging issue with high impact labels Mar 2, 2020
@kjeremy
Copy link
Contributor Author

kjeremy commented Mar 2, 2020

Yep! I think the LSP side is the first step.

@kjeremy
Copy link
Contributor Author

kjeremy commented Apr 8, 2020

@Veetaha @matklad

You may want to jump in on the issue I created in the vscode-languageclient repo.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 19, 2021

We've got a great upstream proposal 🎉
Here's the comment with it: #5123 (comment)

==============================
@jrieken , let's continue the upstream discussion here: an open issue seems to be the more appropriate place for this.

I have a few bullets with the proposals and questions, in no spefic order of importance.
I've tried to elaborate every one of them, sorry for the wall of text.
If you want to, we can discuss every one of them separately here or in Zuilp if that's simpler for you.

The range of the hint

(the tsdoc on the field and the constructor)

If I've understood the fooLang example right, it's not really a range of the hint itself, but rather the range of the element this hint applies to.
If I'm wrong, then it's a bit strange (also cumbersome and error-prone) that I have to specify both the hint start, end and the label: feels like the end can be derived automatically then.

The range for which line hints should be computed.

Not sure what a line hint is, guess that have to be inline.
Also, looking at the search results, "inlay hints" are way more popular than "inline hints", so it might slightly less confusing to use the former naming.

  • do we define how the editor behaves during selection and navigation around the hints?

This is one of the biggest sources of confusion for people using RA, I'd like to talk it through explicitly. Currently, our decorator-based approach makes the caret "jump" near the end of the element with the hint: #2888
Something similar happens with the hint that is located before the element, not after.

It would be great to explicity define how the caret should behave in such cases, including the modifier keys (alt + arrow that forces the caret to skip the whole word is a great case).

  • whitespaceBefore / whitespaceAfter are rather odd in my opinion

I can hardly imagine why would we need to add those and when: if we'd want to add a whitespace, we'll add it on the server side, we are already adding things like : for the type hints.
We use different style customizations, I'll describe them in the next bullet, might be good to merge them together if you think they are useful still.

  • no way to customize the hints' style

We use a number of style options on our hints, some of them can be customized by the users: color, backgroundColor, fontStyle, fontWeight, textDecoration.

https://github.com/rust-analyzer/rust-analyzer/blob/77ffe137f2691164b199abfbadd14f73abede48d/editors/code/src/inlay_hints.ts#L60-L64

It does not have to be configured the same way and can be moved into a theme settings, for instance, but still nice to have and have the docs for that.

  • no way to specify the position of the hint in relation to the element it hints

We use both "before element" (for parameter name hints) and "after element" decoration (for type hints and chaining hints), but I don't see any parameters related to that in the proposal.

  • description, especially the markdown form of it also seems to be more of a special case, see the next bullet for the more generalised proposal.

Frankly, I have no idea why would I want to show another hover pop-up, the variable hover covers the majority of the use cases for me. Maybe I'm just missing a good usecase example here.

  • add ways to interact with the hints

intellij-rust allows ctrl/cmd + clicking on the hints to navigate into the definition (and it respects generics and other complex types' parts!), highlights different parts of the type on hover and unfolds shortened hints on a regualr click: intellij-rust/intellij-rust#4217

It would be great to provide some kind of a callback (done lazily in another request maybe?) that allows the server to return whatever it wants to do on hover (markdown, text, just highlight the hint, etc.) and on click, potentially with the modifier keys (unfold the hint, navigate into the type, etc.).

For that, some additional data might be needed, see the next bullet.

  • no way to pass additional data in the hints' objects

Since the hints display logic ideally needs to follow the resolve pattern and be stateless, it's way simpler to pass around some additional data storage between requests, similar way VSCode has CompletionItem.data field that helps during the resolve stage.

For example, we have different kinds of hints (parameter, type, chaining) and that requires different handling, especially if we want to add actions or any other complex things in our logic.

  • add caret position (line, selection, etc.) to the provideInlineHints method.

Despite all our efforts to shorten the hints, there are plenty of cases where the code gets bloated with them.
One of the ways to handle that is to provide the way to turn the hints on and off and we do this already.
A better way seems to display the hints for the line with the caret only (or, reversed, hide the hints for the line with the caret).

@kjeremy
Copy link
Contributor Author

kjeremy commented Jan 19, 2021

@DanTup you may be interested in this as well. I know that flutter provides hints at the end of lines when building widgets.

@DanTup
Copy link

DanTup commented Jan 19, 2021

@kjeremy ooh interesting, yes I am interested! I'll have a proper read through soon and post any feedback on the VS Code issue. Thanks!

@jrieken
Copy link

jrieken commented Jan 20, 2021

whitespaceBefore / whitespaceAfter are rather odd in my opinion

With server-whitespaces you cannot escape the bounding box, e.g the whitespace-characters will inherit the background color and whitespace[Before|After] should be seen as margin around the hint.

If I've understood the fooLang example right, it's not really a range of the hint itself, but rather the range of the element this hint applies to.

Hints having a range is for an emepheral mode where hints would actually overlay source text. VS supports this mode, e.g while you hold Alt+F1 hints are rendered on top, not in between. This comment contains a good sample: microsoft/vscode#113285 (comment). We haven't actually implemented this yet and it's not P1 right now.

do we define how the editor behaves during selection and navigation around the hints?

Agreed. But it is hard to come up with good rules. Also, we do use the same approach for color-decorations in CSS files and haven't gotten too much negative feedback. Maybe it is something you need to get used to and something that shouldn't be controlled by extensions but by user-settings. Tho, it is likely that we are taking this as an opportunity to revisit the underlying implementation in the editor. I will update you as I know more.

add ways to interact with the hints

That's an interesting one. I will need to see how to best fit that into our API. One thing that comes to my mind is a subset of markdown (no tables, lists, etc) which allows for some styling and links. Links in return can be command-links or "normal" resource/file links. With that we might also address the styling concerns - tho, I am also unsure if that's an extension or user setting... We generally try to avoid the "looks like a christmas tree" problem by keeping control with users. The current implementation does define theme colours (for background and foreground) which users or theme can tweak.

add caret position (line, selection, etc.) to the provideInlineHints method.

Unsure about that. We try to design our language API editor-agnostic, and we would need to find a nice way abstract that away. E.g. keep the LiveShare scenario in mind: there might be another user using the API without "your" renderer having an editor open for that document.

If you want to, we can discuss every one of them separately here or in Zuilp if that's simpler for you.

Despite having an API proposal and implementation we are still in early phases. I would really value if we can meet (Teams what not) so that I can pick your brain live and so that you can demo to me what you currently have

@flodiebold
Copy link
Member

flodiebold commented Jan 20, 2021

I am not a fan of hiding or showing inline hints based on cursor position, personally. I think it will lead to a lot of things switching around while moving through code. (And if something like that is implemented, it would probably make sense to do it on editor level, not in the providers.)

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 20, 2021

should be seen as margin around the hint

Right, now I start to understand, very good point.
Again, this all reminds me some kind of CSS styling reinvention here, so it might make sense to somehow unite with the other style-related settings such as background?
Although I suspect that you'll reply that the theme settings don't allow this (does it allow the font tweaking btw?), so as a middle ground, we could hide those under some ExtraHintStyle optional field in the InlineHint object instead of two high-level fields.
But no strong opinion here except the fact that it's not that simple to deduce this knowledge without proper explanations.

Hints having a range is for an emepheral mode where hints would actually overlay source text

Right, so another param in the provideInlineHints later to indicate the mode enabled, I presume.
Out of curiosity, how would the rendering, hover and caret navigation around behave in an "ordinary" mode when hint's text and range mismatch?
I'm imagining various cases when LSP lags or dies, leaving us with offset hints that might even overlap with each other and the mess it can cause.
But it's all speculations, so no real case I can think of for now.

Thanks for your thoughts, especially for the caret example. Maybe it's a far stretched idea after all indeed at this point.

if we can meet

Yes, let's do it if you think it's beneficial.
Any time starting next week forks for me, as long as you warn at least 24h upfront.

I believe, other people might want to participate too (matklad is always curious in everything and kjeremy has an extensive knowledge of our LSP impls), so we might keep that in mind when setting up the convo.

It would be super cool if you could provide any agenda for it, so I can prepare.
Right now I cannot come up with anything but showing how things work in RA (and showing our impl code) and in intellij-rust.

@SomeoneToIgnore
Copy link
Contributor

@jrieken , I can also implement our hints on top of your proposal, the prototype should not require big changes.

If I understand it right, I need the insiders build of vscode and a new version of vscode-languageclient?
I've got the insiders build, but found no hint-related changes (such as InlineHintsProvider) in https://github.com/microsoft/vscode-languageserver-node/commits/master

@jrieken
Copy link

jrieken commented Jan 22, 2021

It will be a little more complex because this proposal isn't in LSP yet. You would to send/receive custom messages in your extension and use the proposed API directly

@jrieken
Copy link

jrieken commented Jan 22, 2021

It would be super cool if you could provide any agenda for it, so I can prepare.
Right now I cannot come up with anything but showing how things work in RA (and showing our impl code) and in intellij-rust.

Yeah, we are just preparing for the Jan release but end of next week would be best for me (Thursday/Friday). I'll come up an agenda but we can keep it short and demo of what you have done, what you like/dislike about it, and what you expect is what I am actually looking for ;-)

@lnicola lnicola added the S-actionable Someone could pick this issue up and work on it right now label Jan 22, 2021
@lnicola
Copy link
Member

lnicola commented Jan 22, 2021

See also #3453, #3333 and #3138.

@d4h0
Copy link

d4h0 commented Jan 22, 2021

Would it be possible to integrate inline hints with the built-in VSCode line wrap feature? It seems, inline hints are ignored by the Word Wrap Column feature, which often results in lines that are wider than the visible space. This eliminates a lot of the usefulness of inline hints for me (seeing the types is sometimes useful, but having to scroll horizontally too often, negates this usefulness).

An option to show type hints above a line of code would also be great (and probably easier to implement than inline hints). The OCaml equivalent to Rust-Analyzer does it this way (at least when I used it the last time), and when I used RA for the first time, I disliked inline hints a bit (code jumps around while typing and sometimes isn't visible because lines are too long). I got used to it, and like inline hints for function parameter names a lot, but horizontal scrolling and code jumping around while typing is a big problem for me.

@kjeremy
Copy link
Contributor Author

kjeremy commented Feb 3, 2021

@SomeoneToIgnore @jrieken I just want to make sure that this issue isn't forgotten and if there's a corresponding vscode issue we should be linking back to this issue.

@IWANABETHATGUY
Copy link

current inlineHint's cursor behavior is still wired
upecG5Ha3q
compare To: intellij
85840869-d18ccb80-b7cf-11ea-8806-d9050a5a4c11

@SomeoneToIgnore
Copy link
Contributor

@kjeremy , no it's not forgotten, thanks for pinging.

We've just agreed to have a brief meeting this Friday to discuss things further, ping me on Zulip for the meeting link, in case you'd like to participate.
(I'm not sure it's wise to expose the link here in public 🙂)

@kjeremy
Copy link
Contributor Author

kjeremy commented Mar 31, 2021

@SomeoneToIgnore If I'm reading the release notes right it looks like the inline values provider API has been stabilized with today's vscode release.

@SomeoneToIgnore
Copy link
Contributor

Thank you for checking on this and reminding me that I have to post the issue status.

That feature you're referring to is related to the debugger functionality and the way it displays data for the variables during the debugging (see the gif below in the link): https://code.visualstudio.com/updates/v1_54#_inline-value-provider-api
So it's not exactly what we're discussing here, even though it's also an amazing feature to have in our plugin for Rust debugs.

@SomeoneToIgnore
Copy link
Contributor

As for the inline hints, I have the https://github.com/SomeoneToIgnore/rust-analyzer/tree/upstream-inlay-hints branch that uses VSCode latest proposed API instead our custom one for the inlay hints.
Once per 1-2 weeks I force push there the fresh master and recheck the feature on the fresh VSCode insiders.

You can run it yourself:

  • Install VSCode insiders
  • Install the LSP server locally: cargo xtask install --server
  • Install the vscode plugin into the insiders version: cargo xtask install --client --code-bin code-insiders
  • Run VSCode insiders with the experimental api enabled: code-insiders --enable-proposed-api matklad.rust-analyzer
  • Configure insiders' settings.json to point at the previously installed LSP server: "rust-analyzer.server.path": "rust-analyzer", given that you've added Cargo bin dir into your path

@SomeoneToIgnore
Copy link
Contributor

Current VSCode api and implementation seems to be rather stalled though: it had not been updated for a while and has a few issues with the performance and the way the hints are displayed.

This is how the hints are displayed by default now:

image

The color is trivial to amend, but there are still API limitations on how we can display them:
Screenshot 2021-04-01 at 16 18 03

The caret behaves now the same way as it does with the decorations.

I've shared the feedback and monitor the API occasionally, so it feels like we need to wait longer for the feature to come.

@rami3l
Copy link
Member

rami3l commented Aug 11, 2021

@SomeoneToIgnore @matklad Now in TypeScript 4.4 we have this feature shipped: microsoft/vscode#16221 (comment)

Is it possible to do the same thing with rust-analyzer? :)

Update: Just realized that the TS plugin is independent of the LSP, so it should be interfacing directly with the injected text API. Given that, I agree that the rust-analyzer implementation should be delayed until inlay hints get upstreamed.

@Walther
Copy link
Contributor

Walther commented Aug 12, 2021

Would it be possible to integrate inline hints with the built-in VSCode line wrap feature? It seems, inline hints are ignored by the Word Wrap Column feature, which often results in lines that are wider than the visible space.

Was about to open an issue on this and found this thread. Not sure how much of this is on rust-analyzer and how much of this is on VSCode, but wanted to provide some example screenshots to clarify the part of the issue I'm having.

The inlay hints are a super useful feature of rust-analyzer that I absolutely love. Another feature that I like on VSCode's side is the "soft wrapping": no horizontal scrollbars, too long lines are soft-wrapped in the viewport but no actual line breaks are inserted. Line numbers just have a visual gap as needed. Sadly, the inlay hints are not taken into account when counting line width for soft wrapping. This means that with rust-analyzer, in various situations, you can end up with a horizontal scrollbar again.

Note how the docstring is soft-wrapped correctly on line 13, but the code on line 23 continues underneath the right sidebar:
rust-analyzer-softwrap1

For comparison, if you disable rust-analyzer for a moment and reduce the window size further, you can see that without the inlay hints, even the line 23 can be soft-wrapped and the user won't have a horizontal scrollbar:
rust-analyzer-softwrap4

@Walther
Copy link
Contributor

Walther commented Aug 12, 2021

Aha, after further searching found this thread microsoft/vscode#32856 (comment)

@Walther
Copy link
Contributor

Walther commented Aug 12, 2021

Further searching reveals this might be actually fixed soon? microsoft/vscode#126582 (comment) 👀

@rami3l
Copy link
Member

rami3l commented Aug 13, 2021

@Walther Yes, I mentioned TypeScript 4.4 above because the built-in VSCode TS plugin now used what they call injected text to display inlay hints, which plays well with word wrapping mode.

@Veykril Veykril added the A-inlay-hints inlay/inline hints label Dec 17, 2021
@lnicola
Copy link
Member

lnicola commented Feb 9, 2022

Inlay hints will be stabilized in the next version of Code.

I made a draft PR at #11445. Unfortunately, it doesn't work at all.

@rami3l
Copy link
Member

rami3l commented Mar 8, 2022

@lnicola
Thanks for the effort! I'm already using it and it seems quite nice! 🎉

One small question though: is it still possible to use the old style : Foo rather than just Foo?
I found the new style a bit confusing... (But maybe it's just me?)

bors bot added a commit that referenced this issue Mar 8, 2022
11658: Add back colons around inlay hints r=Veykril a=lnicola

Fixes #2797 (comment).

I originally thought that other extensions don't include the colons, but the TypeScript one seems to do.

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inlay-hints inlay/inline hints E-hard fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now
Projects
None yet