Add inlay hints backend into the server #1549

SomeoneToIgnore · 2019-07-19T09:05:34Z

Types that are fully unresolved are not displayed:

A few concerns that I have about the current implementation:

I've adjusted the file_structure API method to return the information about the let bindings.
Although it works fine, I have a feeling that adding a new API method would be the better way.
But this requires some prior discussion, so I've decided to go for an easy way with an MVP.
Would be nice to hear your suggestions.
There's a hardcoded {undersolved} check that I was forced to use, since the method that resolves types returns a String.
Is there a better typed API I can use? This will help, for instance, to add an action to the type lenses that will allow us to navigate to the type.

lnicola · 2019-07-19T09:10:21Z

Do you have a more realistic sample? I fear this can be too distracting, there should probably be a preference for it. Also, how does it handle let (x, c) = (42, 'a');?

I'm not sure why RA didn't resolve SS in your second let, but it might be unrelated.

SomeoneToIgnore · 2019-07-19T09:23:31Z

I'm not sure why RA didn't resolve SS in your second let, but it might be unrelated.

Looks like RA does not resolve for cases when the structure is a method local one. Works fine with the regular structure.

Also, how does it handle let (x, c) = (42, 'a');?

Nice catch, it displays nothing. Should be easy to fix though.

Here's an updated example:

One more thing that needs improving is an already defined parameter.
But both this one and the let (x, c) = (42, 'a') one are relatively easy to fix if we decide to continue with the feature.

Do you have a more realistic sample?

Here you go, the analyzer itself:

I fear this can be too distracting, there should probably be a preference for it.

Fully agree, the setting is good to have, but I'm more concerned about the API changes and the overall correctness of the implementation, so let's get over it first :)

SomeoneToIgnore · 2019-07-19T09:28:35Z

Notice another funny RA type inference artifact: [missing name] close to the bottom of the last screenshot.
I get the same string in the hover tooltip.

This is another reason why it would be cool to have a way to get an actual type information, not just a string.

lnicola · 2019-07-19T09:34:48Z

GitLens has a kind of annotation that shows up at the end of the line:

Are you familiar with those? Could they be used to get the same UI as IntelliJ has?

bjorn3 · 2019-07-19T09:36:13Z

Maybe hide the code lens by default when the type is obviously known to the programmer.

struct SS {}

// the type of test is obviously SS.
let test = SS {};

// the type of num is obviously u8.
let num = 5u8;

// should we hide the type here? not everybody knows that this defaults to `i32`.
let num = 5;

SomeoneToIgnore · 2019-07-19T09:42:29Z

Are you familiar with those? Could they be used to get the same UI as IntelliJ has?

Nope alas I'm not that good with knowing about VS Code features.
If I understood correctly, the GitLens message is on the right side of the line, right?
I like that this way it does not add an extra line break to the editor, but otherwise I find it questionable:
the lines can be relatively long hence I think it's hard to read the type and its name.
Also, if any action will be introduced to the lens, it's harder to click it on the right.
But it might become another setting, why not.

If we can display the way IntelliJ does it, it would be really awesome, but no idea if it's possible with the existing lenses.

SomeoneToIgnore · 2019-07-19T09:44:31Z

Maybe hide the code lens by default when the type is obviously known to the programmer.

Hard to say, I find it hard to determine the rules of obviousness, especially in Rust.
In my opinion, we'd better not guess and display all types that we can or nothing.

lnicola · 2019-07-19T09:55:41Z

If we can display the way IntelliJ does it, it would be really awesome, but no idea if it's possible with the existing lenses.

They seem to be normal decorations ~~with a hover (?) message,~~ placed at the end of the line. I don't know whether they work in the middle of a line, but it doesn't seem unreasonable: ~~https://github.com/eamodio/vscode-gitlens/blob/master/src/annotations/recentChangesAnnotationProvider.ts#L55-L84~~ https://github.com/eamodio/vscode-gitlens/blob/e43881989c867533f7795bac652a76505825846f/src/annotations/blameAnnotationProvider.ts.

Hard to say, I find it hard to determine the rules of obviousness, especially in Rust.

If the expression on the right is a literal or a type constructor application (not sure how they are called), the type is more or less obvious -- that is, if you ignore inference for integral types, for example. If it's a function call, it's not.

SomeoneToIgnore · 2019-07-19T10:23:44Z

I don't know whether they work in the middle of a line, but it doesn't seem unreasonable

