Consider subst in prioritising placeholder inherent methods #14555
Conversation
rustbot
added
the
S-waiting-on-review
| let substs = self.substs_for_method_call(generics, generic_args); | ||
| let substs = match subst { | ||
| Some(s) => { | ||
| // FIXME: here we need the method subst, but result of `lookup_method` incorrectly returns the trait |
There was a problem hiding this comment.
We probably want to look into rustc's probing logic and copy that fully at some point
| #[test] | ||
| fn method_resolution_prefer_explicit_bound() { | ||
| check_types( | ||
| r#" | ||
| trait Tr<T> { | ||
| fn tr(self) -> T; | ||
| } | ||
|
|
||
| impl<T> Tr<T> for T { | ||
| fn tr(self) -> T { | ||
| self | ||
| } | ||
| } | ||
| fn test<T: Tr<i32>>(x: T) { x.tr(); } | ||
| //^^^^^^ i32 | ||
| fn test<T: Tr<i32> + Tr<i64>>(x: T) { let x: i64 = x.tr(); } | ||
| //^^^^^^ i64 | ||
| "#, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yes. I was looking at the unknown types for finding regressions of #14470 and when I found out that this is definitely not a regression, it was too late :)
I don't think it needs chalk support, as it is a method resolution thing. The current implementation also handles the Into2 case mentioned in the issue, I will add a test for it.
There was a problem hiding this comment.
I don't think this solves the general issue unfortunately -- Into2 case that's mentioned in the issue is the following (mostly taken from the playground linked in rust-lang/chalk#584), which still fails with this PR. I don't think we can solve this without overhauling chalk, because it needs to prioritize T: Into<i32> bound when proving subobligation chalk produces internally which we don't have control over.
trait Into<T> {
fn into(self) -> T;
}
impl<T> Into<T> for T {
fn into(self) -> T { loop {} }
}
trait Into2<T> {
fn into2(self) -> T;
}
impl<U, T> Into2<T> for U where U: Into<T> {
fn into2(self) -> T { loop {} }
}
fn test<T: Into<i32>>(t: T) {
let u = t.into2();
//^i32
}There was a problem hiding this comment.
Hmm, you are right. If that issue in chalk becomes solved, our iterate_inherent_trait_methods will become completely unnecessary?
There was a problem hiding this comment.
I believe we need it regardless. That function is for prioritizing some traits (principal trait for dyn, trait bounds for placeholder) to search methods from, whereas the chalk issue is for prioritizing where clauses to be considered when solving obligations.
Consider the code below. Even if we could prove both C: Foo and C: Bar (with or without the chalk issue resolved), we need to pick Bar::f when resolving v.f().
trait Foo {
fn f(self);
}
trait Bar {
fn f(self);
}
fn test<C: Bar>(v: C) {
v.f();
}There was a problem hiding this comment.
Okay, I think what I said above is true, at least that's how rustc is handling things. Since rustc doesn't take expectation into account during method resolution, it does seem to try solving T: Into<?0> rather than T: Into<T> and go into the steps I described above.
There was a problem hiding this comment.
It seems you are correct. So, in the rustc code there is no usage of <i32> part of the Into<i32> until trait solving? And we can prioritize methods of Into<_> and the rest will be handled by trait solver? I also tried method where bounds, but it seems they don't affect resolution, which confirms this. In the function you linked from rustc it is passing trait_refs, but it seems it later only uses the trait id?
There was a problem hiding this comment.
Eh, so turns out I wasn't fully correct in regards to substitution i.e. <i32> part. assemble_inherent_candidates_from_param() records trait ref as you pointed out, and its substitution seems to be used to instantiate obligations rustc feeds into its trait solver. But I don't think it matters much, because the solver can only prove T: Into<i32> anyway due to prioritization.
There was a problem hiding this comment.
So, the current approach either should be completely removed, or only plays a small rule in the ideal solution. From that question, I wanted to know that does it worth to keep this as a workaround? It appears to work for the basic case of impl Into<i32>, but it makes our code unnecessarily ugly. So if you think the ideal solution is accessible in near future, we should wait for that and abandon this.
And if we decide to go with this as a workaround, we should check that this behaviour is a subset of the rustc's, and doesn't introduces new false positive type mismatches.
There was a problem hiding this comment.
Yeah, I don't think we should do this. This needs to be fixed in Chalk.
|
☔ The latest upstream changes (presumably #14578) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing per the discussion. I still think it is an acceptable workaround for an old and high impact problem, but I see the value in complete solutions. |
Reduces unknown types on self to 18