Feat: LSP Type Hints #5934

poliorcetics · 2023-02-12T03:15:33Z

Add support for LSP type hints (merged in version 3.17 of the spec, a few month old now).

For now, I only added it in the main editor views, pickers do not have type hints.
Even if we decide they need to, I would prefer doing it in another PR since this one is already big
enough as it is.

I'll join a screen in a comment below, and if people could try it out with their LSPs and configs, it
would be very useful, since I pretty much only use Rust-Analyzer so testing may be somewhat limited still.

Closes #2070

EDIT: instructions to test

poliorcetics · 2023-02-12T03:16:03Z

Proful · 2023-02-12T04:29:15Z

Amazing!

pascalkuthe

Nice to finally see the virtual text API put to use 🎉

Sadly I think you missunderstood the API somewhat and therefore had to do a bunch of unnecesassary changes. I designed the virtual text API very carefully (including with this usecase in mind) and the way you changed the API here makes it less efficent and less elegant.

The API is a bit abstract without an example and you are not the first person to be initially confused. I think the core of your misunderstanding is that you created a single layer (call to add_inline_annotations) per inlay a hint

That are a lot of layers (and not efficent since every layers must be checked at every singel char when transvcersing the document) and I therefore you probably thaught that you needed to store these somewhere.

Instead the idea is that one layer corresponds to a single TYPE of virtual text.
Therefore it is possible to construct these layers on the fly in the text_annotations functions and only store the contents of the layers. In your case there should only be the need for 4 layers. Constructing a Vec of 4 elements on the fly is basically free and is how the API was supposed to be used.

I left a bunch of comments with more details.

Once you apply my suggestion the diff should reduce significantly and this should turn into a smaller PR.

helix-view/src/document.rs

helix-core/src/text_annotations.rs

helix-view/src/view.rs

helix-view/src/document.rs

helix-term/src/commands/lsp.rs

poliorcetics · 2023-02-12T21:44:41Z

Sadly I think you missunderstood the API somewhat and therefore had to do a bunch of unnecesassary changes. I designed the virtual text API very carefully (including with this usecase in mind) and the way you changed the API here makes it less efficent and less elegant.

The API is a bit abstract without an example and you are not the first person to be initially confused. I think the core of your misunderstanding is that you created a single layer (call to add_inline_annotations) per inlay a hint

That are a lot of layers (and not efficent since every layers must be checked at every singel char when transvcersing the document) and I therefore you probably thaught that you needed to store these somewhere.

