feat: Show substitution where hovering over generic things #18707
Conversation
rustbot
added
the
S-waiting-on-review
|
The most relevant I think are #13394, where this resolves the first request ( |
There was a problem hiding this comment.
(review for the actual presentation of things follows in a bit, will read up on the issues again first)
One general concern I have is whether this has any perf impact for things that do not care about the substs, that is mainly search and other stuff that does bulk resolution like syntax highlighting (this one you can easily bench via the corresponding integrated benchmark)
That aside, very nice, I've been looking forward to having this for quite some time :)
| /// | ||
| /// This can take three values: `null` means "unlimited", the string `"hide"` means to not show generic substitutions at all, and a number means to limit them to X characters. | ||
| /// | ||
| /// The default is 20 characters. |
There was a problem hiding this comment.
This sounds like a very low limit, 20 works for inlay hints as they are meant to be concise even if it means losing information. The main goal of hovering something is seeing information though so I feel like we should bump this higher.
There was a problem hiding this comment.
I took into account that there may be multiple generic parameters. When browsing with this feature on in the rust-analyzer repository, 20 was enough to see interesting things and not see uninteresting things (e.g. impl Fn, iterators etc.).
There was a problem hiding this comment.
Fwiw I wrote this thinking you'd be substituting the signature that is being shown not listing the substitutions separately in which case this limit is a lot more reasonable now 😅
I was also worried about this, but the nice thing is that it turned out that this information is almost always readily available. The only things we do that we didn't do before are querying for the trait environment (which should be done once then cached), and just a tiny bit of AST juggling and hashmap lookups for the inference type of result. Which should have a marginal impact. |
There are few things to note in the implementation: First, this is a best-effort implementation. Mainly, type aliases may not be shown (due to their eager nature it's harder) and partial pathes (aka. hovering over `Struct` in `Struct::method`) are not supported at all. Second, we only need to show substitutions in expression and pattern position, because in type position all generic arguments always have to be written explicitly.
ChayimFriedman2
force-pushed
the
subst
branch
from
7cc2e9a to
b5486ff
Compare
There are few things to note in the implementation:
First, this is a best-effort implementation. Mainly, type aliases may not be shown (due to their eager nature it's harder) and partial pathes (aka. hovering over
StructinStruct::method) are not supported at all.Second, we only need to show substitutions in expression and pattern position, because in type position all generic arguments always have to be written explicitly.
Closes #18688.
I chose a different approach than outlined in that issue. Instead of substituting the generic arguments inline, I display them separately. This is for two reasons: first, it is harder to inline them. Second, inlining them is a lossy process. For example, it makes where bounds less decipherable, and also it will mean we'll have to switch to show the hir-ty version of the definitions (instead of the hir-def version), which has other implications, e.g. some types are not shown well in hir-ty, and in general hir-def is more like what the programmer wrote, which I think is important for the familiarity of hover.