[ty] Remove some nondeterminism in constraint set tests#22064
Merged
Conversation
dcreager
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
dcreager
added
internal
dcreager
commented
Dec 18, 2025
| /// - `C → D`: This indicates that `C` on its own is enough to imply `D`. Any path that assumes `C` | ||
| /// holds but `D` does _not_ is impossible and can be pruned. | ||
| #[derive(Debug, Default, Eq, PartialEq, get_size2::GetSize, salsa::Update)] | ||
| struct SequentMap<'db> { |
Member
Author
There was a problem hiding this comment.
There are other FxHashMaps in here, but they're only used for existence checks; we never iterate over them. The ones changed below are the only ones we iterate over, and where insertion order is therefore important.
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Contributor
|
It's hard to tell if this fixes the issue, because if it doesn't, or if it does but it "solidifies" an ordering that doesn't match what we happen to get on the "pre" run, the results will look the same 😆 |
carljm
approved these changes
Dec 18, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Seems reasonable, if potentially expensive?
Guess there's no way to see if it worked other than land it?
Member
|
And (regarding #22064 (comment)) we we already non-deterministic before the |
Member
Author
I hope (and codspeed CI seems to agree) that it won't be too bad — the biggest change is sorting before starting sequent map construction, and my intuition is that we're doing enough work finding all of the sequents that the extra sorting step won't be a bottleneck. |
Member
Author
To be fair, where we are now is really quite non-deterministic 😅 |
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
3 participants
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.
We're seeing a lot of nondeterminism in the ecosystem tests at the moment, which started (or at least got worse) once
Callableinference landed.This PR attempts to remove this nondeterminism. We recently (#21983) added a
source_orderfield to BDD nodes, which tracks when their constraint was added to the BDD. Since we build up constraints based on the order that they appear in the underlying source, that gives us a stable ordering even though we use an arbitrary salsa-derived ordering for the BDD variables.The issue (at least for some of the flakiness) is that we add "derived" constraints when walking a BDD tree, and those derived constraints inherit or borrow the
source_orderof the "real" constraint that implied them. That means we can get multiple constraints in our specialization that all have the samesource_order. If we're not careful, those "tied" constraints can be ordered arbitrarily.The fix requires
threefourseveral steps:source_order. That ensures that we insert things into the sequent map in a stable order.SequentMapto useFxOrderSetinstead ofFxHashSet, so that we retain that stable insertion order.source_order. However, we were not considering that we might get derived constraints, and therefore constraints with "ties". Because of that, we need to make sure to use a stable sort, that retains the insertion order for those ties.All together, this...should...fix the nondeterminism. (Unfortunately, I haven't been able to effectively test this, since I haven't been able to coerce local tests to flop into the other order that we sometimes see in CI.)