Sounds nice, but if I understand correctly, there's no way to do this with the current api provided by the lst-types crate.
There's only one lens object: https://github.com/gluon-lang/lsp-types/blob/master/src/lib.rs#L3008
I've tried to fiddle with its range parameter, but nothing forced it to move out of the left side of the line.

lnicola · 2019-07-19T10:34:35Z

Sounds nice, but if I understand correctly, there's no way to do this with the current api provided by the lst-types crate.

It's not a lens, it's a decoration with isWholeLine set, which makes Code render it at the end somehow. So it looks like it's not possible to put them in the middle, but @matklad might know for sure.

flodiebold · 2019-07-19T10:56:13Z

Notice another funny RA type inference artifact: [missing name] close to the bottom of the last screenshot.
I get the same string in the hover tooltip.

This is most likely related to a hacky bit of the Chalk integration: We keep track of the name of a type parameter inside the type, but Chalk doesn't, so if the type parameter goes through Chalk, it loses the name. This needs refactoring to get rid of the name on our side as well, and instead find it out from the context when printing the type...

Although in this function, there is no type parameter, so there might be something else going wrong...

p-avital · 2019-07-19T13:22:11Z

Hey, I actually did an implementation for this on the vscode-rls project, here's the PR I made :)

I had also tried with lenses, but ended up finding out like you seem to have that fiddling with their range does little more than order them if the are on the same line.

Decorators can be placed in line just fine, but they are completely client side. My implementation on vscode-rls is more of a hack if anything: it looks for stuff that looks like a declaration (let, for, closures...), and uses Hover requests to get the type names.

Feel free to cherry pick the parts that seem useful to you. I would have given it a shot if ra-lsp had less cases of {unknown}, but I guess some of you are less patient than I am :)

lnicola · 2019-07-19T13:38:46Z

So the salient point here is that an inline text decoration can be inserted using the before (or after?) render options.

matklad · 2019-07-19T14:26:10Z

Excellent, we really should do this ❣️

Though, I expect we should do things slightly differently.

First, on the back end, we should make this a custom request, as suggested by @SomeoneToIgnore . Something like ra-lsp/hints should work:

type HintsResponse = Vec<Hint>

struct Hint {
    text_range: TextRange,
    label: String
}

This should be added as a method to Analysis in ra_lsp_api and threaded through handlers.

Second, on the front-end, I feel like we should explore @p-avital approach.

This is maybe just me, but I find the whole idea of code-lenses, that shift code vertically, utterly horrendous. The IntelliJ solutions for this (icons in the gutter area) is so much more convenient. OTOH, I am not against having lenses as a rendering option, if it is behind a config flag.

@SomeoneToIgnore let's perhaps star with just backed bit, which can be tested independently?

SomeoneToIgnore · 2019-07-19T19:13:03Z

Wow, nice work there @p-avital !
Not sure if I'll be able to salvage anything, but I'll try :)

This should be added as a method to Analysis in ra_lsp_api and threaded through handlers.

Any tips on the method name and semantics?
If we plan to reuse CLion's approach, it seems like we need the data on all let bindings and closure parameters from from the file.
fn file_type_hints?

@SomeoneToIgnore let's perhaps star with just backed bit, which can be tested independently?

Ok. In the worst case, we can throw away the rendering part from the PR.
As far as I understand, it is not really trivial to implement the lens settings due to #1355

matklad · 2019-07-19T19:27:03Z

I'd do the following:

pub struct InlayHint {
    range: TextRange,
    text: SmolStr, // invariant: does not contain `\n`
}

impl fmt::Display for InlayHint { ... }

pub fn inlay_hints(&self, file_id: FileId) -> Vec<InlayHint> {
     ....
}

I'd like to be open ended about semantics: we certainly should start with type hints, but it could be the case that we invent something else useful as well. IIRC, Kotlin shows inlay hints for lambda returns, for example:

SomeoneToIgnore · 2019-07-20T20:52:59Z

One step closer, I hope: the new API added, along with a *.snap test.
Now also gather the closure parameters information.

I have a small doubt about the InlayHint.text field: now I put the corresponding node text there, but it's rather useful for the API user, since he gets the range already.
On the other hand, this text is helpful for debugging and checking the test snap file.

matklad

Scaffold looks about right, though the cusom LSP request is missing.

To get the actual types, you'll need to use source_analyzer. Take a look at syntax highlighting implemenation: it also uses this "walk the tree, get the types" approach

