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 #11445

Merged
merged 16 commits into from
Mar 7, 2022
Merged

Upstream inlay hints #11445

merged 16 commits into from
Mar 7, 2022

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Feb 9, 2022

Closes #2797
Closes #3394 (since now resolve the hints for the range given only, not for the whole document. We don't actually resolve anything due to hard requirement on label being immutable. Any further heavy actions could go to the resolve method that's now available via the official Code API for hints)

Based on @SomeoneToIgnore's branch, with a couple of updates:

@lnicola lnicola mentioned this pull request Feb 9, 2022
@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 9, 2022

Huh, surprisingly, I've been doing similar things today.
See microsoft/vscode#129528 (comment) for more context.

Does not work for me too: the hint provider is being called, but the TRACE logs in the extension host show me that 0 hints are actually emitted after that call. I'm not sure why that happens exactly, but will look closer this week.

@SomeoneToIgnore
Copy link
Contributor

downloading the .d.ts no longer works, but you can get it manually from https://raw.githubusercontent.com/microsoft/vscode/release/1.64/src/vscode-dts/vscode.proposed.inlayHints.d.ts

I've implemented that in my PR, you have to specify a certain preview feature since the old "enable preview api" setting does nothing and is deprecated.

if I'm reading the definition right, InlayHintKind needs to be serialized as a number, not string

That could be the case actually, I'll check it when I have time, thanks for the pointer.

I'll continue my experiments in my branch, if you don't mind 🙂

@lnicola
Copy link
Member Author

lnicola commented Feb 9, 2022

I've implemented that in my PR, you have to specify a certain preview feature since the old "enable preview api" setting does nothing and is deprecated.

It's not that. The API got stabilized today, so the proposed .d.ts is no longer there.

@SomeoneToIgnore SomeoneToIgnore marked this pull request as ready for review February 9, 2022 19:19
@SomeoneToIgnore SomeoneToIgnore marked this pull request as draft February 9, 2022 19:19
@jhgg
Copy link
Contributor

jhgg commented Feb 9, 2022

I really hope that vscode has improved their styling of inlay hints, because the ones provided by the TS language server are super ugly compared to those provided via r-a's vscode extension...

@jrieken
Copy link

jrieken commented Feb 10, 2022

I am curious why this isn't working for you. Let me know if you have questions and something simple for my to try. Also, todays, insiders should have the API available without proposed guard. That makes things a little simpler

@lnicola
Copy link
Member Author

lnicola commented Feb 10, 2022

@jrieken thanks! I posted yesterday on microsoft/vscode#129528, and I didn't manage to figure it out.

Unfortunately, Insiders crashes on start-up in SQLite for me -- I couldn't find a bug for it, but I didn't report it yet. I know that a version from maybe a couple of weeks ago was working.

@jrieken
Copy link

jrieken commented Feb 10, 2022

Saw your comment on the other issue, replied inline (microsoft/vscode#129528 (comment)). Interesting finds, this fix is near :-)

@lnicola
Copy link
Member Author

lnicola commented Feb 10, 2022

Thanks, @jrieken, I assumed it was something like this when I noticed that even the range was printed as an array.

Well, I must be passing the wrong position anyway 😅 :

image

Better screenshot:

image

@jrieken
Copy link

jrieken commented Feb 10, 2022

Nice. Glad to see this working (no matter the true position ;-)). Fyi - I have just pushed microsoft/vscode@0fd15bb which relaxes how the base types work

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 10, 2022

@jrieken , a few questions and notes that I have:

* editor.inlayHints.fontFamily and editor.inlayHints.fontSize are autocompleted in settings.json, yet they change nothing. Is it really something that is going to be supported?

that file contains all our client code that uses the proposed API.
We trigger hints updates on every document update, using the onDidChangeInlayHints Event for that.
Yet the logs show that 2 seconds pass since the event trigger before the provider does anything. That is quite disorienting after rust-analyzer's blazing speed.

I will record a set of videos to show the caret and update cases.

@lnicola
Copy link
Member Author

lnicola commented Feb 10, 2022

editor.inlayHints.fontFamily and editor.inlayHints.fontSize are autocompleted in settings.json, yet they change nothing. Is it really something that is going to be supported?

Nb. these actually work fine for me:

image

What bothers me more is that they're usually not very well aligned with the code.

but does jump around Code's hints?

This is actually interesting, and it behaves somewhat differently depending on the direction you're moving in.

@SomeoneToIgnore
Copy link
Contributor

@lnicola , have you tried opening more than one project? For me, only a single project now displays the hints, if I do cargo new and open the new one, no hints are displayed. Similarly, other "production" projects of mine don't show anything, yet the first opened project reliably displays the hints.

