fix: Rewrite method resolution to follow rustc more closely #20974
Conversation
rustbot
added
the
S-waiting-on-review
| fn loops() { | ||
| check_number( | ||
| r#" | ||
| //- minicore: add, builtin_impls |
There was a problem hiding this comment.
Unlike our old implementation, the rustc implementation for operators require the presence of the trait and an impl even for builtin types. So we need to add them in a bunch of tests.
| fn test() { | ||
| let x: &[isize] = &[1]; | ||
| // ^^^^ adjustments: Deref(None), Borrow(Ref('?2, Not)), Pointer(Unsize) | ||
| // ^^^^ adjustments: Deref(None), Borrow(Ref(Not)), Pointer(Unsize) |
There was a problem hiding this comment.
I removed the region from the borrow adjustment (because rustc lacks it and it's easily retrievable by looking at the target type, plus we've never used it). This requires adjusting (pun intended) everywhere we debug-print adjustments.
| fn infer_slice_method() { | ||
| check_types( | ||
| r#" | ||
| //- /core.rs crate:core |
There was a problem hiding this comment.
Another change is that we assume crates other than the sysroot cannot define incoherent impls (even rustc crates do not use this possibility, and it's an internal feature). This helps narrowing the search space, but it also means tests defining incoherent impls need to pretend to be core or std.
| check( | ||
| r#" | ||
| //- minicore: receiver | ||
| #![feature(arbitrary_self_types)] |
There was a problem hiding this comment.
This is another change, also borrowed from rustc. Receiver will only be considered if the crate activates the arbitrary self types unstable features.
ChayimFriedman2
force-pushed
the
ns4
branch
2 times, most recently
from
7b4d425 to
843470a
Compare
| struct Foo; | ||
| use module::T; | ||
| impl T for usize { | ||
| impl T for Foo { |
There was a problem hiding this comment.
This is another important (breaking) change. Previously, when checking whether a type impls a trait, we considered three blocks: the type's, the trait's, and the current one. I removed the current one, since where we're trait-solving from should have little impact to its result. The non_local_definitions lint also warns against this.
| let $0sub = &s.sub; | ||
| sub[0]; | ||
| let $0x = &s.sub; | ||
| x[0]; |
There was a problem hiding this comment.
The name x is used because the type name is preferred over the field name in name suggestion (maybe we should change that). Previously it was sub because the impl was incorrect (for std instead of core), leaving an error type.
| Either::B => (), | ||
| } | ||
| match loop {} { | ||
| // ^^^^^^^ error: missing match arm: `B` not covered |
There was a problem hiding this comment.
rustc also emits this error, not sure why we didn't previously, but this has something to do with the bits I changed about coercion in match scrutinee.
| // The order is the following: first, if `parent_is_trait == true`, comes the implicit trait predicate for the | ||
| // parent. Then come the explicit predicates for the parent, then the explicit trait predicate for the child, | ||
| // then the implicit trait predicate for the child, if `is_trait` is `true`. | ||
| predicates: EarlyBinder<'db, Box<[Clause<'db>]>>, |
There was a problem hiding this comment.
rustc emits an implicit Self: Trait predicate for a trait (it's not the only implicit predicate, there are also implied bounds, but they're about lifetimes and we don't implement them yet). The solver expects this predicate, as well as other parts of the compiler.
rustc uses more than two duplicate queries for the different kinds of predicates, which is quite wasteful. Instead, I encoded them all in one struct with a clever encoding.
It cannot be exactly the same, because we have needs rustc doesn't have (namely, accurate enumeration of all methods, not just with a specific name, for completions etc., while rustc also needs a best-effort implementation for diagnostics) but it is closer than the previous impl. In addition we rewrite the closely related handling of operator inference and impl collection. This in turn necessitate changing some other parts of inference in order to retain behavior. As a result, the behavior more closely matches rustc and is also more correct. This fixes 2 type mismatches on self (1 remains) and 4 diagnostics (1 remains), plus some unknown types.
|
I've wanted this change for ages, great work as always 🎉 |
ShoyuVanilla
left a comment
ShoyuVanilla
left a comment
There was a problem hiding this comment.
Looks great 👍 I'll look into the analysis-stats failure on CI as well
|
This does seem to slow down analysis stats by quite a bit though |
|
I see why The You can check this with the following test: //- minicore: index, slice
fn sample_array<const N: usize>() {
let indices = [0; N];
let _x = indices[0];
}This passes with no errors in our test suite but if you remove either |
|
So, I think basically we have two options:
|
|
I've already analyzed the failure, it is because we depend on |
4 participants
It cannot be exactly the same, because we have needs rustc doesn't have (namely, accurate enumeration of all methods, not just with a specific name, for completions etc., while rustc also needs a best-effort implementation for diagnostics) but it is closer than the previous impl.
In addition we rewrite the closely related handling of operator inference and impl collection.
This in turn necessitate changing some other parts of inference in order to retain behavior. As a result, the behavior more closely matches rustc and is also more correct.
This fixes 2 type mismatches on self (1 remains) and 4 diagnostics (1 remains), plus some unknown types.