Repository navigation
gh-160929: Preserve ChainMap lookup values in reverse unions - #160932
Open
JoaoMarcos44 wants to merge 3 commits into
Open
JoaoMarcos44 wants to merge 3 commits into
JoaoMarcos44 wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Why
dict | ChainMapcan replace a value exposed by the right-hand mapping with a deeper map's stored value. For example,ChainMap(Counter(), {'x': 5})['x']is0, but its reverse union returned5.What changed
ChainMapview so its lookup semantics determine the values.Counteranddefaultdict, including collisions, key order, and the original maps' state.Scope
The change is limited to reverse union and its regression tests.
Tradeoffs
The union now performs per-key chain lookups, as
dict(chain)does. Long disjoint chains can require quadratic lookup work. Default factories can run and insert missing keys, as they do when reading the sameChainMapdirectly.Verification
5to the expected0.test_collections test_userdict test_userlist test_userstring test_numeric_towerwith ten hash seeds. Each pass reports 286 tests, 284 passes, two skips, and zero failures or errors. Both new tests are present and pass in every run. Native validation and results.make patchcheck, andgit diff --checkpass. Lint result. Contribution checklist result.Illustrated explanation
The first
Counter()layer returns0for the missing key. The old reverse union copied the deeper stored5; the fix preserves the value returned by theChainMapview.Accessible text explanation and validation notes. The illustration is supplemental; it is hosted outside this source patch.
Fixes #160929.
AI tools assisted the investigation, patch, tests, and supplemental illustration. I have personally reviewed the AI-assisted material in this PR.