enable eager param_env norm in new solver#156976
Conversation
|
|
9cef0c9 to
62e38a9
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
enable eager `param_env` norm in new solver
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (13060c6): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.5%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 511.751s -> 510.766s (-0.19%) |
|
r? @jackh726 as Boxy is on leave this week |
This comment has been minimized.
This comment has been minimized.
|
r=me with green CI |
This comment has been minimized.
This comment has been minimized.
|
This PR changes a file inside |
d52b353 to
db4b783
Compare
|
The only user facing change covered by this FCP is dd884e4: we now entirely ignore region errors when normalizing the This change causes #136661 to compile with the old solver, see the added tests in the last commit: //@ check-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver
// In `fn hrtb` normalizing the elaborated where-clause
// `T: for<'a> Supertrait<<&'a () as WithAssoc>::As>` results in
// a region error as `&'a ()` is not `&'static ()`.
//
// This happens even though the where-clauses themselves are well-formed
// as we never have to prove `&'a (): WithAssoc` as `'a` is a bound variable.
//
// Unlike `normalize-param-env-missing-implied-bound-1.rs` this snippet caused
// an ICE with the old trait solver, as we did register region constraints from
// relating types. This ICE was tracked in #136661.
#![allow(unused)]
trait Supertrait<T> {}
trait Other {
fn method(&self) {}
}
impl WithAssoc for &'static () {
type As = ();
}
trait WithAssoc {
type As;
}
trait Trait<P: WithAssoc>: Supertrait<P::As> {
fn method(&self) {}
}
fn hrtb<T: for<'a> Trait<&'a ()>>() {}With this PR, we entirely ignore region constraints from The core issue here is that the old solver already accepted code like this if the constraint was an explicit outlives clause instead, see I am slightly unhappy about this as it feels like fully ignoring region constraints here could be unsound in theory. I haven't been able to come up with an exploit for this issue and even if one exists, it should only be easier to trigger as we already ignore explicit outlives where-clauses on stable. This PR only causes us to also ignore region constraints emitted by type relations. In the medium term this should be fixed by proper assumptions on binders. @steffahn @theemathas would be interesting to see whether u can exploit this issue. To summarize:
Given my reasoning in the above paragraph, I would merge this as is:
@rfcbot fcp merge types |
|
@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
the old solver doesn't register `TypeOutlives` and `RegionOutlives` goals if `ignoring_regions` is set. The new solver always registers these requirements, so we instead ignore errors caused by these constraints.
db4b783 to
e7e6758
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/goodbye.20proper.20param_env.20normalization/near/594260464
Slightly unfortunate that this is what we ended up with. but for now, this fixes issues and landing the new solver with this is certainly not worse than keeping the old impl.
This PR only eagerly normalizes, it does not yet treat aliases in
param_envcandidates as rigid, which is the worse part of this change.fixes rust-lang/trait-system-refactor-initiative#265
fixes #136661
r? @BoxyUwU