Bun.deepEquals: drop the unused fourth skipPrototype argument - #37836
Bun.deepEquals: drop the unused fourth skipPrototype argument#37836robobun wants to merge 2 commits into
Conversation
functionBunDeepEquals read callFrame->argument(3) and, together with strict, dispatched to Bun__deepEquals<true, false, false, true>, which bindings.cpp instantiated explicitly for that one call. The argument was plumbing for util.isDeepStrictEqual's skipPrototype, which has had its own native entry point (Bun__deepEqualsNodeStrictSkipProto) since #34660. It was never typed, documented, tested or called in-tree. Bun.deepEquals(a, b, true, true) now behaves like Bun.deepEquals(a, b, true); the node skipPrototype instantiation used by util.isDeepStrictEqual and the Assert class is unchanged.
|
Warning Review limit reached
Next review available in: 25 minutes 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 (3)
Comment |
|
Status: ready for review. Reproduced on main with Self-review done, nothing outstanding. Checked in particular that the node CI: across the two builds so far the change has compiled on every platform and |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — removes dead-code fourth argument from Bun.deepEquals and its now-unused explicit template instantiation.
What was reviewed:
- Confirmed no remaining references to
Bun__deepEquals<true, false, false, true>after removal. - Verified
util.isDeepStrictEqual's skipPrototype path still routes throughBun__deepEqualsNodeStrictSkipProto(NodeUtilTypesModule.cpp:879), which is untouched. - Confirmed the only in-tree JS caller of
Bun.deepEquals(src/js/node/assert.ts:52) passes three arguments. - New test covers both argument orders, nested, boxed-String subclass, and non-boolean extras; strict/loose behavior for the first three args is unchanged.
Extended reasoning...
Overview
This PR removes an undocumented fourth skipPrototype argument from functionBunDeepEquals in src/jsc/bindings/BunObject.cpp (7 lines deleted) and the explicit template instantiation Bun__deepEquals<true, false, false, true> in src/jsc/bindings/bindings.cpp that existed solely for that branch (4 lines deleted). It adds a regression test in test/js/bun/bun-object/deep-equals.test.ts asserting that any argument after strict is ignored.
Security risks
None. Pure removal of an unreachable-in-practice code path from a comparison utility; no user input parsing, no I/O, no auth/crypto surface.
Level of scrutiny
Low-to-medium. The change is a small, mechanical dead-code deletion with a clear provenance story (introduced for internal plumbing in #34434, obsoleted by #34660's dedicated Bun__deepEqualsNodeStrictSkipProto entry point). I verified the key claims: grep confirms no remaining users of the removed instantiation; jsFunctionIsDeepStrictEqual in NodeUtilTypesModule.cpp still calls Bun__deepEqualsNodeStrictSkipProto for the node-compat skipPrototype behavior; and the sole in-tree Bun.deepEquals caller in src/js/node/assert.ts passes three arguments. The public type declaration and docs already describe three parameters, so this brings the runtime back in line with the contract.
Other factors
The new test is well-constructed: it exercises both operand orders, nested objects, and boxed-primitive subclasses (the paths where the constructor/prototype check matters), tries true and several non-boolean fourth arguments, and includes positive controls showing strict mode still works and loose mode is unaffected. The PR description states the five toBe(false) assertions with true as the extra arg fail on main, satisfying the fails-for-the-right-reason requirement. The description also flags a one-line merge conflict with #37788 in bindings.cpp, which is a helpful heads-up but doesn't affect this PR's correctness.
|
Updated 3:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 6aadf07 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 37836That installs a local version of the PR into your bun-37836 --bun |
There was a problem hiding this comment.
LGTM — removes an undocumented, never-shipped fourth argument from Bun.deepEquals and its now-orphaned template instantiation.
What was reviewed
- Confirmed no remaining consumers of
Bun__deepEquals<true, false, false, true>in the tree; the nodeskipPrototypepath goes throughBun__deepEqualsNodeStrictSkipProto(<true, false, true, true>), which is untouched. - Checked
src/js/for four-argumentdeepEqualscalls — none. - New test covers both argument orders, nested, boxed-String subclass, non-boolean extras, and confirms strict/loose semantics still hold.
Extended reasoning...
Overview
This PR strips an internal-only fourth skipPrototype argument from functionBunDeepEquals in src/jsc/bindings/BunObject.cpp and deletes the Bun__deepEquals<true, false, false, true> explicit instantiation in src/jsc/bindings/bindings.cpp that existed solely for that branch. A regression test is added to test/js/bun/bun-object/deep-equals.test.ts asserting that arguments after strict are ignored. Net native change: -11 lines.
Security risks
None. This narrows the surface of a comparison utility to match its documented and typed contract; no auth, crypto, filesystem, or network paths are involved.
Level of scrutiny
Low. The change is a mechanical dead-code deletion. The PR description traces the history precisely (#34434 added the argument as internal plumbing, #34660 gave the node path its own entry point, leaving this orphaned), and I verified the claims: Bun__deepEqualsNodeStrictSkipProto in bindings.cpp:3103 uses the <true, false, true, true> instantiation and is called from NodeUtilTypesModule.cpp, so util.isDeepStrictEqual's skipPrototype behavior is unaffected. Grepping src/js/ for four-argument deepEquals calls found none, and no other reference to the removed <true, false, false, true> instantiation exists.
Other factors
The argument never shipped in a stable release per the description, so there is no user-facing compat concern. The new test is well-constructed: it fails on main (five toBe(false) assertions receive true when the extra arg is true), covers symmetry, nesting, and non-boolean extras, and confirms the strict-mode contract otherwise holds. The test comment names both node entry points that still expose skipPrototype. The PR also flags a one-line merge conflict with #37788 in bindings.cpp, which is helpful for whoever merges second.
1 participant
Problem
Bun.deepEquals(new Foo(), { a: 1 }, true)returns false, as documented, butBun.deepEquals(new Foo(), { a: 1 }, true, true)returns true on current main.Bun.deepEqualsis typed and documented with three parameters, yet the runtime reads a fourth that turns strict mode into node's skipPrototype mode. It does not type check, is undocumented and untested, and nothing in tree passes it.util.isDeepStrictEqual; node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660 gave that its own native entry point, so it has had no consumer since. It has not shipped in a stable release.Fix
strictis now ignored, as in every stable release.util.isDeepStrictEqual(a, b, true)andnew Assert({ skipPrototype: true }); their entry point is untouched.strictand expects them to be ignored; five of its assertions fail on main and pass with this change. The node assert, node util and expect suites were also run on a debug build.Background
Bun.deepEquals(a, b, strict)is Bun's public deep equality. Strict mode also compares constructors, so a class instance never equals an object literal, and counts a property set toundefinedas present.util.isDeepStrictEqualand theAssertclass: strict comparison minus the constructor check. Bun implements it as a flag on the shared native comparer, reached by those node APIs through their own binding.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/bun-object/deep-equals.test.ts
Original description
Repro
Bun.deepEqualsis declared as(a, b, strict?)inpackages/bun-types/bun.d.tsand documented with three parameters, but the runtime reads a fourth one. The four argument call above does not type check, is mentioned nowhere in the docs, and has no test; the only in-tree caller ofBun.deepEqualsis the three argument loose call insrc/js/node/assert.ts.Cause
#34434 added
callFrame->argument(3)tofunctionBunDeepEqualsinsrc/jsc/bindings/BunObject.cppso that the JS implementation ofutil.isDeepStrictEqual(a, b, skipPrototype)could reach the skip-prototype instantiation ofBun__deepEqualswithout a separate binding. The review thread on that PR states the intent: the argument is internal plumbing and the public contract ofBun.deepEqualsstays at three arguments (#34434 (comment)).#34660 then gave
util.isDeepStrictEqualand theAssertclass their own native entry point (Bun__deepEqualsNodeStrictSkipProto, called fromjsFunctionIsDeepStrictEqualinNodeUtilTypesModule.cpp). That left the fourth argument ofBun.deepEquals, and theBun__deepEquals<true, false, false, true>explicit instantiation inbindings.cppthat existed only for it, with no consumer.Fix
Remove the
argument(3)branch fromfunctionBunDeepEqualsand the explicit instantiation it was the only user of.Bun.deepEquals(a, b, true, <anything>)is now the same asBun.deepEquals(a, b, true), which is what every release so far has done: the argument landed after 1.3.14 and has not shipped in a stable release, so no user code can depend on it.Removing rather than documenting it because that is the intent recorded when it was added, and because the mode it selected (strict, minus the constructor/class name check) is node's
skipPrototypeoption, which Bun exposes where node does:util.isDeepStrictEqual(a, b, true)andnew Assert({ skipPrototype: true }). Those still use theskipPrototypeIdentitytemplate parameter throughBun__deepEqualsNodeStrictSkipProto; that parameter and entry point are untouched.This came up in #37776, which adds another strict-mode check gated on the same template parameter and skipped testing the four argument form because of the type mismatch. Note for merging: #37788 adds a sibling explicit instantiation next to the one removed here, so whichever of the two lands second has a one-line conflict in
bindings.cpp.Tests
test/js/bun/bun-object/deep-equals.test.tsgets "ignores arguments after strict": a class instance vs a literal (both orders and nested), a boxedStringvs aStringsubclass instance (both orders), withtrueand a few non-boolean values as the extra argument, plus checks that strict mode otherwise still applies and loose mode is unaffected. On main the fivetoBe(false)assertions withtrueas the extra argument all receivetrue; with this change the file passes.Also ran
test/js/node/assert/deep-equal.test.ts(coversutil.isDeepStrictEqual'sskipPrototypeargument, 305 pass),test/js/node/util/util.test.js(208 pass) andtest/js/bun/test/expect.test.js(415 pass) on the debug build.