@lnicola
Copy link
Member Author

lnicola commented Feb 10, 2022

have you tried opening more than one project?

Opening other projects with File / Open Recent works fine for me.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 10, 2022

carets_and_hints.mov

In this PR's code, I've added a logging event just before the event.fire() I use to signal about the new hints, and two more in the beginning and end of the provideInlayHints.

Here's what it showing for any hint update:

DEBUG [10/02/2022, 16:24:56]: Hints update event is fired
DEBUG [10/02/2022, 16:24:57]: provideInlayHints got just called
DEBUG [10/02/2022, 16:24:57]: provideInlayHints exits...
DEBUG [10/02/2022, 16:24:58]: Hints update event is fired
DEBUG [10/02/2022, 16:25:00]: provideInlayHints got just called
DEBUG [10/02/2022, 16:25:00]: provideInlayHints exits...

It's always 1-2 seconds of latency between the event firing and the provideInlayHints method actually being called.

And, what's really odd, I'm still able to see the hints in the very first Rust project I've opened with visual-studio-code-insiders, all the others do not show the hints despite identical settings?

@jrieken
Copy link

jrieken commented Feb 10, 2022

@jrieken , a few questions and notes that I have:

How drastically can we change hints' labels during resolve? Not quite sure after seeing microsoft/vscode@1.64.1/src/vscode-dts/vscode.proposed.inlayHints.d.ts#L145
Can we first return in provideInlayHints the hint with {unknown} label and then, during resolveInlayHint, make it an actual type we want?

You actually cannot change the label, only tooltip, command, and location can be added/changed later. This is for a stable UI and extensions should provide hints with the final label.

When LSP protocol should get the updates? Or is it so, that the hints release in VSCode is about the editor only now?

/cc @dbaeumer Likely next milestone. It's winter and everyone is skiing ❄️

I was surprised when I've discovered that, but the caret now moves flawlessly around the decorations in our custom rust-analyzer implementations (as in intellij), but does jump around Code's hints?

There is microsoft/vscode#127703 for more explicit stops when using arrow keys, clicking should add stops already. Looping in @hediet for details.

The hint speed update seems to be odd.

We artificially delay updates while typing - before that things were moving too much (see microsoft/vscode#133730). This of course can be tweaks further. With trace logging enabled, the window log will show the actual delay (it's a dynamic value based on the time taken, tho never under 25ms). It should look like so [2022-02-10 18:43:06.462] [renderer1] [trace] [DEBOUNCE: InlayHint] for file:///Users/jrieken/Code/vscode/src/vs/base/common/resources.ts is 25ms. When scrolling the "true speed" should be visible 🐎

@SomeoneToIgnore
Copy link
Contributor

the final label

Thank you, good to be aware of this; might be worth mentioning in the documents.

"true speed"

Hints appear fast when scrolling indeed, but feels like different people could get used to different debouncing speeds, might be good to leave it configurable.

Otherwise, feels really nice and code saving, thank you for having those.
I'll will look into the resolve functionality more (unless somebody does that before) this weekend, to check for some quirks there (less inlay hints to load and code navigation via hint clicking looks compelling to try and soften the pill for people who'll switch from old hint behavior they were used to).

@lnicola
Copy link
Member Author

lnicola commented Feb 12, 2022

Screenshot without padding:

image

If anyone wants to test this, you'll need to use the Insiders version of Code.

@bjorn3
Copy link
Member

bjorn3 commented Feb 12, 2022

@jrieken I think I personally would either like to have an option to make the debouncing faster or at least have it indicate inlay hints which are outdated in some way until they are updated. Or maybe it could keep the size of the inlay hint identical for a bit and only change the contents, so you get empty space if it gets shorter and an ellipsis when it would overflow.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 12, 2022

I've played a bit and personally disliked the padding-less approach after a while, ergo bringing the padding back. I guess, we might want to add it via the settings eventually? Not sure if anybody wants that form though, due to odd alignment of the hints.

I've also tried to emulate the dynamic hints and found the tooltips behaving a bit odd:

  • they do not update when I hover over other InlayHintLabelPart without exiting the hint with my cursor. If I leave it, the tooltip updates.
  • constant "Loading..." appears despite no resolveInlayHint was implemented: all data was statically put into the hints on provideInlayHints request
  • InlayHintLabelPart are not highlighted anyhow before the user presses ctrl/cmd, which makes the code navigation feature a bit hard to discover.
    I wonder, if it makes sense to add some dynamics into the hint by default (without any modifiers pressed), so that a user could guess that cmd+click could work. For instance, JetBrains had added matching bracket highlighting on by default in their hints for the same purposes also, I think.
