-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Description
A coworker asked this question, and I was little surprised to not have an answer. I'll quote what he said:
let b1: Result<Vec<i32>, anyhow::Error> = vec![1, 2].into_iter().map(|i| Ok(i)).collect(); let b2: Vec<Box<dyn std::error::Error>> = vec!["1".to_owned()].into_iter().map(Into::into).collect();The call to
collect
for b1 made use of impl<A, E, V> FromIterator<Result<A, E>> for Result<V, E>, but there was no way in the IDE for me to discover that fact—not by hover, not by go-to-def.
Similarly, the call toInto::into
for b2 made use of impl From for Box, but again there's no IDE way for me to discover this.[Some details redacted]
- If you hover over
collect
it currently just prints the verbatim declaration of collect, namelypub fn collect<B>(self) -> B where B: FromIterator<Self::Item>
. I think it should also show (1) what B and Item are in this case, (2) the means by which "B: FromIterator" is satisfied. Similarly forInto::into
.- If you cmd-click over
into
then it takes you toimpl<T,U> const Into<U> for T where U:~const From<T> \\ fn into(self) -> U
. I think the IDE, when given a single-method trait likeFrom
, should also include the impl of that method in its list of go-to-def candidates.
The coworker thought (and I personally agree) that it'd be a neat feature for Rust Analyzer to surface these implementations these implementations at the callsite. I'd be happy to implement this feature, as I'm assuming that Chalk has most of this information already. However, I'm struggling to think of a general, principled way of surfacing this information, but I suppose that having rust-analyzer surface just the From
/Into
/TryFrom
/TryInto
impls for a given type is probably reasonable.
Activity
ljw1004 commentedon Oct 10, 2022
Here's my first stab at a general/principled solution.
Background: currently rust-analyzer hover just shows the verbatim declaration, for example:
Proposal 1. Hover on an occurrence of a generic symbol should show how that symbol has been instantiated at the occurrence in question. Most languages perform that substitution inline, but I think it's nicer to show on a separate line.
f4<T>
f4<String>
f4<T> <hr/> T = String
f5<T>
f5<MyTrait>
f5<T> <hr/> T = MyTrait
Typescript, C#, Hack all performs the occurrence generic substitution into the declaration itself.


Proposal 2. Also, if the occurrence of a generic substitution involved some constraints, then hyperlink to the impl where that constraint is satisfied.
f4<T> where T:Into<String>
f4<T> where T:Into<string> <hr/> T = S <hr/> [impl From<S> for String](...)
f5<T> where T:MyTrait
f5<T> where T:MyTrait <hr/> T = S </hr> [impl MyTrait for S](...)
Neither C# nor Typescript show constraints, nor how they're satisfied. Hack does show the constraint, but not how it's satisfied.

I'll show you something interesting though. In class-based languages like C#, Typescript, Hack, it is universal to show the "defining" class. That gives you an indication of where the relevant implementation can be found. I think that showing where the impl has a similar impetus -- it's to show the user where something useful can be found.


I don't know enough about the Rust typesystem to understand all the ways in which a constraint might be satisfied. For instance, I know that if we "impl From" then the "impl Into" comes for free. Where/why does it come for free? I would like to learn that by hovering, and having the hover tooltip educate me.
Proposal 3. Hover types should show "usefully-qualified names". The thing I've found most frustrating is when I hover over a function, and hover shows its return type as "Result", but I don't have a clue what the return type actually is! I have to cmd+click on the function, then scroll up to the top to scan through for something of the form "use xyz::Result". It's quite laborious. It's bad enough that for my team I have banned the common Rust idiom "use xyz::Result" at the top of the file: instead everyone must write out their methods in full using only the standard result type.
anyhow::Result<()>, Result<()>, std::Result<(), anyhow::Error>
even though the three methods all return identical typesHere's how C# implements it. Depending on where you hover, the hover signature for the method

f
shows either "N.C" or just "C" according to how you would have to qualify it here.Veykril commentedon Oct 11, 2022
Regarding David Barsky's second point:
That we can't just do, this could get quickly out of control for blanket implementations referring many traits and it would certainly confuse people at the same time wondering why its showing unrelated references. Nevertheless I agree that there is a feature "hole" here, goto def for into is quite useless (blanket impls in general make goto def and the like not as useful). I believe IntelliJ (I might be misremembering) goes to the blanket impls first, but if you re-invoke the goto thing immediately it will go the actual point of interest. We might want to look into something similar for that.
Regarding the hover ideas (that is David Barsky's first point and ljw1004's comment):
Proposal 1 is basically #11317 certainly something we should have, how we show that info is bikesheddable of course. #3588 is also related and contains some instructions
Proposal 2 is a bit a more tricky to think about, I don't think we want to necessarily overload a hover with information, that is we don't want to pack it so full that you have to start searching for the actual "useful" information (useful being whatever the person might be interested in). This might not be a problem though, we'd see if it works or not if we have something working for that, worse case we could make it configurable.
Proposal 3 is tough, it certainly sounds lovely to have, but I am uncertain if we can currently implement that.
Overall I like the ideas you have outlined here and I think they are certainly something we should try to pursue
This confuses me though, why do you have to scan through things? Why does goto def not work for you here?
flodiebold commentedon Oct 11, 2022
I think a 'general' solution for this would be 'monomorphised go to definition' #8373, which would basically mean that if you go to definition on
Into::into
, we would somehow remember the active type parameters, which would mean that when you click on theFrom::from
call in the impl, we could take you to the correctFrom
impl from the original location. Also, we could e.g. show substituted associated type values and so on. The problem here is of course the 'somehow'; this kind of stateful interaction is really hard to do with LSP (even as an extension).flodiebold commentedon Oct 11, 2022
Another nice-to-have thing I've wanted would be a kind of 'trait query browser' where you can ask 'why does type X implement trait Y' and it shows you a UI that says 'because of impl Z' and lets you drill down further into the where clauses etc., basically visualizing the Chalk recursive solver's solution tree. That's also quite UI-heavy though.
ljw1004 commentedon Oct 11, 2022
Here is my proposal for go-to-def.
Proposal 4. When a method call involves constraint satisfaction, then the syntax that the user wrote which looks like it's using a single definition (that of the receiver method), is actually also using a second definition -- that which justifies how the constraints are satisfied. When we cmd-click "go-to-def" on the method call we should be offered all the definitions which went into it.
f4(S {})
, we're using (1) the definition offn f4
, (2) the definition ofimpl From<S> for String
. So both should be shown as go-to-def targets. Except, as a heuristic, the traitFrom<S>
is a single-method trait, so it's overwhelmingly likely that this method will be invoked, we should instead show (2) the definition of this specificfn from
inside the impl.f5(S {})
, we're using (1) the definition offn f5
, (2) the definition ofimpl MyTrait for S
. So both should be shown as go-to-def targets. Note that MyTrait is a two-method trait, so it doesn't make sense to show either of them.let _b1 = ... collect()
, we're using (1) the definition ofIterator::collect
, (2) the definition ofimpl<A,E,V> FromIterator<Result<A,E>> for Result<V,E>
. Both should be shown. Except the same heuristic kicks in. This is a single-method trait, so we should instead show (2) the definition offrom_iter
inside that that impl.let _b2 = ... collect()
, I honestly don't know enough Rust to point to whichfrom_iter
is being invoked. I hope that with any of the proposals in this issue, then rust-analyzer would teach me!Here's a slightly different example from Hack where GoToDef offers multiple results. Imagine you write
new B()
and the user cmd+clicks on B. It's ambiguous in this context whether the symbol B is referring in the user's mind to the typeclass B
or to the constructor methodfunction B::__construct
. We should notionally offer both targets for gotodef. However, gotodef is kind of unpleasant when it offers multiple targets -- it breaks the user's flow.B::__construct
is better. (It's usually no more than 2-10 lines lower than the class definition also, since the construct is the first thing people write after fields).class B
new B()
. Did they want to see how their arguments would be consumed by the constructor method? Or did they want to see the definition of class B? In this case it's about 50/50 what the user intended to go to, so Hack offers multiple answers to the LSP gotodef request, and VSCode displays them with its characteristic interstitial.I think that Proposal4 will be bad if it shows multiple results but users 99% of the time only want to go to one of the results. My idea is to maybe limit it at first, using it only when the target of the call is from a white-list of standard library calls. I think the initial content of this white-list might be solely
Iterator::collect
andInto::into
.ljw1004 commentedon Oct 11, 2022
@Veykril I'm sure @davidbarsky could tell you more about our company's setup, but our rust-analyzer works fine at providing language services for the project you're working on where it's already warm. But then you ask it to warm itself up and resolve everything it needs to understand a file within stdlib or a third-party crate then it's often either slower at the warmup than I am myself at scrolling through the first 30 lines of the file, or it fails entirely.
In C# they never face this because types imported from stdlib or third-party packages are always exposed to the user as as "signature-only" and the language server is never asked to spin itself up to process/understand/build a third-party crate. Indeed, for C#, my developer machine might not even have installed the tools needed to compile that third-party crate's source code.
I'm not disagreeing with Rust! I think it's wonderful that I can cmd+click into a third-party crate to see how it's implemented. I just think that if my workflow depends upon rust-analyzer being able to work correctly for all stdlib and third-party crate source code then it feels like a higher bar being placed for rust-analyzer than, say, for C#. The proposals in this issue would reduce the bar.
davidbarsky commentedon Oct 11, 2022
fwiw, most of the limitations you might experience today are not rust's or rust-analyzer's fault—they're mine for being too lazy/short-on-time to implement support for the files correctly. looking back, most/all of these are pretty easy to fix and don't require help from the very nice rust-analyzer folks 😄
davidbarsky commentedon Oct 11, 2022
I like this approach quite a bit! Maybe as an initial pass, this component can reuse the existing "view status/HIR" pane as the UI? I'll take a look at Chalk and the existing integration with RA.
flodiebold commentedon Oct 11, 2022
The biggest problem with that one is that it requires either some way of tracing Chalk's solution, or basically replicating all of Chalk's logic.
davidbarsky commentedon Oct 11, 2022
Ah, gotcha! I assumed there was a handy "get me all applicable impls for type
Foo
" in Chalk, primarily because I thought it'd be needed for the rustc error that lists out applicable implementations when the user-provided types don't fit.davidbarsky commentedon Oct 11, 2022
Oh, reading over the Chalk book, I can see how some reflexive implementations could lead to an unbounded search: https://rust-lang.github.io/chalk/book/canonical_queries.html#the-traditional-interactive-prolog-query. That... can throw a wrench into a naive implementation 😅
11 remaining items