Conversation
assert.deepStrictEqual and util.isDeepStrictEqual compare a Proxy of an array with the own-property walk. That walk never reads `length`, so a Proxy whose get trap reports another length equals the array. node compares `val1.length !== val2.length` with [[Get]] and reports them unequal. In the node entry point, when both values are arrays and one of them is a Proxy, read `length` from each side with [[Get]] and compare the values with ===. Arrays that are not proxies keep the direct length comparison.
|
Warning Review limit reached
On-demand reviews are free for the next 18 days. After that, they cost $0.25 per reviewed file. Or wait 10 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: the fix and the test are pushed. The Buildkite build runs now. Reproduction, with a Proxy of
The automated review found no issues. No review threads are open. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix with correct exception hygiene.
What was reviewed:
- The new
checkPrototypes-gated block inBun__deepEquals:RETURN_IF_EXCEPTIONfollows everyget()andstrictEqual()call, matching the surrounding pattern;v1Arrayimpliesv2Arrayhere since the mismatch case returned at line 971. - Scope: gated by
if constexpr (checkPrototypes)andisProxy(), soBun.deepEquals/expect()and the non-proxy array fast path are untouched. - Tests: five new cases added to the existing
deep-equal.test.tstable, covering both argument orders,===vs==on length (the"3"case), and the no-op-Proxy-stays-equal case.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap in assert.deepStrictEqual / util.isDeepStrictEqual: when comparing a Proxy-wrapped array whose get trap overrides length, Bun was reporting it equal to a plain array with the target's real length. Node reads length via [[Get]] and compares with !==, so a trap-reported length participates in the comparison. The fix adds a ~12-line block in Bun__deepEquals (src/jsc/bindings/bindings.cpp) that, only in the checkPrototypes template instantiation and only when at least one side is a Proxy, reads length from both objects via [[Get]] and compares with strictEqual. Five new parameterized cases are added to the existing test/js/node/assert/deep-equal.test.ts cases table.
Security risks
None. Bun__deepEquals already invokes user-controllable traps and getters throughout (e.g., objectPrototypeToString, property enumeration on proxies). The new get() calls are guarded with RETURN_IF_EXCEPTION(scope, false) immediately after each, so a throwing trap propagates cleanly rather than being swallowed or leaving a pending exception. No untrusted-size arithmetic, no allocation, no state held across the trap calls.
Level of scrutiny
Low-to-moderate. The change is small, mechanical, and mirrors both Node's documented behavior (val1.length !== val2.length) and the surrounding code's exception-checking pattern line-for-line. It is gated behind if constexpr (checkPrototypes) — the template flag used only by the node:assert/util entry points — and further gated on isProxy(), so the existing non-proxy array fast path (lines 989+) and the Bun.deepEquals/expect() instantiations are unaffected. The v1Array != v2Array early return at line 971 guarantees both sides are arrays when the new block runs. No CODEOWNERS entries cover these paths.
Other factors
Tests were added to the existing module test file (not a new file), follow the file's established cases table pattern, and cover the variant matrix REVIEW.md asks for: both argument orders, the ===-not-== distinction (string "3" vs numeric 3), the identity case (two proxies with the same reported length stay equal), and the regression guard that a no-op Proxy still compares equal. The looseBug annotations honestly document that assert.deepEqual (loose) is unchanged and still diverges from Node — this PR does not claim to fix that. The bug-hunt exited on dry_streak with no findings and no ruled-out candidates worth noting.
This branch has not been deployed
2 participants
Problem
assert.deepStrictEqualandutil.isDeepStrictEqualreport a Proxy of[1, 2, 3]whose get trap returns7forlengthas equal to[1, 2, 3]. Node and Bun 1.4.0 report them unequal.Bun__deepEquals(src/jsc/bindings/bindings.cpp:974). The own-property walk that compares it never readslength, becauselengthis not enumerable.new Proxy([1, 2, 3], {}), which node accepts. node:assert: compare constructors like node in deepStrictEqual, not [[Prototype]] identity #40131 removed that check for the node entry point.Fix
lengthfrom each side with [[Get]] and compare with===. This is node'sval1.length !== val2.lengthinobjectComparisonStart.Bun.deepEqualsandexpect()do not change.test/js/node/assert/deep-equal.test.ts(five new cases, stock bun fails three). Also all oftest/js/node/assert/, the vendored node assert tests, and theBun.deepEqualsandexpect()suites.Background
Bun__deepEqualsis one native function with template flags. ThecheckPrototypesinstantiation serves onlyassert.deepStrictEqualandutil.isDeepStrictEqual.gettrap, so the trap decides thelengththat a script sees.assert.deepEqual(loose) usesBun.deepEquals. It already differed from node for these values in 1.4.0. The new cases mark this withlooseBug.Notes
Strict mode (
util.isDeepStrictEqual), cross-checked against node v26.3.0:new Proxy([1,2,3], {})vs[1,2,3][1,2,3]vs[1,2,3][1,2,3]vs length 7 Proxy of[1,2,3]'3'Proxy of[1,2,3]vs[1,2,3][1,2,3][1,2,3]{ x: <length 7 Proxy> }vs{ x: [1,2,3] }[]vs[][]vs[ , ][1,2,3,4,5]vs[1,2,3]The last row is a difference this PR does not change. Node compares only the indexes below the reported length. Bun compares every own enumerable key of the target. Bun 1.4.0 behaves the same way.
The comparison uses
===and not ToLength, because node uses!==. So a trap that returns'3'is unequal to an array of length 3.A
lengthtrap that throws now propagates its error, as in node. Before this change the trap did not run.#32948 (open) adds a length check for
Bun.deepEqualsandexpect(). It uses ToLength, and it does not touch the node entry point.Suites run with the debug build:
test/js/node/assert/(all files),test/js/bun/bun-object/deep-equals.test.ts,test/js/bun/test/expect.test.js(1000 pass, 0 fail), and the vendoredtest-assert.js,test-assert-checktag.js,test-assert-deep-with-error.js,test-assert-typedarray-deepequal.js,test-assert-class.js,test-util-isDeepStrictEqual.js.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file