Implement Node's deep equality algorithm for node:assert - #33068
Conversation
assert.deepEqual, assert.deepStrictEqual and assert.partialDeepStrictEqual delegated to Bun.deepEquals, which implements the Jest expect().toEqual algorithm, not Node's. Port Node v26.3.0's lib/internal/util/comparisons.js and use it for node:assert and util.isDeepStrictEqual. - Add a native getOwnNonIndexProperties binding (the algorithm depends on Node's equivalent; emulating it in JS is O(indices) for typed arrays). - Bun.deepEquals now treats two invalid dates as equal (SameValue, not ==), matching both Jest and Node. It is otherwise unchanged. - Define Symbol.toStringTag on globalThis as "global" like Node; Node's tag-driven comparison and test-assert-checktag.js depend on it. Fixes #29030 Fixes #28760 Fixes #23877
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
Comment |
|
Updated 11:27 AM PT - Jun 29th, 2026
❌ @robobun, your commit 8423cd5 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33068That installs a local version of the PR into your bun-33068 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not duplicates, but they overlap on outcomes. #29037, #32872 and #28763 change The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a large change — a ~1000-line port of Node's comparison algorithm, new native C++ bindings, and a globally-visible Symbol.toStringTag on globalThis — so it warrants a human review before landing.
Extended reasoning...
Overview
This PR replaces node:assert's deep-equality implementation (previously delegated to Bun.deepEquals) with a faithful port of Node v26.3.0's lib/internal/util/comparisons.js (~1000 new lines in src/js/internal/util/comparisons.ts). It also: adds a native getOwnNonIndexProperties host function in UtilInspect.cpp; defines globalThis[Symbol.toStringTag] = "global" in ZigGlobalObject.cpp; fixes invalid-Date equality in bindings.cpp; rewires assert.ts / util.ts to the new module; and adds ~300 lines of tests.
Security risks
None apparent. The change is pure comparison/inspection logic with no auth, crypto, network, or filesystem surface. The new C++ host function is read-only property enumeration over a caller-supplied object and follows existing JSC patterns with proper exception scopes.
Level of scrutiny
High. This is a behavioral rewrite of widely-used node:assert APIs (deepEqual, deepStrictEqual, partialDeepStrictEqual) and util.isDeepStrictEqual, plus native bindings changes and a globally-observable modification to globalThis (Symbol.toStringTag). While the port is well-documented and the PR description shows extensive differential testing against Node (116k fuzzed comparisons, Node's own test-assert-deep.js going from 23/54 → 52/54), the sheer size and the cross-cutting nature (JS runtime + C++ + global object) put it well outside the "simple, mechanical" bar for auto-approval.
Other factors
The bug-hunting system found no issues, the new test file is thorough, and the verification story in the description is strong. Still, design decisions here (e.g., adding Symbol.toStringTag to the global, the perf trade-off of moving from native Bun.deepEquals to a JS-side algorithm, and the one acknowledged divergence around assert.deepEqual(dataView, arrayBuffer)) are the kind of calls a maintainer should sign off on.
…cted failures Bun's oxlint no-duplicate-conditional-property-access rule requires hoisting repeated property reads in comparisons.ts. Two vendored Node tests asserted Buffer vs Uint8Array distinctions that Bun.deepEquals could not see, so they passed vacuously. Both now fail for pre-existing reasons outside node:assert and are recorded in test/expectations.txt: - test-child-process-advanced-serialization.js: advanced IPC revives a Buffer as a plain Uint8Array (Node's v8 DefaultDeserializer returns a Buffer for every Uint8Array view). - test-stream-iter-readable-interop.js: Buffer.from(string) is not pool-allocated in Bun, so stream/iter's concatBytes zero-copy fast path returns the pushed Buffer where Node copies into a plain Uint8Array.
…bling Same single-gc()+single-setImmediate FinalizationRegistry race already documented for test-tls-connect-memleak.js, on the same two alpine x64 targets (build 66749). The test only uses assert.strictEqual; the trigger is the startup heap-layout shift from registering a new builtin module, not the comparison change.
There was a problem hiding this comment.
No bugs found, and the earlier CI-quarantine nit is addressed in 8b7436d — but this is a ~1000-line port of Node's comparison algorithm plus a new native host function and a globally-visible Symbol.toStringTag on globalThis, so it deserves a human pass.
Extended reasoning...
Overview
This PR replaces node:assert's deep-equality backend (previously Bun.deepEquals) with a faithful port of Node v26.3.0's lib/internal/util/comparisons.js (~1050 new lines in src/js/internal/util/comparisons.ts), rewires assert.deepEqual / deepStrictEqual / partialDeepStrictEqual and util.isDeepStrictEqual to it, adds a new native getOwnNonIndexProperties host function in UtilInspect.cpp, defines Symbol.toStringTag = "global" on globalThis in ZigGlobalObject.cpp, and fixes the NaN-date case in Bun__deepEquals (bindings.cpp). It also adds a 290-line test file and quarantines three vendored Node tests in expectations.txt whose false-passes were unmasked by the stricter comparison.
Security risks
None apparent. The comparison algorithm is pure inspection (no eval, I/O, or privilege boundaries). The new C++ host function reads own property names via JSC's existing getOwnNonIndexPropertyNames / method-table getOwnPropertyNames and follows the standard THROW_SCOPE / RETURN_IF_EXCEPTION pattern. The globalThis[Symbol.toStringTag] addition matches Node and is non-enumerable/read-only.
Level of scrutiny
High. node:assert underpins both user test suites and Bun's own vendored Node test corpus, so semantic changes here ripple widely (the PR itself documents two vendored tests that flipped from vacuous-pass to fail). The ported algorithm is intricate — cycle detection via a stack-overflow-then-memoize fallback, Set/Map bipartite matching with direction heuristics, partial-mode subsequence matching, loose-mode == coercion paths — and any subtle deviation from Node's behavior is exactly the class of bug this PR is meant to eliminate. The globalThis toStringTag change is small but globally observable. New native code in UtilInspect.cpp (Proxy fallback path, reifyAllStaticProperties, jsOwnedString lifetime) merits a human eye.
Other factors
- The bug-hunting system found nothing, and my earlier inline comment (missing
test-net-connect-memleakquarantine on linux-x64-musl) was addressed in 8b7436d and is now resolved. - Test coverage is thorough (53 new cases checked against Node v26.3.0, plus differential fuzzing described in the PR body), and the PR description carefully documents the known deviations and unmasked failures.
- The latest CI report on the thread (build #66756 for bab3430) still shows three musl
build-bunfailures; there is no CI result posted yet for the current HEAD 8b7436d. That, plus the overlap with three other open PRs (#29037, #28763, #32872), is another reason a maintainer should sign off on the direction here rather than auto-merge.
|
CI status: every lane that exercises this diff is green; the remaining red is three macOS jobs, none related to the change. Final state of the two runs of this exact tree (66763 at 8b7436d and 66812 at 8423cd5, which is an empty retrigger commit):
Everything CI attributed to this PR in the first run is addressed: the oxlint errors are fixed, and the two vendored Node tests whose Buffer-vs-Uint8Array assertions were passing vacuously, plus the musl FinalizationRegistry sibling, are recorded in Not pushing further retriggers to avoid spinning CI; this is ready for review. |
|
Status check against current main (f426a8e), since #29030 was closed as fixed by #34660 in the meantime. I copied this PR's
One correction to the description: the So this is not covered by main; leaving it open. It conflicts with main now that #34660 restructured |
1 participant
Fixes #29030
Fixes #28760
Fixes #23877
assert.deepEqual,assert.deepStrictEqualandassert.partialDeepStrictEqualdelegated toBun.deepEquals(), which implements the Jestexpect().toEqual()algorithm, not the one Node documents forassert. The two disagree in both directions, so assertions silently pass on Bun that fail on Node, and vice versa.Passing on Bun, failing on Node (every line below now throws):
Failing on Bun, passing on Node (every line below now passes):
Fix
lib/internal/util/comparisons.jstosrc/js/internal/util/comparisons.tsand use it forassert.deepEqual/notDeepEqual/deepStrictEqual/notDeepStrictEqual/partialDeepStrictEqualandutil.isDeepStrictEqual, the same way Node wires it. The hand-rolledpartialDeepStrictEqualin assert.ts is deleted: Node folded that into comparisons.js as its partial mode.getOwnNonIndexProperties(object, filter)host function (UtilInspect.cpp), the counterpart of the Node binding the algorithm relies on. JSC'sgetOwnNonIndexPropertyNamesmakes it proportional to the named properties. Emulating it in JS has to materialize every index key, which turnsassert.deepStrictEqualof two 1 MBUint8Arrays into ~700 ms instead of microseconds.Bun.deepEquals(soexpect().toEqual/toStrictEqual) keeps its Jest semantics and is untouched except for one bug: two invalid dates now compare equal. TheirNaNtimestamps were compared with==; Jest and Node both treat them as equal.Symbol.toStringTagas"global"onglobalThis(own, non-enumerable), like Node. Node's comparison is tag-driven and itstest-assert-checktag.js, already in our suite, relies onObject.prototype.toString.call(globalThis) === "[object global]"to tell a copy of the global's enumerable properties apart from the global itself.Verification
test/js/node/assert/assert-deep-equal.test.ts: 29 of 53 cases fail on current Bun; every expectation was checked against Node v26.3.0.internal/util/comparisons(via--expose-internals) on 116k comparisons over a curated corpus plus structural fuzzing: no mismatches in any of the three modes. The single known deviation is one where Node itself throws an internalTypeError(assert.deepEqual(dataView, arrayBufferOfSameByteLength)passes a non-view to Node's buffer binding); the port reports not-equal instead of throwing.test/parallel/test-assert-deep.jsgoes from 23/54 to 52/54 subtests andtest-assert-partial-deep-equal.jsfrom 114/140 to 139/140 on this branch. The three remaining subtests are blocked on unrelated pre-existing gaps (util.inspectproxy rendering undershowProxy,util.inspectof errors inside the generated diff message, andFilenot extendingBlob), which is why those two files are not vendored in this PR.test-assert.js,test-assert-checktag.js,test-assert-typedarray-deepequal.js,test-assert-deep-with-error.jsand everything intest/js/node/assert/pass.Two vendored Node tests were passing vacuously
The first CI run surfaced two vendored Node tests whose assertions rely on
deepStrictEqualdistinguishingBufferfromUint8Array. They passed before only becauseBun.deepEqualscannot tell them apart, and they now fail for pre-existing reasons outsidenode:assert. Both are recorded intest/expectations.txtwith the root cause:test-child-process-advanced-serialization.js: overserialization: "advanced"IPC, Node's v8DefaultDeserializerrevives everyUint8Arrayview as aBuffer, so{ buffer: Buffer.from("Hello!") }round-trips as aBufferin Node but as a plainUint8Arrayin Bun.v8.deserialize(v8.serialize(buf)) instanceof Bufferisfalsein Bun andtruein Node, so this needs a fix in the serializer, not in assert.test-stream-iter-readable-interop.js: Node'sBuffer.from(string)allocates out of the 8 KB pool, sostream/iter'sconcatBytescopies such a chunk into a plainUint8Array(it never covers its whole backing allocation). Bun's Buffers are not pool-allocated, so the same zero-copy fast path returns the pushedBufferitself. This cannot be papered over inconcatBytes: Node'stest-stream-iter-transform-sync.jspins the opposite result type (aBufferout ofbytes()) for exact-allocation chunks like zlib output, and the only discriminator in Node is the allocation pool.Related PRs
#29037, #32872 (null prototypes), #28763 (Map/Set) and #32948 (Proxy) each fix individual cases inside
Bun__deepEquals. Those still matter forexpect(), but after this changenode:assertno longer goes throughBun__deepEqualsat all, so the three assert issues above are fixed here regardless.