tooltips.mov

@jrieken
Copy link

jrieken commented Feb 22, 2022

InlayHintLabelPart are not highlighted anyhow before the user presses ctrl/cmd, which makes the code navigation feature a bit hard to discover.

Agree. I will push a change today that adds the underline on hover which become an active link when cmd is pressed (similar to links in the editor)

Screen.Recording.2022-02-22.at.11.08.53.mov

@IWANABETHATGUY
Copy link

IWANABETHATGUY commented Mar 4, 2022

The inlay hint has been finalized in the latest vscode, maybe we could land it in the rust-analyzer now.

@lnicola
Copy link
Member Author

lnicola commented Mar 7, 2022

Can we merge this to give it some time to bake?

@nemethf
Copy link
Contributor

nemethf commented Mar 7, 2022

@lnicola, isn't it necessary to update the experimental capabilities as well? Thanks.

https://github.com/rust-analyzer/rust-analyzer/blob/2bcde5953a1740a960477d47aced8607f028b794/crates/rust-analyzer/src/caps.rs#L115-L129

@lnicola
Copy link
Member Author

lnicola commented Mar 7, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 7, 2022

@bors bors bot merged commit 49646b7 into rust-lang:master Mar 7, 2022
@lnicola lnicola deleted the upstream-inlay-hints branch March 7, 2022 17:37
bors bot added a commit that referenced this pull request Mar 8, 2022
11653: fix: client distribution string replacement looking for wrong key r=Veykril a=Veykril

cc #11445
bors r+

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
@RagibHasin
Copy link

RagibHasin commented Mar 9, 2022

Probably I'm late here, but why do these new inlay hints not contain colons? It is very distracting to distinguish between a hint and code even with their different styling options. Or, should I open a separate issue for this?

@lnicola
Copy link
Member Author

lnicola commented Mar 9, 2022

@RagibHasin the colons were added back in #11658.

ian-h-chamberlain added a commit to ian-h-chamberlain/rust-analyzer that referenced this pull request Mar 14, 2022
Closes rust-lang#6883 

This functionality was changed as of rust-lang#11445 and now can be customized using native VSCode settings instead of `rust-analyzer`-specific ones.
InlayKind::ChainingHint => lsp_ext::InlayKind::ChainingHint,
InlayKind::ParameterHint => Some(lsp_ext::InlayHintKind::PARAMETER),
InlayKind::TypeHint => Some(lsp_ext::InlayHintKind::TYPE),
InlayKind::ChainingHint => None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do chaining hints not have their own kind? (CC)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely cause the proposal only defines type and parameter hint kinds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose a chaining hint is a kind of type hint?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors bot added a commit that referenced this pull request Mar 15, 2022
11708: Update manual for inlay hint customization r=Veykril a=ian-h-chamberlain

Related to #6405, #6883 but not sure if they should be closed or not as this is just a documentation update.

This functionality was changed as of #11445 and now can be customized using native VSCode settings instead of `rust-analyzer`-specific ones.

Co-authored-by: Ian Chamberlain <ian-h-chamberlain@users.noreply.github.com>
bors bot added a commit that referenced this pull request Mar 16, 2022
11720: fix: Mark chaining hints as types r=SomeoneToIgnore a=lnicola

CC #11445 (comment)

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
HKalbasi pushed a commit to HKalbasi/rust-analyzer that referenced this pull request Mar 17, 2022
Closes rust-lang#6883 

This functionality was changed as of rust-lang#11445 and now can be customized using native VSCode settings instead of `rust-analyzer`-specific ones.
@Sheepyhead Sheepyhead mentioned this pull request May 3, 2022
bors bot added a commit to aya-prover/aya-dev that referenced this pull request Jun 12, 2022
424: New features to LSP r=ice1000 a=imkiva

this PR upgraded LSP4j to 0.14 with inlay hints support; added some features to the LSP backend:
- Inlay type hints for bind patterns (feel free to suggest more)
- CodeLens of each definition showing `%d usages`
- Code folding for definitions that occupy 3 or more LOC.
- Search Everywhere in VSCode (symbols only since VSC does not ask more from backend)

Try it: https://github.com/aya-prover/aya-vscode/suites/6895435995/artifacts/267446633

## See also
Language Server Protocol has recently upgraded to 3.17, and VSCode stabilized their inlay hints APIs
- https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_inlayHint
- rust-lang/rust-analyzer#11445
- microsoft/vscode-languageserver-node#495
- aya-prover/aya-vscode#21


Co-authored-by: imkiva <imkiva@islovely.icu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move inlayHints LSP request to "resolve" or range pattern for perf Upstream inlay hints