Sema: Fix accidental mutation of DenseMap in ConstraintSystem::selectDisjunction() [6.3] #86379
Conversation
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
…Disjunction() The firstScore and secondScore bindings were references, so the assignments to those were mutating the DenseMap unintentionally. Refactor this code a bit, we don't want to just change the bindings to rvalues because then we would copy the FavoredChoices vector, so instead bind a const reference to the DisjunctionInfo and dereference its fields instead.
Contributor
Author
|
@swift-ci Please test |
tbkka
approved these changes
Jan 8, 2026
DougGregor
approved these changes
Jan 8, 2026
jamieQ
reviewed
Jan 8, 2026
| if (firstActive == 1 || secondActive == 1) | ||
| return secondActive != 1; | ||
|
|
||
| if (auto preference = isPreferable(*this, first, second)) |
Contributor
There was a problem hiding this comment.
@slavapestov out of curiosity: i thought there was some frontend/driver setting to try and catch nondeterminism that would like compile twice and compare the outputs – would that have caught this? (assuming i'm not hallucinating...)
Contributor
Author
There was a problem hiding this comment.
No, the non-determinism here can only be caught by invoking the predicate more than once on the same pair of inputs and observing that it produces a different result.
Contributor
Author
There was a problem hiding this comment.
So it’s more accurate to say the predicate had an unexpected side effect which changed subsequent calls, instead of being non-deterministic. The overall compiler invocation is still deterministic in its output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
6.3 cherry-pick of #86352.
Description: Fix accidental modification of a DenseMap inside the selectDisjunction() algorithm. The accesses of the values from the DenseMap were unintentionally declared as references, when they should be values, because the assignments were not meant to be written back to the DenseMap.
Origination: The new logic was part of the solver-perf optimizations and did not ship in a release yet.
Tested: I do not have a test case to exercise the formerly-incorrect behavior. I only found the problem while debugging some changes to the algorithm where I intentionally performed the selection pass twice, and I noticed the result was not deterministic.
Risk: Disjunction selection order can affect both performance, and -- unfortunately -- the actual solution produced by the solver. There is some remote risk this incorrect behavior was load-bearing in some cases, but this is why I want to land it in 6.3, since we don't want users to rely on it going forward.
Reviewed by: @xedin