Make region equality emits Eq constraints#155258
Make region equality emits Eq constraints#155258ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
r? lcnr |
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
| //~| ERROR mismatched `self` parameter type | ||
| //~| NOTE expected struct `Foo<'a, 'b>` | ||
| //~| NOTE found struct `Foo<'b, 'a>` | ||
| //~| NOTE lifetime mismatch |
There was a problem hiding this comment.
Other tests are untouched but this has been affected somehow (I guess this might be due to having some duplication with both 'a: 'b, 'b: 'a and 'a == 'b)
I think this might be okay as it's unchanged anyway modulo diagnostics deduplication
There was a problem hiding this comment.
This somehow got returned back again while destructuring eq bounds 😂
This comment has been minimized.
This comment has been minimized.
49b0929 to
8d8e4f3
Compare
This comment has been minimized.
This comment has been minimized.
8d8e4f3 to
4119383
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.
Make region equality emits Eq constraints
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (52727e5): 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.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary -5.3%)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: 489.33s -> 490.845s (0.31%) |
|
Oh, looks like having less number of constraints by collapsing into Eq helps the perf a bit |
compiler/rustc_borrowck/src/type_check/constraint_conversion.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I would keep assumptions as bidirectional outlives and never have Eq in there
can you make sure we always exhaustively match on ConstraintKind 🤔 I do dislike constraint kind as a concept and feel like we should just match on the actual region instead of separately storing that in the ConstraintKind
compiler/rustc_infer/src/infer/region_constraints/leak_check.rs
Outdated
Show resolved
Hide resolved
| /// - We want to uplift bidirectional constraints to the caller instead of unifying them | ||
| /// when solving nested goals, otherwise we often lose implied bounds. | ||
| /// - But we still want to know they are equal from the caller. This is crucial when we | ||
| /// are proving other nested goals which are sensitive to region equalities. |
There was a problem hiding this comment.
this comment is slightly confusing/unhelpful to me right now. It is hard to express why exactly we need this, because really, it is a hack required by the better solution not being possible
"otherwise we often lose implied bounds", what do you mean, what is often, link to a relevant example
I think the framing here isn't quite right. For me the issue is that having existential regions which aren't actually existential is not something which we can support.
Also, "know they are equal" seems slightly odd to me. Know they are equal means "unify existential regions with regions they are equal to, to resolve them to the same thing" here I think? Maybe make that more clear
I also think our conversation on zulip has been useful here, so maybe just link to that 😁
Okay, I'll keep them as they are.
Yeah, I also thought |
|
yeah, I feel like |
35c064f to
0f3c1ee
Compare
| } | ||
|
|
||
| // Index in-edges in reverse order, to match what current tests expect. | ||
| // (It's unclear whether this is important or not.) |
There was a problem hiding this comment.
rustc is such a good codebase 🙃
can you add a fixme to merge these two loops?
There was a problem hiding this comment.
I'm afraid I might be the one who often writes such good codes 😂
| ConstraintKind::RegSubVar => { | ||
| let sup_vid = c.sup.as_var(); | ||
|
|
||
| let reg_sub_var = |sub: Region<'tcx>, |
There was a problem hiding this comment.
instead of making this closures, can you change the iterators to iter over self.data.contraints.iter().flat_map(|constraints| constraints.convert_to_outlives()) which does the eq to bidir outlives mapping.
And then use that everywhere u eq should just be handled as outlives
| // coroutine well-formedness. | ||
| if self.tcx.sess.opts.unstable_opts.higher_ranked_assumptions { | ||
| storage.data.constraints.retain(|(c, _)| match c.kind { | ||
| storage.data.constraints.retain_mut(|(c, _)| match c.kind { |
There was a problem hiding this comment.
hmm, alternatively, can you already do this eq to bidir outlives when constructing the storage and then just always assert we only have outlives constraints everywhere else?
| && outlives_env | ||
| .higher_ranked_assumptions() | ||
| .contains(&ty::OutlivesPredicate(sup_type.into(), sub_region)) | ||
| .contains(&ty::OutlivesPredicate(sup_type.into(), sub_region).into()) |
There was a problem hiding this comment.
these new .into() calls should be unnecessary, are they?
| ConstraintKind::VarSubVar | ||
| | ConstraintKind::RegSubVar | ||
| | ConstraintKind::VarSubReg | ||
| | ConstraintKind::RegSubReg => {} |
There was a problem hiding this comment.
here we could also change the iterator to do a flat_map on map to bidir?
| } | ||
| } else { | ||
| region_constraints.data().constraints.iter().for_each(|(c, _)| each_edge(c.sub, c.sup)) | ||
| region_constraints.data().constraints.iter().for_each(|(c, _)| { |
There was a problem hiding this comment.
and here :3
| deps1.larger.insert(RegionTarget::RegionVid(vid_b)); | ||
| deps1.smaller.insert(RegionTarget::RegionVid(vid_b)); | ||
| } | ||
|
|
There was a problem hiding this comment.
could this also just destructure eq to bidir outlives to avoid this code?
| .flat_map(|(constraint, _)| match constraint { | ||
| ty::RegionConstraint::Outlives(outlives) => iter::once(outlives).chain(None), | ||
| ty::RegionConstraint::Eq(eq) => { | ||
| let [outlives1, outlives2] = eq.into_bidirectional_outlives(); | ||
| iter::once(outlives1).chain(Some(outlives2)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
this match should be a method given that we should use it a bunch :3
| .or_default() | ||
| .push(c.sup); | ||
| } | ||
| } |
There was a problem hiding this comment.
this can also act on the destructured eq bounds :3
| //~| ERROR mismatched `self` parameter type | ||
| //~| NOTE expected struct `Foo<'a, 'b>` | ||
| //~| NOTE found struct `Foo<'b, 'a>` | ||
| //~| NOTE lifetime mismatch |
0f3c1ee to
f36814a
Compare
This comment has been minimized.
This comment has been minimized.
1cce45b to
ee76953
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.
Make region equality emits Eq constraints
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (d50a4bb): 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 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 6.8%, secondary -3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.936s -> 490.267s (0.07%) |
ee76953 to
91fe2b7
Compare
| ConstraintKind::RegSubVar | ||
| | ConstraintKind::RegSubReg | ||
| | ConstraintKind::RegEqReg => {} | ||
| } |
There was a problem hiding this comment.
can you also map use bidir for both of the loops here?
| self.add_constraint( | ||
| Constraint { kind: ConstraintKind::VarEqVar, sub: a, sup: b }, | ||
| origin, | ||
| ); |
There was a problem hiding this comment.
mind adding a fixme that we could only emit this constraints if unify_var_var returns an error?
View all comments
For context, see..
rustc_type_ir::predicate::RegionEqPredicate