Fix Set/Map deep-equality with duplicate-by-value elements#28763
Fix Set/Map deep-equality with duplicate-by-value elements#28763
Conversation
When comparing Sets (and Maps) with duplicate-by-value elements via
deepEqual/deepStrictEqual, the linear-scan fallback did not track which
elements in the second collection had already been matched. This allowed
the same element to satisfy multiple matches, causing unequal collections
to be reported as equal.
For example, Set([{a:1},{a:1}]) vs Set([{a:1},{a:2}]) was incorrectly
considered equal because both {a:1} entries in set1 matched the single
{a:1} in set2.
Use a BitVector to track which indices in the second collection have
been consumed during fallback linear scans, enforcing one-to-one
correspondence.
|
Updated 4:36 PM PT - Apr 1st, 2026
❌ @robobun, your commit e528165 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 28763That installs a local version of the PR into your bun-28763 --bun |
WalkthroughImplements a two-phase deep-equality for Sets and Maps: a fast membership/value-check pass and a slow O(n²) fallback that enforces one-to-one matching via a bitset. Adds regression tests exercising deep equality for Sets and Maps with duplicate-by-value objects. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 1127-1133: The current scan sets matchedIndices.set(idx2) as soon
as Bun__deepEquals reports the keys equal, then checks the value, which can
incorrectly consume an entry when duplicate-by-value keys exist; change the
logic so that you only call matchedIndices.set(idx2) after both the key and
value comparisons succeed: perform Bun__deepEquals for the key (as you do now),
then — still inside the same scan iteration — perform Bun__deepEquals for the
corresponding value, call RETURN_IF_EXCEPTION after the value compare, and only
if both comparisons return true set matchedIndices.set(idx2) and set
foundMatchingKey = true; do not mark matchedIndices prior to the value check and
ensure idx2, key1, key2, and the value comparison are used in the same
iteration.
- Around line 1043-1047: matchedIndices currently only tracks rhs slots matched
by the linear fallback, allowing earlier fast-path matches to be reused by later
fallback matches; fix this by making the bitset represent all matched rhs slots
(both fast-path and fallback). Concretely, when a fast-path match is made in the
equality routine (the spots that currently set needsFallback or return early),
also set the corresponding bit in matchedIndices (or replace matchedIndices with
a unified matchedAll BitVector) so that if needsFallback becomes true later the
fallback loop will see those bits and skip them; likewise, when toggling
needsFallback ensure you populate matchedIndices with any already-consumed rhs
indices so the subsequent linear scan enforces one-to-one matching across the
entire rhs collection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e683c098-841a-4c8a-8f98-91b3ef75af92
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/regression/issue/28760.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review feedback: - Set: fast path (has()) is now a pure pre-check. If any element fails, restart with full O(n²) tracked scan. Prevents fast-path matches from leaking into fallback. - Map: same two-phase approach. Value is checked inside the inner loop before marking entry consumed, fixing greedy key-only matching. - Added tests for shared-reference + deep-equal duplicate (Set) and opposite-order duplicate-key Maps.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 1120-1129: The current fast-path for comparing Map values
incorrectly treats a present key with an undefined value as a missing key
because map2->get(...) returns undefined for both cases; update the logic in the
loop that uses map2->get(globalObject, key1) and value2.isUndefined() (affecting
allFastPath) to call map2->has(globalObject, key1) when value2.isUndefined() is
true and only flip allFastPath to false if map2->has(...) returns false; keep
using Bun__deepEquals<isStrict, enableAsymmetricMatchers>(...) for non-undefined
comparisons and preserve RETURN_IF_EXCEPTION(scope, {}) checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d12ac8e0-ae28-46fd-8233-625d9d571841
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/regression/issue/28760.test.ts
There was a problem hiding this comment.
LGTM — the two-phase design correctly addresses all previously raised concerns.
Extended reasoning...
Overview
This PR modifies specialObjectsDequal in src/bun.js/bindings/bindings.cpp to fix incorrect deep-equality results for Sets and Maps containing duplicate-by-value elements. A regression test file test/regression/issue/28760.test.ts is added with 9 test cases.
Security risks
No security-sensitive code paths are involved. This is a correctness fix for value comparison logic with no authentication, crypto, or permissions implications.
Level of scrutiny
This is a focused algorithmic fix in C++ binding code. The change is moderate complexity but well-contained. Two prior review rounds with multiple identified bugs (fast-path/fallback reuse, greedy key-only Map matching) and all were resolved in commit e528165 via the two-phase design.
Assessment of the final implementation
The current diff implements a clean two-phase approach: (1) fast path iterates the entire first collection checking hash/reference membership — if all elements match, return true immediately; (2) on any miss, restart from scratch with a full O(n²) scan using a WTF::BitVector to enforce one-to-one matching. For Maps, the slow path correctly gates matchedIndices.set(idx2) behind both keysEqual && valuesEqual before consuming an entry. The fast path is now a pure non-consuming pre-check, eliminating the half-consumed-state bug I previously identified. Test coverage includes the tricky mixed fast-path/fallback scenario (Set([shared, {a:1}]) vs Set([shared, {a:2}])). All prior inline comments are resolved.
1 participant
Fixes #28760
Problem
assert.deepEqual/deepStrictEqual(andBun.deepEquals,expect().toEqual) incorrectly report unequal Sets as equal when they contain duplicate-by-value objects.Cause
In
specialObjectsDequal(bindings.cpp), the linear-scan fallback for Set comparison does not track which elements in the second set have already been matched. This allows the same element to satisfy multiple matches — both{a:1}entries in set1 match the single{a:1}in set2, so{a:2}is never accounted for.The same bug existed in the Map comparison path for duplicate-by-value keys.
Fix
Use a
WTF::BitVectorto track which indices in the second collection have been consumed during fallback linear scans, enforcing one-to-one correspondence between matched elements.Verification
USE_SYSTEM_BUN=1 bun test test/regression/issue/28760.test.ts→ 5 fail (bug present)bun bd test test/regression/issue/28760.test.ts→ 9 pass (fix works)