Go to definition on 5.to_string() should go to the implemention

[jrmuizel]

In rust-analyzer using "go to definition" on the to_string() part of 5.to_string() takes you to:

pub trait ToString {
    fn to_string(&self) -> String;
}

In IntelliJ it takes you to the more useful:

impl<T: fmt::Display + ?Sized> ToString for T {
    #[inline]
    default fn to_string(&self) -> String {
        use fmt::Write;
        let mut buf = String::new();
        buf.write_fmt(format_args!("{}", self))
           .expect("a Display implementation returned an error unexpectedly");
        buf.shrink_to_fit();
        buf
    }
}

[matklad]

This is currently blocked on chalk -- at the moment, it by design doesn't expose the specific trait impl that matches.

[bstrie]

When you say "by design", do you mean that Chalk has announced the intention to never implement such a feature, or merely that such a feature would be desired but is currently not on the roadmap? If the latter, is there a corresponding issue on the Chalk repo?

[flodiebold]

It's an open design problem; as I understood it, @nikomatsakis agreed we need some way to do this, but alluded to having some particular ideas how it should be done (or how it should not be done). You'd have to ask him for details. I don't think there's an issue in the Chalk repo yet, but I may be wrong.

[matklad]

From what I recall, the idea is to keep the core chalk's interface pure, so that you can only ask "does this type implements this traits", and then leave the "trail of evidence" (which you need to actually codegen) to some extra API.

[nikomatsakis]

Hmm, my plan was to treat this as roughly a kind of associated type normalization. In particular, every method in an impl has a unique type, so the idea was that you would effectively be "normalizing" <T as ToString>::to_string and you would get back either a placeholder (we don't know the impl) or the unique type for the function. There probably isn't much work to implement this in chalk exactly, except to kind of extend the idea of "associated types" beyond types to other sorts of associated members.

[flodiebold]

Ah yeah, that's kind of what I imagined since you mentioned normalization. We wouldn't really need one associated item per method though, would we? Except maybe to handle default methods, but that could also be done in a simpler way...

[nikomatsakis]

@flodiebold right, one per method -- basically each thing in the impl, whether it be a type, fn, or whatever, could be normalized.

[waynr]

Is anyone working on this? And if not, anyone willing to provide me a bit of guidance in making something happen here? I'm practically useless without go to definition in my text editor :(

[avkonst]

I see this ticket received the label Unactionable. I understand it is blocked by Chalk and it is waiting for the Chalk to complete some work. Can we have a reference to a ticket for Chalk here? as @bstrie requested initially.

[flodiebold]

Actually, I think this is actionable without any extensions to Chalk (using Niko's plan), but I would like to get the current transition of hir_ty to Chalk's IR done first.

[flodiebold]

@nikomatsakis One thing though. How would we handle "go to impl" for associated types? I.e. if we have <T as Iterator>::Item, ideally we could go to impl on Item and get taken to the correct impl block as well. We can't model that as simply a projection of Item, since that will just give us the value of the associated type but not where it's defined. I guess with associated constants we could have a fake associated constant representing the impl and project that.

[nikomatsakis]

@flodiebold that's a good point, my plan can't really model that scenario. I suppose we could add something into the logic just for this, basically a kind of projection where you are not finding the value but the impl -- i.e., instead of NormalizedTo(<T as Foo>::bar -> U) we could have SourceImpl(<T as Foo>::bar -> DefId) or something like that.

[matklad]

@flodiebold quick question: can we just hack around this on IDE side? Seems like we can try finding direct impl, and, if that fails, try finding the single blanket impl?

[flodiebold]

Hmm I don't think it's always that easy, consider something like this:

impl<T> Trait for Foo<T> where T: Debug {}

impl<T> Trait for Foo<Baz<T>> {}

And we can already implement it using Chalk, except for associated types.

[matklad]

Yeah, I mean we need chalk support to cover 100% of cases, but it seems that 80% solution is hackable? Most of the impls are just impl Trait for SpecificType.

[lf-]

Yeah, I mean we need chalk support to cover 100% of cases, but it seems that 80% solution is hackable? Most of the impls are just impl Trait for SpecificType.

I concur, when I was reading RA code earlier, I was looking at some stuff like TryToNav and it would have been a nice time save to have this. If you folks want, I can have a crack at implementing the 80% solution this weekend or next.

[matklad]

@lf- go for it!

[flodiebold]

I don't really see the point of doing the 80% solution when we can do the 100% solution for methods.

[matklad]

Ah, sorry, I misunderstood you. If we already can implement this with chalk, then, judging by the amount of upvotes on this issue, we probably should :)

[nikomatsakis]

Hmm so

[nikomatsakis]

I'd like to work with someone to make this work, as an excuse to learn more about rust-analyzer

[nikomatsakis]

Also to prove out the impl strategy I have in mind

[flodiebold]

I could help, but maybe @lf- is interested as well.

[nikomatsakis]

@lf- what do you think? We could setup a time to sketch out how it might work. Ideal for me would be to collaborate with someone who can do most of the coding, just because my time is limited.

[matklad]

Just to throw in some mentoring instructions for all interested parties:

• The most relevant test is this one: https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/ide/src/goto_definition.rs#L359
• It can be extended to cover a trait+impl scenario
• Actually, we probably want two tests: one where the fn is defined by the impl, and one where it is defaulted by the trait.
• The implementation of this functionality lives in the same module. Here’s probably the line that pulls the most weight: https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/ide/src/goto_definition.rs#L88. This line takes the name in the source code, and tries to find it’s “semantic classification”
• “semantic classification” is deliberately not a crisply-defined thing, as as one specific textual identifier can refer to several syntactic things. That’s why the result is an enum with special cases: https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/ide_db/src/defs.rs#L271
• I think, for a proper solution, we want to introduce an extra variant to that enum, which captures name resolution info in detail. This new variant should store a pair of associated function in a trait and an impl (it probably makes sense to have an utility method to go to the corresponding impl’s override)
• In terms of control flow, rather than data types,the current remove logic goes via following steps:
  • https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/ide_db/src/defs.rs#L302
  • https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/hir/src/semantics.rs#L215
  • https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/hir/src/source_analyzer.rs#L148
  • https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/hir_ty/src/infer.rs#L136
  • That is, “resolution” is basically a lookup in the side-table we created during type inference
  • architecture-wise, the important boundary here is the signature of the method on Semantics: it separates “internal compiler API” from “public IDE API”.

[flodiebold]

Inside hir_ty, I'd probably create a new query for this (this doesn't need to happen inside the infer query). We record the substitutions for method calls in the inference result (see the line referenced above), which gives you the self type to resolve the method.