Yes, this is really confusing at first because there is pretty much zero doc in Helix about virtual text and annotations (or, if there is, it's absolutely not where I expect it to be, on the types and methods) so I was really confused, thanks for clarifying !

Instead the idea is that one layer corresponds to a single TYPE of virtual text.
Therefore it is possible to construct these layers on the fly in the text_annotations functions and only store the contents of the layers. In your case there should only be the need for 4 layers. Constructing a Vec of 4 elements on the fly is basically free and is how the API was supposed to be used.

Yes, that approach is much better, I used it instead. I can't separate padding from the hints though, else it breaks in all kinds of ways: everything is squished together and unreadable.

Once you apply my suggestion the diff should reduce significantly and this should turn into a smaller PR.

Not really 😅 I added quite some docs for inlay hints in documents that re-inflated the count after reducing it, but still the MR is better than it was before thanks to you

EDIT: I left a few comments open voluntarily, close them or answer as you want, for the others I either fixed the problem or changed the code too much to leave them as-is

helix-view/src/view.rs

helix-term/src/commands/lsp.rs

pascalkuthe · 2023-02-12T21:54:58Z

Yes, that approach is much better, I used it instead. I can't separate padding from the hints though, else it breaks in all kinds of ways: everything is squished together and unreadable.

Huh that is odd, it shouldn't let me check.

Not really sweat_smile I added quite some docs for inlay hints in documents that re-inflated the count after reducing it, but still the MR is better than it was before thanks to you

Well you added quite a bit of docs and its still almost 25% smaller so I would call that a win :D

pascalkuthe · 2023-02-12T22:27:21Z

Yes, that approach is much better, I used it instead. I can't separate padding from the hints though, else it breaks in all kinds of ways: everything is squished together and unreadable.

Huh that is odd, it shouldn't let me check.

Not really sweat_smile I added quite some docs for inlay hints in documents that re-inflated the count after reducing it, but still the MR is better than it was before thanks to you

Well you added quite a bit of docs and its still almost 25% smaller so I would call that a win :D

I just implemented this myself and it worked fine for me.
However for this to work the order of layer is important, since all these annotations will occur at the same position:

first add the left padding layer
the the actual label layers
then the right padding layer

helix-view/src/document.rs

pascalkuthe · 2023-02-12T22:53:21Z

helix-view/src/document.rs

+            let apply_inlay_hint_changes = |annotations: &mut Rc<[InlineAnnotation]>| {
+                if let Some(data) = Rc::get_mut(annotations) {
+                    for inline in data.iter_mut() {
+                        inline.char_idx = changes.map_pos(inline.char_idx, Assoc::After);


Neither Assoc::After or Assoc::Before works well here that is because both of these were designed for ranges where the start is mapped with Assoc::Before and Assoc::After. However for inlay hints we want something different. Inlay hints are bound to a single point so when that point is removed when also want to delete the inlay hint. For that you need to add a new variant Assoc::Point which returns None in case the position which contains char_idx was removed/replaced.

For that to worth tough we need a way to remove InlineAnnotations so the equivalent for Vec::retain but for Rc<[T]> which is not possible without a custom reference counted type (which we probably don't want to add here).

I am considering just switching TextAnnotations back to using slices instead of Rc which it originally did. I changed it to make the API easier to use/remove lifetime issues that occured in #5340 but I think those issues were actually resolved by something else so maybe a slice is a better choice afterall.

I don't have more time to look into this right now but you could start with implementing Assoc::Delete and I will try to comeup with something in the next couple of days.

I'm not sure I want to do this in this PR. Nor that I am qualified to do it.

ChangeSet::map_pos() currently unconditionnally returns a usize, adding Assoc::Delete or Assoc::Point would need to change the return type to account for the deletion cleanly.

Instead I think this PR should either be blocked until map_pos and Assoc::... are better equipped or, if we don't want to block too long, make an issue for it and fix it later on since for now Assoc::After works well enough (it probably has strange edge cases at the end of files but apart from that it should be okay for a v1)

If you don't feel confident working on this that's fine. I will create a PR myself in the next couple days that that you can then depend on. It's not that big of a change so I dlunt it will block this PR for long.

I think it is a requirement for this because without properly handeling this some odd effects do occur. For example if you delete two lines 2xd then all inlay hints within those lines are moved to the start of the next line

I actually changed my mind on this. Mapping points doesn't work well in general. The right fix here would be to convert these points to ranges by expanding to the word at the specific position and then tracking that range (only removing it when empty just like we do for diagnostics).

I have a pretty good idea how to implement this but it's a bit more effort and I don't have the necessary time right now. In practice this not as large of a problem as I orginally tough because the type hints get refreshed relatively quickly (on idle timeout) so the artificats usually don't stick around for that long. So I think I can do the necessary refactors in a followup PR.

helix-term/src/ui/editor.rs

helix-view/src/document.rs

helix-term/src/commands/lsp.rs

poliorcetics

Fixed some of the comments, I'm gonna work on the bigger ones now

helix-view/src/view.rs

helix-view/src/document.rs

helix-term/src/ui/editor.rs

helix-term/src/commands/lsp.rs

lazytanuki · 2023-02-14T21:17:57Z

Hi and thanks for working on this!
Been testing for a couple hours and it's really nice.
I find that having the hints pop up in the function arguments while typing them can be a bit unnerving, as it shifts the cursor back and forth.
I think it would be nice to have a setting to disable inlay hints for the current function parameters, in insert mode a least. Or maybe in insert mode altogether ? Only on the current line ? What do others think ?

poliorcetics · 2023-02-14T21:42:57Z

Those should probably be options. I agree that some of that sound nice, though I think implementing them in this MR would delay it too much: I would prefer having something working merged then refine it as user reports pop up :), that way we can both keep this MR relatively small (my time is sadly limited, the maintainers' time too) and account for lots of reports at once instead of just the people following this MR

Does that sound good to you ?

univerz · 2023-02-15T04:12:20Z

I find that having the hints pop up in the function arguments while typing them can be a bit unnerving

+1. I didn't notice this problem while using vscode, so there is a way to do it "right".

marcbull · 2023-02-15T16:40:03Z

Thanks for the work on this part of Helix, this is one of the features I really want to use.

I find that having the hints pop up in the function arguments while typing them can be a bit unnerving, as it shifts the cursor back and forth.

If I compare it with VS code, the experience there is more pleasant because there is a delay after the last change until the LSP type hint is adjusted.

I don't think it would be good to disable the LSP type hint in the current line or completely in insert mode, because I personally like to check the result value as I type and it's a welcome hint to see if the result is an option or already a T, especially if you're new to a project or using a new API.

archseer · 2023-03-09T11:00:33Z

Hmm this isn't blocking but I've noticed that if I make a selection the code is selected but the hints aren't highlighted as a part of the selection. It's probably better this way, but do we get the same behaviour in VSCode and elsewhere?

pascalkuthe · 2023-03-09T11:27:33Z

Yeah vscode behaves the same and it's very tricky to change because we do selectionswith syntax highlights. If we applied those to virtual text than virtual text would have the full style of the text (so syntax highlight color, dagnowtic underlines...)

KittyBorgX · 2023-03-09T13:06:12Z

+1 This behaves similar to VSCode

erasin · 2023-03-09T14:25:53Z

I've been using this pr for a week or two, and I didn't realize the problem before.
There has a problem that the cursor position in insert mode is after the virtual text.

helix:

vscode:

poliorcetics · 2023-03-09T15:04:37Z

I can’t do anything about that, all inlay hints in Helix will behave like that due to how they’re rendered, though I agree it isn’t the most agreable.

Writing a « dumb » fix here (offsetting by one) for type hints would break for arguments and hints at the end of a line

pascalkuthe · 2023-03-09T15:09:57Z

I've been using this pr for a week or two, and I didn't realize the problem before. There has a problem that the cursor position in insert mode is after the virtual text.

helix:

vscode:

It's not perfect but yeah as @poliorcetics said that's kind of just how the cursor works in helix. In insertmode we increase the selection size by one and insert before the cursor block. If you use block cursors its much more clear why it works the way it does: The selection extends one character after the virtual text. Maybe if we switch to zero width cursors oneday that could be solved but i am not sure about that. I could also do some evil trickery here to sepcical case bar cursors but I would rather not do that.

Lordxan · 2023-03-09T15:14:52Z

I've been using this pr for a week or two, and I didn't realize the problem before. There has a problem that the cursor position in insert mode is after the virtual text.
helix:

vscode:

It's not perfect but yeah as @poliorcetics said that's kind of just how the cursor works in helix. In insertmode we increase the selection size by one and insert before the cursor block. If you use block cursors its much more clear why it works the way it does: The selection extends one character after the virtual text. Maybe if we switch to zero width cursors oneday that could be solved but i am not sure about that. I could also do some evil trickery here to sepcical case bar cursors but I would rather not do that.

I actually would like to see an optional patch for that (sorry can't do it myself just started learning rust)

erasin · 2023-03-09T15:27:55Z

Since I am a non-ascii characters user, the block cursor is so unfriendly to Chinese characters that I hardly ever use it.

The fact that this problem was not realized before means that it does not affect me much. Hope to become a better experience in the future.

thanks

poliorcetics · 2023-03-09T16:31:27Z

(I'm won't do it in this MR but here is a potential solution):

If the cursor is next to an inlay hint where the format is text<cursor><space> or <space><cursor><text>, we could display the cursor closer next to the text, adapting the position of the hint locally to obtain: text<cursor><hint><space> and <space><hint><cursor>text
In the case of a conflict (text<hint>text), we would just defaut to the current behaviour.

I'm not certain how much work it would be to do that, or if it's even possible, but if it works it would probably solve most of the problematic cases ?

EDIT: <space> means non-text, it can be a line-ending, a space, a tab ...

archseer · 2023-03-10T02:13:57Z

I've noticed this issue too, but given that it's a new feature that's also off by default we can resolve this in a follow up.

A quick solution we could use: only display hints outside of insert mode. That would also address issues with the hints shifting the code around while you type. Not sure how desirable this would be, the hints are helpful when typing out method parameters. Though that's also resolved by signature help. It just seems to me that hints are more beneficial during reading the code (normal mode) than writing (insert mode).

poliorcetics · 2023-03-10T08:23:33Z

only display hints outside of insert mode.

I don't know, things disappearing just before I start typing seems very jarring too.

It just seems to me that hints are more beneficial during reading the code (normal mode) than writing (insert mode).

I'm also reading code when I type some more, adding code is not done in a vacuum

If we do this it should be an option, because it seems to me to be very much useless to only have type hints outside of insert mode

jquesada2016 · 2023-03-10T13:36:52Z

only display hints outside of insert mode.

I personally think this is a really bad idea. For example, when you're chaining method calls, it's often hard to remember what types the method will return, and being able to see it while in insert mode is very helpful.

erasin · 2023-03-10T13:53:43Z

Unlike #6059, type hints is very useful in insert mode. We do not need additional option.

archseer · 2023-03-11T02:29:04Z

Agreed, it was just an idea.

evanrichter · 2023-03-11T02:59:44Z

so hyped for all the fantastic virtual text features coming down the pipeline. the next release is going to be off the charts! thanks to the maintainers and everyone involved <3

Hywan · 2023-03-11T11:05:26Z

Awesome!

eulerdisk · 2023-03-11T20:13:17Z

I think inlay-hints theme text style should fallback to the comment style.
Now a lot of theme are "broken", inlay hints for types are like user written types.

* misc: missing inline, outdated link * doc: Add new theme keys and config option to book * fix: don't panic in Tree::try_get(view_id) Necessary for later, where we could be receiving an LSP response for a closed window, in which case we don't want to crash while checking for its existence * fix: reset idle timer on all mouse events * refacto: Introduce Overlay::new and InlineAnnotation::new * refacto: extract make_job_callback from Context::callback * feat: add LSP display_inlay_hint option to config * feat: communicate inlay hints support capabilities of helix to LSP server * feat: Add function to request range of inlay hint from LSP * feat: Save inlay hints in document, per view * feat: Update inlay hints on document changes * feat: Compute inlay hints on idle timeout * nit: Add todo's about inlay hints for later * fix: compute text annotations for current view in view.rs, not document.rs * doc: Improve Document::text_annotations() description * nit: getters don't use 'get_' in front * fix: Drop inlay hints annotations on config refresh if necessary * fix: padding theming for LSP inlay hints * fix: tracking of outdated inlay hints should not be dependant on document revision (because of undos and such) * fix: follow LSP spec and don't highlight padding as virtual text * config: add some LSP inlay hint configs

cidit · 2023-12-25T02:10:51Z

do i need to use helix manually compiled on main to use this feature ?

gabydd · 2023-12-25T02:59:42Z

No this was released in 23.05 I believe, you just need to enable it in your config. And for some languages you might need to enable it on the actual language server.

Since helix-editor/helix#5934 landed in 23.05 helix supports hints from the LSP. This PR change the hints colors to be the same as comments because otherwise they show the same colour as code which is confusing.

poliorcetics mentioned this pull request Feb 12, 2023

LSP Type Hints #2070

Closed

poliorcetics force-pushed the 2070-lsp-type-hints branch from 1ee5692 to e977c35 Compare February 12, 2023 03:28

pascalkuthe self-requested a review February 12, 2023 12:30

pascalkuthe requested changes Feb 12, 2023

View reviewed changes

poliorcetics force-pushed the 2070-lsp-type-hints branch from e977c35 to 3f1b568 Compare February 12, 2023 21:34

pascalkuthe reviewed Feb 12, 2023

View reviewed changes

helix-view/src/view.rs Outdated Show resolved Hide resolved

pascalkuthe reviewed Feb 12, 2023

View reviewed changes

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved

pascalkuthe reviewed Feb 12, 2023

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

pascalkuthe reviewed Feb 12, 2023

View reviewed changes

helix-term/src/ui/editor.rs Show resolved Hide resolved

archseer reviewed Feb 13, 2023

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

archseer reviewed Feb 13, 2023

View reviewed changes

helix-view/src/document.rs Show resolved Hide resolved

archseer reviewed Feb 13, 2023

View reviewed changes

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved

poliorcetics force-pushed the 2070-lsp-type-hints branch from 3f1b568 to b96469d Compare February 13, 2023 22:28

poliorcetics commented Feb 13, 2023

View reviewed changes

poliorcetics mentioned this pull request Mar 10, 2023

See #6417 instead [Feat: CodeLens-style diagnostics] #6059

Closed

archseer approved these changes Mar 11, 2023

View reviewed changes

archseer merged commit bdcd4d9 into helix-editor:master Mar 11, 2023

EpocSquadron mentioned this pull request Mar 13, 2023

Whitespace inside LSP hints should not render #6297

Closed

AlexanderBrevig mentioned this pull request Mar 14, 2023

feat: lint for ui.virtual and catch present and future virtual based features #6320

Merged

yuyidegit mentioned this pull request Mar 24, 2023

Inline Diagnostics #6417

Merged

poliorcetics deleted the 2070-lsp-type-hints branch February 12, 2024 22:11

jbtrystram mentioned this pull request Jul 10, 2024

Make hints the same colour as comments. CptPotato/helix-themes#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: LSP Type Hints #5934

Feat: LSP Type Hints #5934

poliorcetics commented Feb 12, 2023 •

edited

Loading

poliorcetics commented Feb 12, 2023 •

edited

Loading

Proful commented Feb 12, 2023

pascalkuthe left a comment •

edited

Loading

poliorcetics commented Feb 12, 2023 •

edited

Loading

pascalkuthe commented Feb 12, 2023

pascalkuthe commented Feb 12, 2023

pascalkuthe Feb 12, 2023

poliorcetics Feb 13, 2023

pascalkuthe Feb 13, 2023

pascalkuthe Feb 15, 2023

poliorcetics left a comment

lazytanuki commented Feb 14, 2023 •

edited

Loading

poliorcetics commented Feb 14, 2023

univerz commented Feb 15, 2023 •

edited

Loading

marcbull commented Feb 15, 2023

archseer commented Mar 9, 2023

pascalkuthe commented Mar 9, 2023 •

edited

Loading

KittyBorgX commented Mar 9, 2023

erasin commented Mar 9, 2023 •

edited

Loading

poliorcetics commented Mar 9, 2023

pascalkuthe commented Mar 9, 2023

Lordxan commented Mar 9, 2023

erasin commented Mar 9, 2023

poliorcetics commented Mar 9, 2023 •

edited

Loading

archseer commented Mar 10, 2023

poliorcetics commented Mar 10, 2023

jquesada2016 commented Mar 10, 2023

erasin commented Mar 10, 2023 •

edited

Loading

archseer commented Mar 11, 2023

evanrichter commented Mar 11, 2023

Hywan commented Mar 11, 2023

eulerdisk commented Mar 11, 2023

cidit commented Dec 25, 2023

gabydd commented Dec 25, 2023

Feat: LSP Type Hints #5934

Feat: LSP Type Hints #5934

Conversation

poliorcetics commented Feb 12, 2023 • edited Loading

poliorcetics commented Feb 12, 2023 • edited Loading

Proful commented Feb 12, 2023

pascalkuthe left a comment • edited Loading

Choose a reason for hiding this comment

poliorcetics commented Feb 12, 2023 • edited Loading

pascalkuthe commented Feb 12, 2023

pascalkuthe commented Feb 12, 2023

pascalkuthe Feb 12, 2023

Choose a reason for hiding this comment

poliorcetics Feb 13, 2023

Choose a reason for hiding this comment

pascalkuthe Feb 13, 2023

Choose a reason for hiding this comment

pascalkuthe Feb 15, 2023

Choose a reason for hiding this comment

poliorcetics left a comment

Choose a reason for hiding this comment

lazytanuki commented Feb 14, 2023 • edited Loading

poliorcetics commented Feb 14, 2023

univerz commented Feb 15, 2023 • edited Loading

marcbull commented Feb 15, 2023

archseer commented Mar 9, 2023

pascalkuthe commented Mar 9, 2023 • edited Loading

KittyBorgX commented Mar 9, 2023

erasin commented Mar 9, 2023 • edited Loading

poliorcetics commented Mar 9, 2023

pascalkuthe commented Mar 9, 2023

Lordxan commented Mar 9, 2023

erasin commented Mar 9, 2023

poliorcetics commented Mar 9, 2023 • edited Loading

archseer commented Mar 10, 2023

poliorcetics commented Mar 10, 2023

jquesada2016 commented Mar 10, 2023

erasin commented Mar 10, 2023 • edited Loading

archseer commented Mar 11, 2023

evanrichter commented Mar 11, 2023

Hywan commented Mar 11, 2023

eulerdisk commented Mar 11, 2023

cidit commented Dec 25, 2023

gabydd commented Dec 25, 2023

poliorcetics commented Feb 12, 2023 •

edited

Loading

poliorcetics commented Feb 12, 2023 •

edited

Loading

pascalkuthe left a comment •

edited

Loading

poliorcetics commented Feb 12, 2023 •

edited

Loading

lazytanuki commented Feb 14, 2023 •

edited

Loading

univerz commented Feb 15, 2023 •

edited

Loading

pascalkuthe commented Mar 9, 2023 •

edited

Loading

erasin commented Mar 9, 2023 •

edited

Loading

poliorcetics commented Mar 9, 2023 •

edited

Loading

erasin commented Mar 10, 2023 •

edited

Loading