Skip to content
This repository was archived by the owner on Nov 18, 2022. It is now read-only.

Added Client Side Type Hints #587

Closed
wants to merge 15 commits into from
Closed

Added Client Side Type Hints #587

wants to merge 15 commits into from

Conversation

p-avital
Copy link

@p-avital p-avital commented May 9, 2019

Hello again,

So after finding out that CodeLenses weren't going to cut it on the Language Server side (See RLS PR#1442) I've tried using Decorations on the client side. And guess what? They're awesome!
image
That also solved the synchronisation issues I had on the RLS version, and now they're properly inlined like I wanted them too originally.

Hope you guys like it :D

@p-avital
Copy link
Author

And now with name shortening to avoir using way too much screen space:
image

@jonasbb
Copy link
Contributor

jonasbb commented May 20, 2019

Hi, I have tested this change the last week am I am really impressed. Overall it works really great and it is super helpful to know the exact type, such as "Is it a reference or not".

Thanks for the nice work.

I found some edge cases/little annoyances though, which I at least want to report on.

  1. Sometimes RLS does not return type information but instead the line declaring/defining the variable.
    Screenshot from 2019-05-20 15-13-27
    This doesn't matter much for the hover annotation, as it still helps in understanding the variable, but looks odd as a type annotation.
  2. You can no longer place the cursor directly behind the variable. You can only place the cursor behind the type hint. It feels a bit odd not being able to do that, e.g., if you want to append something to the variable name. Granted, this could also be a limitation of the annotation API.
  3. If you delete code, the type hint stays until the next update. If you delete a whole function, this can leave quite many type hints. It would be nice, if the type hint would be deleted when the character it is "attached to" is deleted.
  4. A tiny oddity in the type simplification. The type is RwLockReadGuard<'_, , but the life time is removed without any replacement making the type hint look like:
    photo_2019-05-20_23-03-58

@p-avital
Copy link
Author

p-avital commented May 20, 2019

Hi there,

Indeed, I had noticed some of these:

  1. Yeah, probably checking for some characters in the hint would fix that, I'll try to come up with a cost effective way to do that.

  2. That's a limitation of the API, the alternative is even more jarring, as clicking on the hint would send the cursor one char before end of word...

  3. I probably should call the decorate function on change, with a little timeout. My main objectives were handling editor switch and end of indexing. Guess that shouldn't be too heavy.

  4. I had noticed that, but haven't found an elegant fix yet. I ended up thinking "oh well, at least it shows that something has been omitted"

The greedy simplifier could use some cleverer code though, as the regex to shorten long composite types will also fail with things like Type<Sub1<...>, MiddleTypes, OtherShortened<...>>, as the regex I had to match these cases and capture the group inside the <> would hang for decades with the JS implementation of regex

@p-avital
Copy link
Author

@jonasbb This commit fixes your issues 1 and 3. I also took the opportunity to avoid adding virtual type hints where actual type hints actually existed, at least for closure parameters and simple let declarations.
It is indeed much more comfortable.

Like I said, issue 2 is API imposed, although some trickery might (but it's not garanteed) let us get the cursor on the left side of the virtual type hint.
I'll work on issue 4 when I find some more time.

Have a nice day.

@p-avital
Copy link
Author

p-avital commented May 22, 2019

Issue 4 fixed :)
I can also confirm that none of the stuff I tried to get the cursor to go left of the annotation worked, so I'm pretty sure we're just gonna have to live with it.

@jannickj
Copy link
Contributor

jannickj commented Aug 7, 2019

why is there no movement on this?, this seems very awesome :)

@jonasbb
Copy link
Contributor

jonasbb commented Sep 14, 2019

@Xanewok It would be great if this PR could get some attention.

@danilaml
Copy link

danilaml commented Apr 2, 2020

I was really missing this feature coming from IntelliJ Rust. I'm frustrated by the lack of movement on this PR. @nrc @Xanewok Is there anything left to be done?

@ryanroe
Copy link

ryanroe commented Apr 2, 2020

@danilaml try rust-analyzer instead

@p-avital p-avital closed this Nov 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants