Fix: PMap buckets group entries by hash(key) % len(buckets)... - #330
Open
M001N wants to merge 1 commit into
Open
Fix: PMap buckets group entries by hash(key) % len(buckets)...#330M001N wants to merge 1 commit into
M001N wants to merge 1 commit into
Conversation
PMap._getitem, _contains, the evolver's set/getitem, and the bucket removal logic compared keys purely via k == key. Buckets group entries by hash(key) % len(buckets), which is coarser than exact hash equality, so a custom __eq__ that claims equality between unrelated objects (e.g. always returning True) could match the wrong bucket entry and return/ overwrite/remove the wrong key's value. Mirror CPython dict's own lookup guarantee: a stored key only matches if it is literally the same object, or if it has the same hash AND compares equal via __eq__. This gates calls to a possibly-buggy __eq__ behind an identity check and an exact hash comparison, exactly as the builtin dict does internally, fixing tobgu#212. Adds regression tests in tests/map_test.py covering __getitem__, __contains__, and .set() on an existing key using a pathological always-equal key type.
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
1 participant
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.
Summary
In pyrsistent/_pmap.py, updated all key-comparison sites (PMap._getitem, PMap._contains, the evolver's set() including its bucket-rebuild comprehension, and the evolver's remove()) to gate every
k == keycall behindk is key or (hash(k) == hash(key) and k == key), mirroring CPython dict's own lookup guarantee: a stored key only matches if it's literally the same object, or if it has the exact same hash AND compares equal. This prevents a pathological eq from ever being consulted across genuine hash collisions caused only by the coarser bucket-modulo grouping.Problem
tobgu/pyrsistent issue reference: #212
Root Cause
PMap buckets group entries by hash(key) % len(buckets) (pyrsistent/_pmap.py _get_bucket), which is coarser than exact hash equality. _getitem, _contains, the evolver's getitem/set, and the bucket-removal logic all compared candidate keys purely via
k == key, so any two objects landing in the same modulo bucket would have eq invoked on them regardless of whether their real hashes matched. A pathological eq (e.g. always True) then causes the wrong entry to be matched, returned, overwritten, or removed. A purek is key or k == keyidentity-shortcut fix (as initially attempted) is INSUFFICIENT: it still calls the buggy eq against every other entry in the bucket, so if a non-matching entry happens to be visited before the true identity match, it wins first. CPython's real dict avoids this because it only ever calls eq on a candidate slot when that slot's exact stored hash equals the lookup key's hash (identity is also checked first as a fast path) -- it never merely relies on a coarser modulo-bucket grouping.Testing
PASS: tests/map_test.py 53/53 passed; full suite 640 passed, 1 skipped with deterministic ordering (-p no:randomly). Note: with pytest-randomly's default random ordering, the full suite shows unrelated RecursionError failures in record/checked_vector/class tests; verified these are pre-existing on the unmodified base commit too (order-dependent, unrelated to this fix) and disappear entirely with deterministic ordering.
Related Issue
#212