Repository navigation
Conversation
…defined values by the key lookup The Map arm of specialObjectsDequal took the first deep-equal key of the other Map and compared only the value under that key. It now looks for an entry whose key and value are both equal, as Jest's iterableEquality does. A key that holds undefined was read as an absent key, so each such entry walked the other Map. One lookup now tells the two apart. A Set or a Map that is an entry joins the cycle stack, so a cycle that runs only through entries ends. node's entry points record every entry. Without asymmetric matchers a primitive that the lookup misses ends the comparison, because a primitive equals only itself.
…ench a Map of undefined values
|
Reproduced with this script on Bun 1.4.2. import { expect } from "bun:test";
// #34830: prints "not equal". This branch prints "equal".
const regexKeys = () => new Map([[/a/, "x"], [/a/, "y"]]);
console.log(Bun.deepEquals(regexKeys(), regexKeys()) ? "equal" : "not equal");
// A key that holds undefined walks the other Map: 0.65 to 1.1 s. This branch: 0.6 ms.
const undefinedValues = () => new Map(Array.from({ length: 10000 }, (_, i) => ["k" + i, undefined]));
const a = undefinedValues(), b = undefinedValues();
const start = performance.now();
expect(a).toEqual(b);
console.log((performance.now() - start).toFixed(1), "ms");
// A cycle through entries only: RangeError: Maximum call stack size exceeded. This branch prints true.
const cycle = () => {
const outer = new Set(), inner = new Set();
outer.add(inner);
inner.add(outer);
return new Set([outer]);
};
console.log(Bun.deepEquals(cycle(), cycle()));This PR is the first part of #38510. It changes only the Set and Map arms. The node pairing stays in #38510. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughMap and Set deep comparisons update entry matching and cycle tracking. Map lookup distinguishes an absent key from a key whose value is ChangesCollection deep equality
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
#34830 requires expect to consider Maps equal when they have referentially distinct but deeply equal RegExp or Date keys and matching values. The Map arm in src/jsc/bindings/bindings.cpp now searc… |
| Out of Scope Changes check | |
The changes remain within deep comparison of Maps and Sets. Map and Set cycle-stack handling supports recursive entry comparisons. Primitive-candidate skips and the benchmark support the fallback look… |
| Title check | |
The title clearly and concisely describes the main change: Map entries now match using both their keys and values during deep equality checks. |
| Description check | |
The description explains the problem, implementation, verification, performance impact, limitations, and follow-up work. It does not use the template headings exactly, but it provides the required inf… |
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked the new Set-arm primitive short-circuit (the hash probe already settles primitive identity when matchers are off, so skipping the walk does not change verdicts), the mapValueOrEmpty empty-vs-undefined distinction (the absent-key path still falls through to the entry walk, so a wrong sentinel would cost time, not correctness), and that every Bun__deepEquals and lookup call in both rewritten arms is followed by RETURN_IF_EXCEPTION under the existing ThrowScope.
Extended reasoning...
The change rewrites the Set and Map arms of specialObjectsDequal in src/jsc/bindings/bindings.cpp, shared by Bun.deepEquals, expect matchers, and node assert/util, and adds tests in three files; it touches no auth, crypto, or input-parsing surface. One verified finding on node-strict Map semantics is posted inline and another verified finding was withheld, so this is not approvable; the note records the adjacent paths that were examined and ruled out.
assert.deepStrictEqual and util.isDeepStrictEqual pair entries one-to-one in node. The rule that lets two entries match the same entry would accept Maps that node rejects, so these entry points keep the answer of the first deep-equal key.
Fixes #34830
Problem
new Map([[/a/, "x"], [/a/, "y"]])is not equal to its own copy (Bun testtoEqualincorrect comparison of maps with referential keys. #34830). The Map arm ofspecialObjectsDequal(src/jsc/bindings/bindings.cpp) compared only the value of the first deep-equal key.undefinedas absent, so each such entry walked the other Map: 653 ms for two 10,000-entry Maps throughtoEqual.RangeError: Maximum call stack size exceeded.Fix
mapValueOrEmpty) tells an absent key from anundefinedvalue. The same Maps take 0.6 ms.test/js/bun/bun-object/deep-equals.test.ts,test/js/bun/test/expect.test.js,test/js/node/assert/deep-equal.test.ts(stock bun fails 62, 6 and 5 new tests), andtest/js/node/assert/. Self-reviewed: 9 concerns raised, 9 addressed.Background
Downsides
expect().toEqualtake 7,896 instructions (main 5,477): each Map entry joins the cycle stack. Release text grows by 2,048 B.expect(a).not.toEqual(b)on 100-entry Maps whose first value differs takes 9,713 instructions (main 1,272). A matcher key could still match.assert.deepStrictEqualandutil.isDeepStrictEqualBun testtoEqualincorrect comparison of maps with referential keys. #34830's Maps stay unequal until deepEquals: pair Set and Map entries one-to-one on node's strict entry points #38510.Notes
Rules after this PR
Bun.deepEquals,expect()andassert.deepEqual: a left entry is settled when the right Map holds its key with an equal value. If not, the arm walks the right Map for an entry whose key and value are both equal. This is the rule of Jest and Vitest. Before, the walk stopped at the first deep-equal key and compared only that value.assert.deepStrictEqualandutil.isDeepStrictEqual: the answers of main stay. A key that both sides hold with unequal values ends the comparison, and the walk stops at the first deep-equal key. Node pairs entries one-to-one, which a walk cannot do, and the rule above would accept Maps that node rejects. The one change on these entry points is theundefinedlookup.Measurements (release builds of the merge base 13a98b0 and of commit 0d5f669, the same tree except
bindings.cpp)size bun-profile): text 80,759,109 -> 80,761,157 B.nm -S -C:specialObjectsDequalx7 30,485 -> 32,607 B. No other symbol changes size.Map<string, undefined>, CPU time:expect().toEqual0.6 ms (main 652.6 ms),Bun.deepEquals0.5 ms (811.7 ms),util.isDeepStrictEqual1.0 ms (1,651.7 ms).bun-profile, because perf and valgrind are not available on the machine.{a: 1, b: {c: 2}}:Bun.deepEquals1,466 (1,466),util.isDeepStrictEqual2,280 (2,284).Bun.deepEquals1,409 (1,391),util.isDeepStrictEqual4,015 (3,977).Bun.deepEquals: reordered 3-object Set 5,337 (5,242), unequal 16-object Sets 12,806 (12,575), same-order 16-object Sets 105,282 (103,424). No mallocs on either build.Bun.deepEquals722 (716),util.isDeepStrictEqual1,100 (1,103),expect().not.toEqual9,713 (1,272). With matchers enabled a matcher key of the right Map could still match the left entry, so the arm walks the right Map once before it answers.expect().toEqual7,896 (5,477),util.isDeepStrictEqual869,525 (692,447). Each nested Map is an entry that joins the cycle stack, and each push searches the stack. The second number is large on both builds because main compares a Set or a Map once per argument order on node's entry points. deepEquals: compare a same-typed pair once, and pair Set and Map entries as node does #45457 removes that.expect().toEqual:Set<string>225.0 (225.0),Map<string, string>386.6 (387.5),Map<string, {v}>840.6 (841.5).util.isDeepStrictEqual:Set<string>447.8 (457.8),Map<string, string>734.8 (746.8),Map<string, {v}>2,404.8 (2,420.8).bench/deepEqual/map.js, new row "deepEqual Map with undefined values": 632 ms -> 476 µs per iteration. The two existing rows move inside the noise of three interleaved runs (557 to 865 µs on both builds).Differentials (seeded random pairs, both argument orders. The member pool has primitives, plain, dictionary-mode, undefined-property and non-enumerable-property objects, arrays, boxed values, nested Sets and Maps, members shared by identity, and asymmetric matchers for the
expect()runs.)Bun.deepEqualsloose and strict,toEqualandtoStrictEqualagainst main: 0 of 4,800,000 verdicts differ.toEqualagainst Vitest 5.0.3equals(a, b, [iterableEquality]): Maps 0 of 400,000 differ (main 51,810). Sets 0 of 200,000 (main 0).util.isDeepStrictEqualagainst main: 0 of 1,200,000 verdicts differ.Not in this PR
assert.deepStrictEqualandutil.isDeepStrictEqualis in deepEquals: pair Set and Map entries one-to-one on node's strict entry points #38510. The row for Bun testtoEqualincorrect comparison of maps with referential keys. #34830's Maps indeep-equal.test.tsis astrictBugrow until then.Bun__deepEqualsmakes over a Set or a Map on those entry points (a regression in 1.4.0 to 1.4.2) is removed by deepEquals: compare a same-typed pair once, and pair Set and Map entries as node does #45457. That PR and this one change the same arms, so the second to merge needs a rebase.assert.deepEqualkeepsBun.deepEqualssemantics (docs/runtime/nodejs-compat.mdx). Its answers that differ from node arelooseBugrows.Self-review
The review ran on #38510. It asked for this part to merge first as its own change. Its concerns that apply here: land the #34830 fix alone, name and pin the
undefined-value walk, add a bench row for it, cover the twoskipPrototypeentry points, use a fixture that Jest agrees with for the insertion-order test, pin each of the four places that pass the cycle-stack argument, keepassert.deepEqualout of the rows that pin Jest's answers, change no answer of node's strict entry points to one that node does not give, and state that the n*n walk stays.