Skip to content

fix: compare by equality, not just hash, in ExtendedSet - #40

Merged
toumorokoshi merged 2 commits into
toumorokoshi:masterfrom
binggao1230:fix-extendedset-hash-collision
Jun 22, 2026
Merged

fix: compare by equality, not just hash, in ExtendedSet#40
toumorokoshi merged 2 commits into
toumorokoshi:masterfrom
binggao1230:fix-extendedset-hash-collision

Conversation

@binggao1230

Copy link
Copy Markdown
Contributor

Summary

The append_unique list strategy silently drops genuinely-distinct elements when they collide on a hash. ExtendedSet.__contains__ decides membership purely from the element's hash and never checks equality, and _hash_element builds a lossy "key:value" string for dicts:

from deepmerge import Merger
m = Merger([(list, ["append_unique"])], ["override"], ["override"])

m.merge([{"a": "1", "b": "2"}], [{"a": "1,b:2"}])
# -> [{'a': '1', 'b': '2'}]            # second dict dropped; expected both kept

Three independent collision classes, all silently wrong:

case hash collision result (before) expected
{"a": "1", "b": "2"} vs {"a": "1,b:2"} both → "a:1,b:2" one dropped both kept
{"k": 1} vs {"k": "1"} both → "k:1" one dropped both kept
-1 vs -2 hash(-1) == hash(-2) == -2 -2 in ExtendedSet([-1]) is True False

Cause

ExtendedSet stored a single {hash: element} mapping and __contains__ returned self._hash_element(obj) in self._values_by_hash — a hash-only comparison. Equality was never consulted, so any hash collision (including the lossy dict-string hash) aliased distinct values together.

Fix

Group elements into hash-keyed buckets (dict[int, list]) and verify true == against the bucket members on both insert and lookup. The hash is only a bucket key; equality is the arbiter. No public API change.

This preserves the existing intended behavior — equal dicts still dedupe regardless of key order (test_strategy_append_similar_dict) and distinct dicts are still both kept (test_strategy_append_unique_nested_dict).

Verification

  • Added test_strategy_append_unique_keeps_hash_colliding_dicts and test_strategy_append_unique_hashable_hash_collision, covering all three collision classes. They fail before the fix and pass after.
  • Full suite: 19 passed. black --check, mypy, and the existing append_unique/append_similar tests all pass.

This pull request was prepared with the assistance of AI, under my direction and review.

ExtendedSet.__contains__ decided membership purely from the element's
hash, and _hash_element builds a lossy "key:value" string for dicts. So
distinct elements that collide on the hash were treated as equal, and
the append_unique list strategy silently dropped genuinely-distinct
items, e.g. {"a": "1", "b": "2"} vs {"a": "1,b:2"}, {"k": 1} vs
{"k": "1"}, or -1 vs -2 (hash(-1) == hash(-2)).

Group elements into hash-keyed buckets and verify equality against the
bucket members on insert and lookup. The hash is only a bucket key; the
== comparison is the arbiter. Equal dicts still dedupe regardless of key
order, and the public API is unchanged.
@toumorokoshi

Copy link
Copy Markdown
Owner

Thanks for the change, and great catch! if you can fix the lint errors, LGTM.

@binggao1230

Copy link
Copy Markdown
Contributor Author

Thanks. I pushed a Black formatting fix for deepmerge/strategy/fallback.py.

Local checks run:

uvx --python 3.12 black --check deepmerge
.venv/bin/validate-pyproject pyproject.toml
.venv/bin/mypy deepmerge
.venv/bin/pytest deepmerge

The new GitHub Actions run is currently waiting for maintainer approval (action_required) because this is a fork PR.

@toumorokoshi
toumorokoshi merged commit fb716b6 into toumorokoshi:master Jun 22, 2026
5 checks passed
@toumorokoshi

Copy link
Copy Markdown
Owner

great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants