Skip to content

Optimize the user experience when line gets too long after inlay hints are inserted. #3138

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

Closed
crlf0710 opened this issue Feb 14, 2020 · 33 comments · Fixed by #8177
Closed

Optimize the user experience when line gets too long after inlay hints are inserted. #3138

crlf0710 opened this issue Feb 14, 2020 · 33 comments · Fixed by #8177
Labels
A-inlay-hints inlay/inline hints A-vscode vscode plugin issues S-unactionable Issue requires feedback, design decisions or is blocked on other work

Comments

@crlf0710
Copy link
Member

Generally the code is usually formatted with the rustfmt, which keeps the characters per line below a certain limit. However when inlay hints get inserted, the lines sometimes gets too long, or even trimmed by screen edge.

Not sure what's the best solution for this. Maybe when a line will become too long after inlay hints are inserted, move all the hints out of line into a separate line using tiny font sizes as annotations?

@Veetaha
Copy link
Contributor

Veetaha commented Feb 14, 2020

I guess we could opt out showing inlay hints when the length of the line of code goes over some user-defined limit.
Is this something we should tackle from vscode frontend part, @matklad ?

@matklad
Copy link
Member

matklad commented Feb 14, 2020

We can do that, but I am unsure if the cure won't be worse than disease. I guess we should just do whatever IntelliJ does in this case (which IIRC is nothing).

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 14, 2020

It would be great to get the code or at least see the screenshots of such cases.
The one that gets trimmed is a very interesting one.
Maybe there are some things we can shorten, but do not know about them yet: for instance, we still do not use type aliases in the hints.

@crlf0710
Copy link
Member Author

crlf0710 commented Feb 15, 2020

Sure, one example is here:
https://github.com/crlf0710/charlesmine-rs/blob/41c6b473daaeb72cf8e9d6bee53682866374563c/src/controller.rs#L53

This line doesn't look very long by itself, however with inlay hints added, it looks like this:
image
the t is on column 60, and the inlay hint itself has 45 characters. When the left panel is not collapsed, the part after ::Targ|.... is trimmed off.

@SomeoneToIgnore
Copy link
Contributor

That's an interesting case actually, first time ever I see the <_ as _> hint.
I would expect it to show the corresponding enum type instead, so that particular place can be improved, I think.
Not sure how to reproduce it though :)

No good ideas for the general case alas.

@crlf0710
Copy link
Member Author

Create a small repro, don't know if this helps:

trait T1 {
    type O;
}

struct F<X: T1>(X);

impl<X: T1> F<X> {
    fn f<F>(&self, v: F) where F: FnOnce(&X::O) {
        unimplemented!()
    }
}

struct U;
impl T1 for U {
    type O = ();
}


fn main() {
    let s: F<U> = F(U);
    s.f(|z| {});
    println!("Hello, world!");
}

z is annotated: z: &<U as T1>::O here.

@SomeoneToIgnore
Copy link
Contributor

Thanks a lot, small repros help tremendously.

Yet this example does not show any type hints for me since RA fails to infer its type:

image

I wonder, can that be caused by the rustc version you use?
I'm on the latest stable (1.41.0) and the latest RA (58f4dcf)

@crlf0710
Copy link
Member Author

I'm on the latest stable (1.41.0) and this version https://github.com/rust-analyzer/rust-analyzer/tree/2020-02-11 of pre-compiled ra.

@Veetaha
Copy link
Contributor

Veetaha commented Feb 16, 2020

@crlf0710, I wouldn't be so sure about the version of precompiled ra). If you installed 2020-02-10 version of ra and then updated to 2020-02-11 you might not have downloaded the 11 version binary. This issue is tackled in #3162. To be totally sure you just press Ctrl+Shift+P, write > Toggle Developer Tools and you'll see the real version in the console tab:

image

@crlf0710
Copy link
Member Author

Thanks for the instructions! Here is my result.
image

@Veetaha
Copy link
Contributor

Veetaha commented Feb 16, 2020

Hmm, yeah, you do use 2020-02-11 ra_lsp_server release.

@lnicola
Copy link
Member

lnicola commented Feb 18, 2020

I can reproduce this:

image

@SomeoneToIgnore
Copy link
Contributor

On a newer version it started to for me too, weird.

But after I looked at it, I think this particular one is type-inference related, so we better fix that, not the inlay hints.
The type of the closure parameter should be (), shouldn't it?
We need to summon @flodiebold to get a guidance on the issue.

@lnicola
Copy link
Member

lnicola commented Feb 19, 2020

That seems reasonable, but our type shortening doesn't seem to work great in this case:

image

@SomeoneToIgnore
Copy link
Contributor

I don't think this particular case differs from this one:

image

Or, rather, it differs in a way that it's type inreference-related: instead of thinking about the hints shortening for this case, we need to concentrate on type resolution to eliminate all <_ as _>::_ for good from the hints.

As for the long identifier names, I think it's a minor case compared to the complex types with <<<>>>, and those we're already trimming, if the correspnding setting is set.

So no bright ideas currently, especially with intellij-rust having neither also :)

@flodiebold
Copy link
Member

But after I looked at it, I think this particular one is type-inference related, so we better fix that, not the inlay hints.
The type of the closure parameter should be (), shouldn't it?
We need to summon @flodiebold to get a guidance on the issue.

The <X as Y>::Foo is an unnormalized projection. I think these should never appear after type inference; the first thing we supposedly to with any type we get from e.g. type annotations is replace those projections with type variables (this method does it). It may be that we're not doing that in some case, so if you could give a full reproduction that would be nice 🙂

@SomeoneToIgnore
Copy link
Contributor

The full repro is here: #3138 (comment)

It uses FnOnce trait, so might be a bit problematic to use in our tests.
I suspect, that might also be the case why it appears in the first place.

@flodiebold
Copy link
Member

FnOnce isn't a problem, we can just add that in the test.

I think I know what the problem is; the projection appears in a type in a where clause, where we can't normalize it at first. We probably need to normalize associated types in any type we get back from trait solving.

@lnicola
Copy link
Member

lnicola commented Feb 19, 2020

Filed #3232.

@gilescope
Copy link
Contributor

gilescope commented Feb 20, 2020

Would it be super slick if we could use the editor ruler as a guide as to whether we have space on the line to add in the inlays? Nothing would then breach the line.

An alternative is to turn the problem on its head - turn editor.rulers off and instead highlight chars past a set width. (could be done in a separate extension)

@Veetaha
Copy link
Contributor

Veetaha commented Feb 20, 2020

@gilescope though I use a ruller, but time-to-time I trespass. I did think about it, though was not brave enough to propose since not everyone uses rullers, and what if there are more than 1 of them? But anyway the feature can be just controlled by a boolean flag and most strict ruller presence, I am not sure that many users will do use it...

@crlf0710
Copy link
Member Author

With the resolution of #3232, the original motivating case looks much better now! Thanks a lot for this.

If there's no more motivating cases raised up (i don't really have any now), i'm happy to just close this issue as resolved~

@SomeoneToIgnore
Copy link
Contributor

Thanks.
Let's keep thins one open, since the other users have more thoughts on how to shorten the hints.
For example: #3273 (comment)

Maybe we should have some heuristics and limit not only the length of a hint, but the total length of all hints per line. And then either opt out hints completely, or reduce their complexity by displaying less nested types.

@Lucretiel
Copy link

Here's another example for your consideration. The rulers in this screenshot are at 72/80/100 columns.
image

@crlf0710
Copy link
Member Author

A new case:
image

@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).

@lnicola
Copy link
Member

lnicola commented Jan 22, 2021

No, but please mention that in #2797 (comment).

@lnicola lnicola added S-unactionable Issue requires feedback, design decisions or is blocked on other work A-vscode vscode plugin issues labels Jan 22, 2021
@ziimakc
Copy link

ziimakc commented Feb 6, 2021

Another example when using acctix-web:
alt text

@danieljl
Copy link

An example using warp (I even needed to decrease the font size to fit it):

image

@trevyn
Copy link

trevyn commented Mar 25, 2021

@SomeoneToIgnore I don't think that PR actually closes this issue, just another band-aid on it. A better solution would be to dynamically adjust based on the line length.

@rust-lang rust-lang deleted a comment from Hezuikn Oct 21, 2021
@Veykril Veykril added the A-inlay-hints inlay/inline hints label Dec 17, 2021
@ZhangHanDong
Copy link

ZhangHanDong commented Mar 4, 2022

rust-analyzer.inlayHints.maxLength = null:

FM8N3gfaAAIZ9W4

From the twitter: https://twitter.com/spacemeowx2/status/1499432606873812995

@Veykril
Copy link
Member

Veykril commented Oct 11, 2022

I don't think this is in our hands any longer now that we moved to native VSCode inlay hints?

@lnicola
Copy link
Member

lnicola commented Oct 11, 2022

I'm not sure how well rust-analyzer.inlayHints.maxLength works, but I think that might already be tracked in another issue.

@Veykril Veykril closed this as completed Oct 11, 2022
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
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 A-vscode vscode plugin issues S-unactionable Issue requires feedback, design decisions or is blocked on other work
Projects
None yet
Development

Successfully merging a pull request may close this issue.