node:assert: preserve partial semantics for TypedArrays, Sets and sparse arrays in partialDeepStrictEqual - #34585
node:assert: preserve partial semantics for TypedArrays, Sets and sparse arrays in partialDeepStrictEqual#34585robobun wants to merge 6 commits into
Conversation
…rse arrays in partialDeepStrictEqual Three branches in compareBranch() were falling through to full strict equality instead of Node's partial-subset semantics: - ArrayBufferView/ArrayBuffer operands delegated to Bun.deepEquals, so a shorter expected view never matched. Node compares the expected bytes as an in-order subsequence of the actual bytes after a Symbol.toStringTag gate (isPartialArrayBufferView / isPartialUint8Array). - Set members were compared with isDeepStrictEqual, so an expected member with fewer keys never matched. Node recurses with the partial algorithm per member and claims distinct actual members greedily. - Holes in the expected array were read as undefined via expected[i], so a sparse expected forced an undefined match. Node skips holes on both sides (partialArrayEquiv / partialSparseArrayEquiv). Each of these surfaces as a false AssertionError on Bun for inputs that pass on Node v26, which blocks test suites porting from Node that use the partial matcher on binary data, Sets of objects, or sparse expected arrays. All expectations in the added tests were verified against Node v26.3.0.
|
Updated 3:48 AM PT - Jul 18th, 2026
❌ @robobun, your commit 07ff6ff has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34585That installs a local version of the PR into your bun-34585 --bun |
|
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 (2)
Comment |
…dial The captured TypedArrayPrototypeGetByteLength was applied only to freshly-constructed Uint8Array values inside isPartialUint8Array, after Uint8ArrayFromBuffer already read .buffer/.byteOffset/.byteLength from the user-supplied view via ordinary prototype lookups, so the capture added nothing. Drop it and read .byteLength directly. For the ArrayBuffer vs SharedArrayBuffer gate, compare isSharedArrayBuffer(actual) against isSharedArrayBuffer(expected), which is a brand check, instead of reading Symbol.toStringTag off the value.
There was a problem hiding this comment.
Beyond the inline nit, I traced the startA/startB handoff into compareBranchSparseArray — the dense-prefix invariant (every actual[0..startA-1] and expected[0..startB-1] is verified non-hole before switching) means array positions and ObjectKeys positions coincide at the switchover, so keysA[startA] / keysB[startB] index correctly. Also checked the moved bounds check in the dense while-loop against the old lastCandidate form — equivalent, and the entry invariant actualPos ≤ actual.length - expected.length + i holds at each iteration start.
Extended reasoning...
The sparse-array handoff was the non-obvious part of this diff: compareBranchArray passes dense array positions (actualPos, i) as indices into ObjectKeys(actual) / ObjectKeys(expected). That only works if the arrays are dense up to those positions so that position-in-array == position-in-keys-list. Traced both entry paths (hole in expected[i], hole in actual[actualPos]) and confirmed every prior slot on both sides was explicitly checked via the isSparse assignment before being consumed or skipped, so the invariant holds. The finder-raised concern about the dense-path bounds check over-constraining when later expected indices are holes was also examined — the check uses expected.length (not remaining own-key count), but any hole in expected triggers the sparse fallback before the dense bound is applied to it, so it can't over-reject.
|
CI build #75248 finished with 281/286 jobs green. The new
This change touches only |
assert.partialDeepStrictEquallost its partial semantics in three branches ofcompareBranch(), producing a falseAssertionErroron Bun for inputs that pass on Node v26.3.0:Cause
Three separate spots in
compareBranch()(src/js/node/assert.ts):ArrayBufferView/isAnyArrayBufferbranch bailed toBun.deepEquals(actual, expected, true), which requires full byte equality. Node's partial mode matches the expected bytes as an in-order subsequence of the actual bytes (isPartialArrayBufferView/isPartialUint8Arrayinlib/internal/util/comparisons.js), after gating on the%TypedArray%@@toStringTagso cross-kind pairs (e.g.Uint8ArrayvsInt8Array) still reject.Setbranch compared each expected member against actual members withisDeepStrictEqual(full strict equality), so{a:1}never matched{a:1,b:2}. Node recurses with the partial algorithm per member (setEquiv/partialObjectSetEquiv). The branch also skipped the existing cycle guard.compareBranchArrayindexedexpected[i]with no own-property check, so a hole became a requiredundefined. Node skips holes on both sides, matching the own-index values ofexpectedas an in-order subsequence of the own-index values ofactual(partialArrayEquiv/partialSparseArrayEquiv), with theexpected.length > actual.lengthgate preserved.Fix
Replace each of the three branches with Node's algorithm: a byte-wise subsequence matcher for views and buffers (
isPartialUint8Array), a recursivecompareBranchcall for Set members inside the existingwithCycleGuard, and a dense-then-sparse subsequence scan for arrays that consultshasOwnPropertybefore reading an index.Buffer.isBufferis dropped fromtypesToCallDeepStrictEqualWithsince aBufferis anArrayBufferViewand is now handled by the view branch.Verification
test/js/node/assert/assert.test.cjscover all three families plus the rejecting edges (out-of-order bytes, longer expected, cross-kind views,ArrayBuffervsSharedArrayBuffer, expected member with extra keys, holes exceedingactual.length, explicitundefinedvs a hole inactual). Every assertion was checked against Node v26.3.0.bun bd test test/js/node/assert/passes (355 tests), as dotest-assert-typedarray-deepequal.jsandtest-assert.js.src/stashed and pass with the fix applied.#33068 replaces the whole comparison path with a port of Node's
internal/util/comparisons, which would also fix these; this PR is the targeted fix for the three reported branches in the current implementation.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 2 rejected · iteration 2
evidence per changed file