Regarding the Chalk integration, the starting point would be the
https://github.com/rust-analyzer/rust-analyzer/blob/7f9c4a59d9a84cd8c734286937439b5cd215be27/crates/hir_ty/src/chalk_db.rs#L46

[lf-]

@lf- what do you think? We could setup a time to sketch out how it might work. Ideal for me would be to collaborate with someone who can do most of the coding, just because my time is limited.

That sounds fine with me, do you have any thoughts on times? I'm in PST and more or less can accommodate any time in the day or evening except next Monday. We can connect on Zulip to further discuss, I'm @/jade. I'm not an expert in the insides of the IDE, although I've touched most of the mentioned files before. I want to learn about chalk :)

[lf-]

I've had a start at this.

I think, for a proper solution, we want to introduce an extra variant to that enum, which captures name resolution info in detail. This new variant should store a pair of associated function in a trait and an impl (it probably makes sense to have an utility method to go to the corresponding impl’s override)

This is something like a pair of (AssocItem, Impl) where the AssocItem is the one attached to the trait? Is there any particular reason to store it as this pair rather than storing the AssocItem for the item on the impl (besides it being a pain in the neck API wise to get the trait of an impl's AssocItem, from experience)?

---

So if I understand it correctly, we are generating the data (hopefully) correctly, but it's being discarded in here:

https://github.com/rust-analyzer/rust-analyzer/blob/a274ae384e38be5ad1b23cd2b7f2120e5a284209/crates/hir/src/semantics.rs#L570-L572

I guess this is when I have to figure out what I want/need to get out of chalk 🙂

architecture-wise, the important boundary here is the signature of the method on Semantics: it separates “internal compiler API” from “public IDE API”.

Got it.

[flodiebold]

I think what @matklad means is to store a (Function, Function) or maybe (Definition, Definition), where the first refers to the definition in the trait and the second to the implementation. Yes, it's not that hard to go from the latter to the former, but that should already have happened at that point -- the result of classify should contain everything one needs.

So if I understand it correctly, we are generating the data (hopefully) correctly, but it's being discarded in here

Not sure what you mean by that, but currently we don't know the correct impl at all. Implementing that logic is what we need to use Chalk for. What we're discarding there is the self type, but going from the self type to the correct impl isn't completely trivial.

[matklad]

rather than storing the AssocItem for the item on the impl

You might not have the item on the impl:

• when the trait has a default impl.
• when we are talking about T: Trait where there isn't a specific impl

I am not sure what exactly do we want to store, but the API should make it clear that we have two things here. It seems that we need something like (Definition, Option<Definition>), or maybe even some hand-crafted enum that enforces that both elements in the pair are of the same kind (two functions, two types, two consts, etc).

[flodiebold]

You might not have the item on the impl

Also in the case of built-in impls, like the impl of Clone for a tuple.

[lf-]

As you might see, there have some cross references to this issue. I've been working on the Chalk side since July and it's now more or less review ready. I am going to stick it onto rust-analyzer over the coming weeks. This feature is a lot of work 🥲

[qm3ster]

This also leads to the "inline fold" on Chain::fold inlining the bad implementation.

[lf-]

I realized that this didn't get updated on the current state of things. Unfortunately, working on this issue burned me out really badly last summer and I won't have any resources to make it happen.

Based on rust-lang/chalk#716, @compiler-errors was interested in working on the chalk side, but I'm not sure what the status is on that today.

[typetetris]

I realized that this didn't get updated on the current state of things. Unfortunately, working on this issue burned me out really badly last summer and I won't have any resources to make it happen.

I hope you recovered from being burned out on this and it didn't spoil your fun on other rust things!

Thanks for working on rust-analyzer.

[lnicola]

Implemented in #12549, I would say. 🎉