abby: always store nextgen region constraints in canonical form - #161306
Open
BoxyUwU wants to merge 8 commits into
Open
abby: always store nextgen region constraints in canonical form#161306BoxyUwU wants to merge 8 commits into
BoxyUwU wants to merge 8 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
5 times, most recently
from
August 21, 2026 13:36
d860264 to
c64bf00
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
from
August 21, 2026 14:12
c64bf00 to
57b2d5e
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
from
August 21, 2026 14:38
57b2d5e to
c17800a
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
2 times, most recently
from
August 21, 2026 14:52
aba4012 to
56320f9
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
2 times, most recently
from
August 21, 2026 15:07
5bedfe3 to
65f3b73
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
from
August 21, 2026 16:15
a10cd16 to
930ca67
Compare
This comment has been minimized.
This comment has been minimized.
Member
Author
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
from
August 21, 2026 16:26
930ca67 to
1bead00
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
marked this pull request as ready for review
August 21, 2026 16:42
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Sep 2, 2026
lcnr
reviewed
Sep 2, 2026
| } | ||
|
|
||
| new_constraints | ||
| RegionConstraint::new_from_or( |
Contributor
There was a problem hiding this comment.
also horrible for perf xd, pls FIXME
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
6 times, most recently
from
September 2, 2026 12:26
989777a to
4eebcea
Compare
BoxyUwU
force-pushed
the
abby_canonical_form_always
branch
from
September 2, 2026 12:38
4eebcea to
b22bffa
Compare
Collaborator
|
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. |
Member
Author
|
@bors r+ rollup=never |
Contributor
Member
Author
|
@bors r=lcnr,khyperia |
Contributor
Contributor
|
⌛ Testing commit b22bffa with merge d8df826... Workflow: https://github.com/rust-lang/rust/actions/runs/33744111368 |
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 3, 2026
…yperia
abby: always store nextgen region constraints in canonical form
title. introduce an `And`/`Or`/`LeafConstraint`/ types to reason about the structure of our region constraints. Never produce arbitrarily nested or/ands and always have constraints in an evaluated form.
I kinda mucked up this PR and accidentally did two things at the same time. Not only do we immediately put everything into canonical form, we also change what it means for a region constraint to be in canonical form. Whoops :>
Rough overview of what a `RegionConstraint` is:
- `RegionConstraint` contains two things: an `AND of LEAFs` and an `OR of AND of LEAFs`. Another way of thinking about it would be to say its an `AND` consisting of arbitrarily many LEAFs and a single `OR of AND of LEAFs`
- There are never any region constraints shared between all ANDs of the OR, instead they're moved into the top level AND
- If the OR constraint is `false` then we wipe the top level `AND` as it doesn't matter what they are, the constraint is always going to be false
- ORs never have two equivalent ANDs within them. Similarly, ANDs never have two equivalent LEAFs within them
this simplifies a lot of things conceptually as we now no longer need to worry about what state our region constraints are in. and our algorithms also don't need to handle arbitrary nesting of ors/ands :) and its a lot easier to read the debug logs 😅
I also wound up needing to do this while trying to compile `std`/`core` with `-Zassumptions-on-binders` as we would otherwise OOM from having both:
- Lots of duplicate region constraints (e.g. `And('a: 'b, 'a: 'b)`)
- Lots of region constraints shared across all elements of an `OR` (e.g. `Or(And('a: 'b, 'b: 'c), And('a: 'b, 'b: 'd))`)
Some future work:
- Remove `RegionConstraint::splatted_and_constraints` it's kind of weird to even need it and probably encourages bad-for-perf patterns
- we Probably want some kind of fast path for pushing new leaf constraints to the region constraint storage. slash have a way to register a leaf constraint directly rather than having to make a `RegionConstraint`. Perf stuff :3
In theory this PR should mostly not have functional changes. In practice it might affect some things due to changing the exact repr of things affecting query responses. There's probably also some behaviour differences here due to us falling on our face more or less in WIP parts of abby now that we have different region constraints. I don't think any of this should be meaningful though. This PR is intended to not fundamentally change the abby algorithm :3
This PR should be reviewed commit-by-commit. There are a bunch of commits restructuring existing logic to assume their input is in canonical form as it will be by the end of the PR.
Then there's the core change in `always canonical form` which actually replaces `RegionConstraint` with all the new types and updates all the locations using them.
Finally there's `propagate ambiguity not evaluate` which deals with the leftover `evaluate_solver_constraint` which was mostly unnecessary now due to moving its main logic into construction of `RegionConstraint` and friends. I didn't want to make actual bug fixes in this PR so I just left some FIXMEs about some of the issues that `propagate_ambiguity` has instead of fixing them here.
Fixes rust-lang/project-assumptions-on-binders#14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
title. introduce an
And/Or/LeafConstraint/ types to reason about the structure of our region constraints. Never produce arbitrarily nested or/ands and always have constraints in an evaluated form.I kinda mucked up this PR and accidentally did two things at the same time. Not only do we immediately put everything into canonical form, we also change what it means for a region constraint to be in canonical form. Whoops :>
Rough overview of what a
RegionConstraintis:RegionConstraintcontains two things: anAND of LEAFsand anOR of AND of LEAFs. Another way of thinking about it would be to say its anANDconsisting of arbitrarily many LEAFs and a singleOR of AND of LEAFsfalsethen we wipe the top levelANDas it doesn't matter what they are, the constraint is always going to be falsethis simplifies a lot of things conceptually as we now no longer need to worry about what state our region constraints are in. and our algorithms also don't need to handle arbitrary nesting of ors/ands :) and its a lot easier to read the debug logs 😅
I also wound up needing to do this while trying to compile
std/corewith-Zassumptions-on-bindersas we would otherwise OOM from having both:And('a: 'b, 'a: 'b))OR(e.g.Or(And('a: 'b, 'b: 'c), And('a: 'b, 'b: 'd)))Some future work:
RegionConstraint::splatted_and_constraintsit's kind of weird to even need it and probably encourages bad-for-perf patternsRegionConstraint. Perf stuff :3In theory this PR should mostly not have functional changes. In practice it might affect some things due to changing the exact repr of things affecting query responses. There's probably also some behaviour differences here due to us falling on our face more or less in WIP parts of abby now that we have different region constraints. I don't think any of this should be meaningful though. This PR is intended to not fundamentally change the abby algorithm :3
This PR should be reviewed commit-by-commit. There are a bunch of commits restructuring existing logic to assume their input is in canonical form as it will be by the end of the PR.
Then there's the core change in
always canonical formwhich actually replacesRegionConstraintwith all the new types and updates all the locations using them.Finally there's
propagate ambiguity not evaluatewhich deals with the leftoverevaluate_solver_constraintwhich was mostly unnecessary now due to moving its main logic into construction ofRegionConstraintand friends. I didn't want to make actual bug fixes in this PR so I just left some FIXMEs about some of the issues thatpropagate_ambiguityhas instead of fixing them here.Fixes rust-lang/project-assumptions-on-binders#14