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 |
This is required now that we send this clause to the solver.
|
Okay, I implemented the required change. |
|
Yeah, I was totally wrong to the CI failure cause 😅 |
|
BTW, the inference time regression - I guess fixing/removing some "wrong" caches like generic predicates would be the cause? - seems a bit concerning, though I'm not rejecting to merging this because of that. (Maybe we could be able to optimize/cache method resolutions later on) |
|
Where are you seeing them regressing? |
This PR on selfhttps://github.com/rust-lang/rust-analyzer/actions/runs/19223234373/job/54945012455?pr=20974 Current master on selfhttps://github.com/rust-lang/rust-analyzer/actions/runs/19223673535/job/54946279380 |
|
That's in debug mode, so basically useless. |
|
I haven't benchmarked the change, however it follows rustc quite closely, so if there is a regression I believe it is necessary. The only exception is method autoderef steps, which in rustc is a query but I didn't want to make a query because of the memory impact. This could theoretically impact other things (more on-demand autoderef), but I don't want to deviate from rustc. Something that changed between rustc and the old version is that rustc does not short-circuit binary operators for builtin types, it does full trait solving for them. I don't know why, but I double-checked and it indeed does not, so I would not want to deviate from it and risk behavioral changes. |
|
I just tested locally with
|
|
Interesting, I do wonder now if method resolution in rustc is indeed slower (and probably needs to be slower) than what we had or it's only in r-a (could be not making a query for autoderef steps, or the fact that looking at the |
|
I also have one fear, rustc doesn't optimize for querying all methods (rather than by name) because it's only for diagnostics, but we use it for completion and it's pretty hot. One thing I noticed is that it re-solves trait constraints for each candidate even when they share the trait, which I'm afraid could lead to a pretty significant slowdown. I am however not sure how to solve it as there is no isolated place where we could group candidates, it's all different from what we had. |
|
Also there are some things rustc does we just didn't, e.g. registering well-formed obligations (which are currently nops, but still go thorough the solver). |
|
Oh, are wfs no-op in next-solver? I was aware of that currently we are not passing them to the solver but it would be okay with giving it enough information as it might be delivered with param envs in most cases 😅 |
|
AFAIK the solver requires us to declare what WF really means (what predicates to elaborate), and we just pass nothing. |
We could check the impact of making this a query, and if it help we could make it an LRU'd query to minimize memory usage regressions. Doubling inference time definitely does not seem great (either way we want to land this change one way or another), but I wonder if there is something simple we are missing for better speed. |
|
I can try a few things to see if I can limit the impact. |
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.