crates/ra_ide_api/src/inlay_hints.rs

crates/ra_ide_api/src/lib.rs

matklad · 2019-07-20T21:10:08Z

crates/ra_lsp_server/src/main_loop/handlers.rs

@@ -749,6 +750,29 @@ pub fn handle_code_lens(
            }),
    );

+    lenses.extend(


This will be moved eventually into a separate LSP requestion right?

Right, removed now to avoid confusion.
I use it currently to do some sort of a final integration testing.

SomeoneToIgnore · 2019-07-21T19:39:16Z

though the cusom LSP request is missing.

Oh, sorry, I haven't understood your idea fully before.
So you would like the client side to make a custom request to get the inlay hints?
As I understood, the rust-lang/vscode-rust#587 PR is based on the decorators, should we return those on DecorationsRequest?
I also think it would be nice to be able to respond to some standard request of the client, so that it loads the hints seamlessly.

SomeoneToIgnore · 2019-07-21T21:01:06Z

To get the actual types, you'll need to use source_analyzer. Take a look at syntax highlighting implemenation: it also uses this "walk the tree, get the types" approach

Nice, now the resolution happens on the server.
Two things that confuse me with this approach:

As far as I understood, we have to create the SourceAnalyzer instance with the SyntaxNode of the exact node that we need to get the type for, is it true? I've tried creating it once for the whole SourceFile used as a SyntaxNode source, but it did not resolve any types this way.
Looks like mock_analysis::single_file does not work exactly the same way as the actual plugin does: in my tests, it did not resolve any Vec type even though it resolves them in the actual VS Code.
Unfortunately, I cannot call the inlay_hints method directly anymore since I need the RootDatabase parameter in it now (due to the SourceAnalyzer).

Otherwise I've adjusted everything except the cusom LSP request thing that's to be discussed.

lnicola · 2019-07-21T21:02:41Z

As far as I understood, we have to create the SourceAnalyzer instance with the SyntaxNode of the exact node that we need to get the type for, is it true? I've tried creating it once for the whole SourceFile used as a SyntaxNode source, but it did not resolve any types this way.

I think you can create one per each function and reuse it inside of it. There's a TODO in the syntax highlighting to reuse them.

SomeoneToIgnore · 2019-07-21T21:10:10Z

I think you can create one per each function and reuse it inside of it. There's a TODO in the syntax highlighting to reuse them.

I see only https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_ide_api/src/syntax_highlighting.rs#L99 there and it creates the analyzer for every node, I think, due to for node in root.descendants_with_tokens() line.

lnicola · 2019-07-21T21:13:01Z

Yeah, that's the one.

SomeoneToIgnore · 2019-07-21T21:17:32Z

Thanks for the tip, let's wait for @matklad first, it might be that I'm doing something wrong and have to change the code.

crates/ra_ide_api/src/inlay_hints.rs

matklad · 2019-07-22T10:46:58Z

I think you can create one per each function and reuse it inside of it. There's a TODO in the syntax highlighting to reuse them.

Yeah, that's true, SourceAnalyzers are per function. But it shouldn't be a huge problem to create them per element.

Looks like mock_analysis::single_file does not work exactly the same way as the actual plugin does:

yeah, it doesn't have access to stdlib (to make the test fast). You can mock out the relevant parts of stdlib though:

struct Vec<T> {
    fn new() -> Self {}
    fn push(&mut self, x: T) {}
}

I also think it would be nice to be able to respond to some standard request of the client, so that it loads the hints seamlessly.

The problem is, there's not standard request for this. You could use lenses, but than you'll lose the nice UI. I still think that it makes sense to make this a fully-custom request, and not to piggyback it onto any existing one.

SomeoneToIgnore · 2019-07-22T20:34:05Z

yeah, it doesn't have access to stdlib (to make the test fast). You can mock out the relevant parts of stdlib though:

Did not work for me alas.
I've tried this:

struct Vec<T> {
    test: T,
}

impl<T: Default> Vec<T> {
    fn new() -> Vec<T> {
        Vec { test: Default::default() }
    }

    fn push(&mut self, value: T) {
    }
}

and it did not give me any new resolutions. Since there are some types from iterators that won't be displayed anyway, I propose to leave the tests as is.
I think it would be great to be able to add stdlib into some tests to check the type inference for the standard library types.

I still think that it makes sense to make this a fully-custom request

Ok, added one. Since there's no serde in the ra_ide_api, I had to add similar to inlay_hints structures to the ra_lsp_server.

matklad · 2019-07-23T09:39:29Z

I've tried this:

There's no Defatult trait (b/c there's no stdlib), so the impl doesn't apply.

I think it would be great to be able to add stdlib into some tests to check the type inference for the standard library types.

The best solutions here is probably to come up with a set of reusable stdlib stubs. Adding literal standard library will make tests much slower and brittle. We have one integration tests that checks that stdlib is hooked up appropriately, and that should be enough.

matklad · 2019-07-23T09:51:25Z

Ok, added one. Since there's no serde in the ra_ide_api, I had to add similar to inlay_hints structures to the ra_lsp_server.

Yeah, we intentionally maintain a strict separation between rust-analyzer types and language server protocol types, to make sure that rust-analyzer is usable without the LSP protocol

bors r+

Thanks!

1549: Show type lenses for the resolved let bindings r=matklad a=SomeoneToIgnore Types that are fully unresolved are not displayed: <img width="279" alt="image" src="https://user-images.githubusercontent.com/2690773/61518122-8e4ba980-aa11-11e9-9249-6d9f9b202e6a.png"> A few concerns that I have about the current implementation: * I've adjusted the `file_structure` API method to return the information about the `let` bindings. Although it works fine, I have a feeling that adding a new API method would be the better way. But this requires some prior discussion, so I've decided to go for an easy way with an MVP. Would be nice to hear your suggestions. * There's a hardcoded `{undersolved}` check that I was forced to use, since the method that resolves types returns a `String`. Is there a better typed API I can use? This will help, for instance, to add an action to the type lenses that will allow us to navigate to the type. Co-authored-by: Kirill Bulatov <[email protected]>

lnicola · 2019-07-23T09:52:45Z

@SomeoneToIgnore do you have a screenshot at hand? Don't sweat over it if now.

lnicola · 2019-07-23T09:54:58Z

Ah, never mind. It looks like there's no UI yet.

SomeoneToIgnore · 2019-07-23T09:55:45Z

Nice, thanks for the review.

Yes, there's no UI yet, I'll continue with that.

bors · 2019-07-23T10:08:29Z

Build succeeded

continuous-integration/travis-ci/push

1591: Make Analysis api cancellable r=matklad a=SomeoneToIgnore Based on the discussion from here: #1549 (comment) Co-authored-by: Kirill Bulatov <[email protected]>

1586: Add type decorators r=matklad a=SomeoneToIgnore A follow-up of #1549 Now the frontend shows inlay hints as VS Code Decorators: <img width="666" alt="image" src="https://user-images.githubusercontent.com/2690773/61802687-918fcc80-ae39-11e9-97b0-3195ab467393.png"> <img width="893" alt="image" src="https://user-images.githubusercontent.com/2690773/61802688-93599000-ae39-11e9-8bcb-4512e22aa3ed.png"> A few notes on the implementation: * I could not find a normal way to run and display the hints for the file that's already open in the VS Code when it starts. The updating code runs ok, but does not actually show anything. Seems like I miss some event that I could add a handler to. I've also experimented with `setTimeout` and it worked, but this is too ugly. The hints appear now when a new file is open or when some change is done in the existing file. * If there's a `dbg!` used in the lsp_server, the frontend starts receiving change events that contain the string from the `dbg!` output. It should not be the case in a real life, but I've decided to cover this case, just in case. * For bigger files, ~500 lines, the decorators start to blink, when updated, this does not seem to be very much of a problem for me at this particular stage of the feature development and can be optimized later. In the worst case, those decorators can be turned off in settings. * Cursor movement is rather non-intuitive on the right edge of the decorator. Seems like a thing to fix in the VS Code, not in the plugin. Co-authored-by: Kirill Bulatov <[email protected]>

SomeoneToIgnore force-pushed the add-type-lenses branch from 9987fce to 50d9a8d Compare July 19, 2019 12:47

lnicola mentioned this pull request Jul 20, 2019

Function-local types are not resolved #1559

Closed

SomeoneToIgnore added 5 commits July 20, 2019 21:39

If possible, show type lenses for the let bindings

b6c662c

Fix rebase issue

1ce864a

Do not show a lens when the type is declared explicitly

dfa8373

Fix tuple type lens resolution

761fc71

Refactor server api

201b344

SomeoneToIgnore force-pushed the add-type-lenses branch from 50d9a8d to 201b344 Compare July 20, 2019 20:46

