feature request: List all applicable From/Into impls

[davidbarsky]

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 to Into::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]

1. If you hover over collect it currently just prints the verbatim declaration of collect, namely pub 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 for Into::into.
2. If you cmd-click over into then it takes you to impl<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 like From, 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.

[ljw1004]

Here's my first stab at a general/principled solution.

Background: currently rust-analyzer hover just shows the verbatim declaration, for example:

use anyhow::Result;

fn f1() -> anyhow::Result<()> { Ok(()) }
fn f2() -> Result<()> { Ok(()) }
fn f3() -> std::Result<(), anyhow::Error> { Ok(()) }
fn f4<T: Into<String>>(x: T) { println!("{}", x.into()); }
fn f5<T: MyTrait>(x: T) { x.foo(); x.bar(); }

fn main() {
    f1().unwrap(); // hover over f1 shows "anyhow::Result"
    f2().unwrap(); // hover over f2 shows "Result"
    f3().unwrap(); // hover over f3 shows "std::Result<(), anyhow::Error>"
    f4(S {}); // hover over f4 shows "f4<T> where T:Into<String>", should also say "T = S. See [impl From<S> for String](...)"
    f5(S {}); // hover over f5 shows "f5<T> where T:MyTrait", should also say "T = S. See [impl MyTrait for S](...)"
}

struct S {}
impl From<S> for String {
    fn from(_: S) -> Self { "s".to_owned() }
}

trait MyTrait {
    fn foo(&self) {}
    fn bar(&self) {}
}
impl MyTrait for S {
    fn foo(&self) {}
    fn bar(&self) {}
}

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.

• hover on f4 currently shows f4<T>
  • inline: would show f4<String>
  • separate line: would show f4<T> <hr/> T = String
• hover on f5 currently shows f5<T>
  • inline: would show f5<MyTrait>
  • separate line: would show 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.

• hover on f4 currently shows f4<T> where T:Into<String>
  • should show f4<T> where T:Into<string> <hr/> T = S <hr/> [impl From<S> for String](...)
• hover on f5 currently shows f5<T> where T:MyTrait
  • should show 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.

• hover on f1, f2, f3 currently show anyhow::Result<()>, Result<()>, std::Result<(), anyhow::Error> even though the three methods all return identical types
• hover should show the simplest representation of the type which would be correct if copied+pasted into the current context

Here'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]

Regarding David Barsky's second point:

2. If you cmd-click over into then it takes you to impl<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 like From, should also include the impl of that method in its list of go-to-def candidates.

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

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"

This confuses me though, why do you have to scan through things? Why does goto def not work for you here?

[flodiebold]

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 the From::from call in the impl, we could take you to the correct From 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]

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]

Here is my proposal for go-to-def.

fn main() {
    f4(S {}); // gotodef on f4 should offer "f4" and "impl From<S> for String :: from"
    f5(S {}); // gotodef on f4 should offer "f5" and "impl MyTrait for S"
    let _b1: Vec<Box<dyn std::error::Error>> = vec!["1".to_owned()].into_iter().map(Into::into).collect();
    // gotodef on collect should offer "Iterator::collect" and "impl<A,E,V> FromIterator<Result<A,E>> for Result<V,E> :: from_iter"
    let _b2: Result<Vec<i32>, anyhow::Error> = vec![1, 2].into_iter().map(|i| Ok(i)).collect();
    // gotodef on collect should offer "Iterator::collect" and <??? I don't know enough rust to say>
}

fn f4<T: Into<String>>(x: T) { println!("{}", x.into()); }
fn f5<T: MyTrait>(x: T) { x.foo(); x.bar(); }

struct S {}
impl From<S> for String {
    fn from(_: S) -> Self { "s".to_owned() }
}

trait MyTrait {
    fn foo(&self) {}
    fn bar(&self) {}
}
impl MyTrait for S {
    fn foo(&self) {}
    fn bar(&self) {}
}

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.

• In f4(S {}), we're using (1) the definition of fn f4, (2) the definition of impl From<S> for String. So both should be shown as go-to-def targets. Except, as a heuristic, the trait From<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 specific fn from inside the impl.
• In f5(S {}), we're using (1) the definition of fn f5, (2) the definition of impl 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.
• In let _b1 = ... collect(), we're using (1) the definition of Iterator::collect, (2) the definition of impl<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 of from_iter inside that that impl.
• In let _b2 = ... collect(), I honestly don't know enough Rust to point to which from_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 type class B or to the constructor method function 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.

• So Hack uses the heuristic that merely taking them to the constructor 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).
• And if there was no explicit constructor written for B, then it just used the automatically-synthesized constructor which the compiler synthesizes based on the class definition, so in this case go-to-def will solely take them to the class definition class B
• BUT... what if, as pictured below, there's a base class with an explicit constructor? Then it's honestly hard to tell what the user wanted when they clicked go-to-def on 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 and Into::into.

• Justification for collect: The Rust docs indeed explain that "Because collect() is so general, it can cause problems with type inference. As such, collect() is one of the few times you’ll see the syntax affectionately known as the ‘turbofish’: ::<>. This helps the inference algorithm understand specifically which collection you’re trying to collect into."
• Justification for Into::into: it's entirely useless showing us the definition of Into::into! We all already know very well what it does! And I think that Into::into is a common enough idiom to need more support.

[ljw1004]

This confuses me though, why do you have to scan through things? Why does goto def not work for you here?

@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]

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.

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]

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.

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]

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]

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]

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 😅