matklad reviewed Jul 20, 2019

View reviewed changes

Code review fixes

24784c6

SomeoneToIgnore added 2 commits July 21, 2019 23:44

Resolve types on the server

09c7c86

Do not show the lens with type hints

ba76017

Merge branch 'master' into add-type-lenses

31aef80

Fix the string conversions

d32774b

Use SmolStr for the type text

25398ad

matklad reviewed Jul 22, 2019

View reviewed changes

crates/ra_ide_api/src/inlay_hints.rs Outdated Show resolved Hide resolved

crates/ra_ide_api/src/inlay_hints.rs Outdated Show resolved Hide resolved

crates/ra_ide_api/src/inlay_hints.rs Outdated Show resolved Hide resolved

crates/ra_ide_api/src/inlay_hints.rs Outdated Show resolved Hide resolved

Code review fixes

8f3377d

SomeoneToIgnore changed the title ~~Show type lenses for the resolved let bindings~~ Add inlay hints backend into the server Jul 23, 2019

bors bot merged commit 8f3377d into rust-lang:master Jul 23, 2019

SomeoneToIgnore deleted the add-type-lenses branch July 23, 2019 10:08

This was referenced Jul 24, 2019

Add type decorators #1586

Merged

Make Analysis api cancellable #1591

Merged

bors bot added a commit that referenced this pull request Jul 25, 2019

Merge #1591

ceb1659

1591: Make Analysis api cancellable r=matklad a=SomeoneToIgnore Based on the discussion from here: #1549 (comment) Co-authored-by: Kirill Bulatov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inlay hints backend into the server #1549

Add inlay hints backend into the server #1549

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019 •

edited

Loading

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

bjorn3 commented Jul 19, 2019 •

edited

Loading

SomeoneToIgnore commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019 •

edited

Loading

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

flodiebold commented Jul 19, 2019 •

edited

Loading

p-avital commented Jul 19, 2019 •

edited

Loading

lnicola commented Jul 19, 2019

matklad commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019

matklad commented Jul 19, 2019

SomeoneToIgnore commented Jul 20, 2019

matklad left a comment

matklad Jul 20, 2019

SomeoneToIgnore Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

lnicola commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

lnicola commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

matklad commented Jul 22, 2019

SomeoneToIgnore commented Jul 22, 2019

matklad commented Jul 23, 2019

matklad commented Jul 23, 2019

lnicola commented Jul 23, 2019 •

edited

Loading

lnicola commented Jul 23, 2019

SomeoneToIgnore commented Jul 23, 2019

bors bot commented Jul 23, 2019

Add inlay hints backend into the server #1549

Add inlay hints backend into the server #1549

Conversation

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019 • edited Loading

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

bjorn3 commented Jul 19, 2019 • edited Loading

SomeoneToIgnore commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019 • edited Loading

SomeoneToIgnore commented Jul 19, 2019

lnicola commented Jul 19, 2019

flodiebold commented Jul 19, 2019 • edited Loading

p-avital commented Jul 19, 2019 • edited Loading

lnicola commented Jul 19, 2019

matklad commented Jul 19, 2019

SomeoneToIgnore commented Jul 19, 2019

matklad commented Jul 19, 2019

SomeoneToIgnore commented Jul 20, 2019

matklad left a comment

Choose a reason for hiding this comment

matklad Jul 20, 2019

Choose a reason for hiding this comment

SomeoneToIgnore Jul 21, 2019

Choose a reason for hiding this comment

SomeoneToIgnore commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

lnicola commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

lnicola commented Jul 21, 2019

SomeoneToIgnore commented Jul 21, 2019

matklad commented Jul 22, 2019

SomeoneToIgnore commented Jul 22, 2019

matklad commented Jul 23, 2019

matklad commented Jul 23, 2019

lnicola commented Jul 23, 2019 • edited Loading

lnicola commented Jul 23, 2019

SomeoneToIgnore commented Jul 23, 2019

bors bot commented Jul 23, 2019

Build succeeded

SomeoneToIgnore commented Jul 19, 2019 •

edited

Loading

bjorn3 commented Jul 19, 2019 •

edited

Loading

lnicola commented Jul 19, 2019 •

edited

Loading

flodiebold commented Jul 19, 2019 •

edited

Loading

p-avital commented Jul 19, 2019 •

edited

Loading

lnicola commented Jul 23, 2019 •

edited

Loading