util: honor isDeepStrictEqual skipPrototype third argument - #33080
util: honor isDeepStrictEqual skipPrototype third argument#33080robobun wants to merge 2 commits into
Conversation
WalkthroughChangesDeep strict equality
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:45 PM PT - Jul 16th, 2026
❌ @robobun, your commit b687664 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33080That installs a local version of the PR into your bun-33080 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The change is complete and the tests pass locally: The only failing CI job is 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 threads a new template parameter through Bun__deepEquals/specialObjectsDequal — the core equality engine behind expect().toEqual/toStrictEqual and node:assert — and CI is showing musl build failures, so it's worth a human look before merging.
Extended reasoning...
Overview
This PR implements Node v26's third skipPrototype argument for util.isDeepStrictEqual. It adds a third bool skipPrototype = false template parameter to Bun__deepEquals and specialObjectsDequal in src/jsc/bindings/bindings.cpp, gates the two calculatedClassName checks on it, propagates the flag through every recursive call (objects, arrays, getters, Map/Set, Error cause/props, boxed String), exposes it as an internal 4th argument to Bun.deepEquals in BunObject.cpp, and forwards it from src/js/node/util.ts. A new test block in test/js/node/util/util.test.js covers the behavior matrix. The two .mdx changes are pure autofix whitespace/table reflow.
Security risks
None. This is a pure-comparison code path with no I/O, auth, or untrusted-data parsing; the new flag only relaxes a class-name equality check when explicitly opted in via util.isDeepStrictEqual's third argument.
Level of scrutiny
Moderate-to-high. Bun__deepEquals backs expect().toEqual/toStrictEqual, node:assert, and Bun.deepEquals, so any regression silently changes test-runner semantics across the ecosystem. The default-false template parameter means existing instantiations should be byte-identical, and the explicit instantiation for <true, false, true> is added so BunObject.cpp links — but the robobun comment reports build failures on x64-musl, x64-musl-baseline, and aarch64-musl for commit ea8ca13, with only a ci: retrigger commit since. Whether those are template-instantiation/linker issues introduced here or unrelated flakes needs confirmation.
Other factors
The implementation looks correct and the test coverage is thorough (recursion through nested objects, arrays, Map keys/values, Sets, getters, boxed strings, truthiness coercion, .length === 3). However, this is a non-trivial change to a hot, correctness-critical native template across three C++ files, and CI isn't green — both warrant a human reviewer rather than an auto-approval.
|
Thanks for the careful look. Two clarifications on the CI state, since the diff actually builds cleanly everywhere: The musl builds pass. In the current build (66789, commit The only genuine failure is infra. On the hot-path concern: |
|
Final CI status for build 66789 (
None of these exercise |
bc7222d to
3d99093
Compare
There was a problem hiding this comment.
No bugs found, but deferring to a human — this threads a new template parameter through Bun__deepEquals (the shared path behind expect().toEqual/toStrictEqual, node:assert, and Bun.deepEquals) and adds an undocumented 4th positional argument to the public Bun.deepEquals. Worth a maintainer look at the API-surface choice and Node-compat semantics.
Checked: all external Bun__deepEquals<...> call sites (bindings.cpp:498/1717/1753, BunObject.cpp:715) rely on the new skipPrototype = false default, so existing instantiations are unchanged.
Checked: toBoolean(globalObject) on argument(3) cannot throw, so no missing exception check.
Checked: the two calculatedClassName gates are the only prototype comparisons in the strict path; recursive calls all forward the flag.
Extended reasoning...
Overview
The PR implements Node v26's third skipPrototype argument to util.isDeepStrictEqual. It adds a compile-time skipPrototype template parameter (default false) to Bun__deepEquals and specialObjectsDequal in src/jsc/bindings/bindings.cpp, gates the two calculatedClassName comparisons on !skipPrototype, forwards the flag through every recursive call (arrays, object props, Map/Set, Error cause), reads a 4th argument in functionBunDeepEquals (BunObject.cpp) via toBoolean, and forwards it from src/js/node/util.ts. An explicit template instantiation for <true, false, true> is added so BunObject.cpp can link against it. Tests cover the behavior matrix from the PR description.
Security risks
None. This is a pure comparison-semantics flag; no I/O, auth, allocation sizing, or untrusted parsing is involved. toBoolean on a JSValue cannot throw and does not run user code.
Level of scrutiny
Moderate-to-high. Bun__deepEquals is the single implementation backing expect().toEqual/toStrictEqual, node:assert.deepStrictEqual, and public Bun.deepEquals — a subtle regression here would affect the entire test runner. The mitigation is strong (the flag is a template parameter defaulting to false, so existing instantiations compile to identical code and if constexpr dead-strips the new branch), and I verified every out-of-template caller (bindings.cpp lines 498, 1717, 1753 and BunObject.cpp line 715) relies on the default. Still, per the approval criteria this is not a "simple, mechanical, or obvious" change — it threads a flag through a ~1000-line hot function across ~15 call sites.
Other factors
- API surface: the fix adds a hidden 4th positional argument to public
Bun.deepEquals(a, b, strict, skipPrototype). A maintainer may prefer a separate internal binding (e.g. ``) rather than extending the public function's arity — that's a design call. - Node compat semantics: the PR claims verification against Node v26.3.0 for a specific matrix, but whether
skipPrototypeshould also skip class checks on Map/Set/Error subclasses themselves (vs. only their contents) is a compat detail worth a human eye. TheMyMap extends Maptest passes, but that path doesn't hitcalculatedClassName— Map/Set are compared by JSType. - Test coverage: comprehensive for the new flag; the author reports
expect.test.js(406 tests) still passes, confirming the default-false path is untouched. - No prior human review on the timeline — only bot comments.
|
On the two design points: API surface. Deliberate: the public Container-subclass semantics. Verified against Node v26.3.0 directly; with the third argument the two runtimes agree on every case:
So the skip path (what this PR adds) matches Node exactly. The no-skip column shows a pre-existing difference: Bun compares Map/Set/Error by JSType and contents rather than prototype identity, and has returned |
3d99093 to
c74c9e7
Compare
There was a problem hiding this comment.
No bugs found, but this threads a new template flag through the shared Bun__deepEquals hot path in bindings.cpp and adds an undocumented 4th positional argument to public Bun.deepEquals — the author explicitly flagged that API-surface choice for maintainer input, so a human should sign off.
Checked: the new skipPrototype template param defaults to false and is gated with if constexpr, so existing <true,false> / <false,false> instantiations (expect matchers, node:assert, loose deepEquals) are unchanged.
Checked: the flag is forwarded through every recursive site in Bun__deepEquals/specialObjectsDequal (arrays, object props, Map/Set, Error cause), and the two calculatedClassName gates are the only behavior change.
Checked: toBoolean on argument(3) is safe (undefined → false; non-throwing on primitives), and the explicit <true,false,true> instantiation is forced in the TU that defines the template.
Extended reasoning...
Overview
This PR implements Node v26's third skipPrototype argument to util.isDeepStrictEqual. It adds a third template parameter skipPrototype (default false) to Bun__deepEquals and specialObjectsDequal in src/jsc/bindings/bindings.cpp, threads it through every recursive call, and gates the two JSObject::calculatedClassName comparisons with if constexpr (!skipPrototype). functionBunDeepEquals in BunObject.cpp reads a new 4th positional argument and dispatches to the <true, false, true> instantiation when truthy. src/js/node/util.ts forwards the third argument. Tests cover the behavior matrix, recursion through containers, truthiness coercion, and .length === 3.
Security risks
None. This is pure comparison-semantics logic — no I/O, auth, crypto, or untrusted parsing. toBoolean on a JSValue does not invoke user getters/toString.
Level of scrutiny
Moderate-to-high. Bun__deepEquals is a core shared path backing expect().toEqual/toStrictEqual, node:assert, and Bun.deepEquals. The change is mechanically conservative — the flag is a compile-time template parameter defaulting to false, so existing instantiations are byte-identical and only the new <true, false, true> variant behaves differently. The bug-hunting system found nothing, and I verified the flag is propagated through every recursive call site and that the explicit instantiation is emitted in bindings.cpp where the definition lives (avoiding a link error).
Other factors
The author explicitly left the API-surface question open in the PR thread: whether the 4th positional argument on the public Bun.deepEquals is acceptable or whether a separate internal binding is preferred. That's a maintainer call per CLAUDE.md's API-design guidance, not something a bot should approve unilaterally. Combined with the fact that this touches the shared deep-equals hot path (even if via if constexpr), a human review is appropriate. Test coverage is thorough and CI is green modulo unrelated infra flakes.
c74c9e7 to
8fb31e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/bindings.cpp`:
- Around line 1424-1427: Update the class-name comparison in the !skipPrototype
branch to run only when strict mode is active as well as when prototype checks
are not skipped, preserving loose-mode equality for boxed strings such as String
and MyStr while retaining strict prototype validation.
- Around line 736-737: Update the second special-object comparison in the
surrounding equality routine to check for a pending exception immediately after
calling specialObjectsDequal(), before reading isSpecialEqual or moving its
value. Match the exception handling already used after the first
specialObjectsDequal() call, preserving the existing result handling when no
exception is pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69e8d426-9ef2-4841-a3b5-3c0db13b4007
📒 Files selected for processing (5)
src/js/node/util.tssrc/jsc/bindings/BunObject.cppsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers-handwritten.htest/js/node/util/util.test.js
There was a problem hiding this comment.
I didn't find any bugs, but deferring to a human for the API-surface decision: this adds an undocumented 4th positional argument to Bun.deepEquals as the internal transport for skipPrototype, and the author explicitly flagged "separate internal binding vs. extra positional argument" as a maintainer call.
What was reviewed:
- The new template param defaults to
falseand bothcalculatedClassNamechecks are gated withif constexpr, so the existing<true,false>/<false,false>/<false,true>instantiations (expect matchers,node:assert,deepMatch) compile to identical code. - All recursive call sites in
Bun__deepEquals/specialObjectsDequalforward the flag; the two-arg callers at bindings.cpp:498/1717/1753 pick up the header default. toBooleanonargument(3)cannot throw onjsUndefined, so the missingRETURN_IF_EXCEPTIONafter it is fine.
Extended reasoning...
Overview
Threads a compile-time skipPrototype flag through Bun__deepEquals / specialObjectsDequal in bindings.cpp, exposes it as a 4th positional argument to functionBunDeepEquals in BunObject.cpp, and forwards it from util.isDeepStrictEqual in src/js/node/util.ts. Adds a 79-line test block covering the Node v26 behavior matrix.
Security risks
None. No untrusted input parsing, no allocation changes, no lifetime changes. The flag only gates two calculatedClassName string comparisons behind if constexpr.
Level of scrutiny
Medium-high. Bun__deepEquals is the shared implementation behind expect().toEqual/toStrictEqual, node:assert.deepStrictEqual, and Bun.deepEquals — a hot, correctness-critical path. The template-default approach means existing instantiations are byte-identical (the only new code path is the explicitly-instantiated <true, false, true>), which is the right design and minimizes risk. The expect.test.js suite was verified unaffected.
Other factors
The author bot explicitly surfaced an open design question for maintainers: whether to keep the undocumented 4th positional argument on the public Bun.deepEquals entry point or route through a separate internal binding. Per repo guidance, adding/changing user-facing API surface (even undocumented) is a maintainer decision. There's also a documented pre-existing divergence from Node in the no-skip path for Map/Set/Error subclasses (tracked separately in #29037) that this PR intentionally leaves untouched. Given the open API-surface question and the shared hot path, a human sign-off is appropriate even though the implementation looks correct.
There was a problem hiding this comment.
I didn't find any bugs. The template-parameter approach means every existing Bun__deepEquals instantiation is byte-identical, and the outstanding CodeRabbit comment on bindings.cpp:1428 is a false positive — the pre-PR code already compared class names unconditionally in that branch (not gated on isStrict), so wrapping it in if constexpr (!skipPrototype) with the default false preserves loose-mode behavior exactly. Deferring for a human to confirm the 4th positional argument on Bun.deepEquals is the preferred wiring (the author offered to switch to a separate internal binding).
What was reviewed:
- Verified all recursive
Bun__deepEquals/specialObjectsDequalcall sites forward the new template arg; the header default keeps other callers (expect,node:assert) unchanged. toBooleanonargument(3)cannot throw, so no exception check is needed there.- Confirmed the explicit
<true, false, true>instantiation is required since BunObject.cpp only sees the header declaration.
Extended reasoning...
Overview
Threads a third skipPrototype compile-time template parameter through Bun__deepEquals and specialObjectsDequal in bindings.cpp, gates the two calculatedClassName comparisons on it, forwards it from functionBunDeepEquals in BunObject.cpp via a new 4th positional argument, and updates node:util's isDeepStrictEqual to pass its third argument through. Adds a comprehensive test block.
Security risks
None. No auth, crypto, filesystem, or untrusted-input parsing paths are touched.
Level of scrutiny
Moderate-to-high: Bun__deepEquals backs expect().toEqual/toStrictEqual, node:assert, and Bun.deepEquals, so a mistake here would be widely visible. However, the template-parameter-with-default approach means every existing 2-arg instantiation compiles identically — the only new code path is the <true, false, true> instantiation, reachable solely through the new 4th argument. The author verified expect.test.js (406 tests) is unaffected.
Other factors
- The unresolved CodeRabbit comment claiming a loose-mode regression at line 1428 is incorrect: I checked the parent commit and the
StringObjectTypecase'scalculatedClassNamecomparison was never gated onisStrict, so!skipPrototype(default true) yields identical codegen. - The resolved CodeRabbit comment (missing
RETURN_IF_EXCEPTIONafter the secondspecialObjectsDequalcall) was addressed in commit 9f31640. - The author explicitly flagged the 4th-positional-argument wiring as a design point they'd change if maintainers prefer a separate internal binding. That's a small API-surface call worth a human ack rather than a bot approval.
- Tests cover the recursion paths (nested objects, arrays, Map keys/values, Sets, boxed String), truthiness coercion, function
.length, and the negative cases where other strict rules still apply.
|
Current CI status (build 73912, head This supersedes the CI notes above, which described earlier builds. Ready for review. |
Node v26 added a third skipPrototype argument to util.isDeepStrictEqual. When truthy, the prototype/class comparison is skipped recursively while every other strict rule (type tags, property counts, strict value compares) still applies, so instances of different classes with identical own properties compare equal. Bun hardcoded the two-argument form and dropped the third argument. Thread a skipPrototype flag through Bun__deepEquals/specialObjectsDequal and bypass the two calculatedClassName comparisons when it is set, then forward the third argument from util.isDeepStrictEqual.
Matches the handling after the first call; specialObjectsDequal can return nullopt with an exception pending, and the comparison must not continue in that state.
9f31640 to
b687664
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this threads a new template parameter through Bun__deepEquals — the hot path behind expect().toEqual/toStrictEqual and node:assert — and adds an undocumented 4th positional arg to Bun.deepEquals, which the author flagged as a design choice for maintainer input. Worth a human look.
Checked that skipPrototype defaults to false at both the header declaration and the forward decl of specialObjectsDequal, so all existing 2-arg instantiations (deepEqualsWrapperImpl, deepMatch, asymmetric matchers) compile to identical code.
Verified every recursive call site in Bun__deepEquals and specialObjectsDequal forwards the flag; the two calculatedClassName gates are the only behavioral changes.
Confirmed argument(3).toBoolean() cannot throw (ECMA ToBoolean never runs user code), so no exception check is needed there.
Ruled out link errors: the four <isStrict, matchers, false> instantiations are already emitted implicitly in bindings.cpp via deepEqualsWrapperImpl, and <true, false, true> is explicitly instantiated.
Extended reasoning...
Overview
The PR implements Node v26's third skipPrototype argument to util.isDeepStrictEqual. It touches:
src/js/node/util.ts— 1-line change to forward the third argumentsrc/jsc/bindings/headers-handwritten.h— addsbool skipPrototype = falsetemplate parameter to theBun__deepEqualsdeclarationsrc/jsc/bindings/BunObject.cpp—functionBunDeepEqualsreads a 4th argument and dispatches between the<true,false,true>and<true,false,false>instantiationssrc/jsc/bindings/bindings.cpp— threadsskipPrototypethrough every recursiveBun__deepEqualsandspecialObjectsDequalcall, gates the twocalculatedClassNamecomparisons on!skipPrototype, adds aRETURN_IF_EXCEPTIONafter the secondspecialObjectsDequalcall, and adds an explicit instantiation for<true,false,true>test/js/node/util/util.test.js— 79 lines of new coverage
Security risks
None. The change is purely a comparison-semantics flag; no I/O, no allocation-size arithmetic, no untrusted-input parsing. toBoolean on the 4th argument does not invoke user-overridable methods.
Level of scrutiny
High. Bun__deepEquals is the shared implementation behind expect().toEqual, expect().toStrictEqual, node:assert.deepStrictEqual, and Bun.deepEquals. A regression here would break the test runner itself. The change is designed to be zero-impact on existing paths (template default = false, if constexpr gates), and I traced every 2-arg call site (bindings.cpp:498, :1718, :1754/deepEqualsWrapperImpl → the four JSC__JSValue__*deepEquals C wrappers, and BunObject.cpp:715) to confirm they still get skipPrototype=false. The boxed-String calculatedClassName check that CodeRabbit flagged was already unconditional before this PR, so wrapping it in if constexpr (!skipPrototype) is behavior-preserving for the default; that thread was correctly resolved.
Other factors
- The author explicitly noted the 4th-positional-arg approach on
Bun.deepEqualsas a design choice they'd change to a dedicated internal binding if maintainers prefer. That's a small API-surface call a human should make. - CI is 285/286 green; the lone failure (
test-worker-message-port-transfer-terminate.json x64-asan) is a pre-existing JSC assertion during worker termination, unrelated to this diff. - Test coverage is thorough (recursion through arrays/Maps/Sets/getters, boxed strings, truthiness coercion, negative cases) and
expect.test.js(406 tests) is reported unaffected. - Both CodeRabbit review threads are resolved (one fixed in commit 9f31640, one withdrawn as a misread).
Given the hot-path surface and the pending API-shape question, deferring to a human reviewer rather than approving.
…s, regexp highlighting, vm namespace, tty.WriteStream + deepEquals fixes (test-util 70% → 83%) (#34434) ## What Brings `node:util` closer to **node v26.3.0** and fixes several `Bun.deepEquals` bugs the newly ported tests exposed. Fixes #33074 Fixes #25736 Fixes #20129 ### `test-util-*` compatibility vs node v26.3.0 | | before | after | |---|---|---| | passing | 21 / 30 (70%) | **25 / 30 (83%)** | **+3 tests added** (verbatim from v26.3.0), **+1 updated**, plus new coverage in bun's own deep-equality matrix and util tests. Beyond `node:util`, this also fixes two bugs the porting turned up: `tty.WriteStream` throwing on a read-only fd, and vm module namespaces having a non-null prototype. > Overlaps four open PRs (#30985, #33080, #32872, #29037) — see [this comment](#34434 (comment)). Happy to drop the overlapping areas or close in favour of them. ## Fixes **`util.styleText` was still on the pre-v26 API.** Ported node v26.3.0's version: hex/RGB colors (`#RGB`/`#RRGGBB`), the `none` format, nested close-code handling, and `{ validateStream, stream }` — colour is suppressed when the target stream isn't a TTY. **New APIs** - `util.getCallSites(frameCount, { sourceMap })` — captures through a private `prepareStackTrace`, so a user-installed `Error.prepareStackTrace` is never invoked and `Error.stackTraceLimit` doesn't bound the result. `CallSite` gains `getScriptId`. Column numbers and the `column` alias verified against the node v26.3.0 binary. - `util.convertProcessSignalToExitCode(signal)` - `util.isDeepStrictEqual(a, b, skipPrototype)` — node v26's third argument, threaded through `Bun__deepEquals` as a template parameter. **`util.inspect`** - **Regexes are syntax-highlighted** by group depth (ported v26's `highlightRegExp`), replacing the flat `red` style — removing the `TODO(BridgeAR): Highlight regular expressions properly` bun inherited from node. - **`hasBuiltInToString`** rejected *any* `Symbol.toPrimitive`, but `Date.prototype[Symbol.toPrimitive]` is built-in, so `util.format('%s', date)` printed the `toString` form where node prints inspect's ISO form. Ported v26's version, which distinguishes own from inherited. - **`extraKeys`** — ported v26's mechanism replacing `unshift(keys, …)`. Those entries are getters, so node brackets them: `ArrayBuffer { [Uint8Contents]: <..>, [byteLength]: 4 }`, and `showHidden` on a typed array reports `[BYTES_PER_ELEMENT]`, `[length]`, `[byteLength]`, `[byteOffset]`, `[buffer]`. - **vm module namespaces get a null prototype.** A Module Namespace Exotic Object is specified to have `[[Prototype]] = null`; bun points the shared structure at an object carrying an `__esModule` accessor for CJS interop. Nulled in `NodeVMModule::namespaceObject` only, so real ESM namespaces keep the accessor. **`Bun.deepEquals` correctness (strict mode)** - boxed **Strings** ignored extra own properties - boxed **Symbols/BigInts** are plain `ObjectType` in JSC and never reached the type switch, so two *different* boxed Symbols compared equal - the fast path fetched the right-hand property without checking **enumerability** - typed arrays skipped own **non-index properties** (e.g. symbols) **Loose mode** is *not* untouched (an earlier revision of this description wrongly said so). Three loose-visible cases moved onto node's behaviour, and one regression was caught in review and gated back: | loose case | node | bun before | bun now | |---|---|---|---| | `Object(1n)` vs `Object(2n)` | false | true | **false** | | `Object(Symbol())` vs `{}` | false | true | **false** | | `Object(Symbol('a'))` vs `Object(Symbol('a'))` | false | true | **false** | | enumerable sym vs non-enumerable | true | true | **true** | ## Perf Both strict fall-throughs are gated on the structure actually carrying named properties. Elements and characters are synthesized by `getOwnPropertySlot` rather than stored in the structure, so an unguarded fall-through lands in the index-enumerating slow path. Measured before the guard: `new String('a'.repeat(100000))` took **2,004,127µs/op** and a 1KB `Uint8Array` **4264µs/op**, both linear. With the guard both are flat (~5µs). ## Verification - A/B against a clean build of the same base commit, using the runner's own dispatch (`bun test` for files containing `node:test`, `bun run` otherwise). - Every expectation checked against a **real node v26.3.0 binary**, not read off the source. Two review findings (a `TypeError` from a primed style cache, and an empty regexp palette) **reproduce identically on node** and are deliberately left as faithful ports. - Regression sweep: `test-assert-*` / `test-buffer-*` / `test-console-*`, `buffer.test.js` (617), the 271-case deep-equality matrix, bun's `expect` suites, and 97/97 node vm tests. `oxlint` clean. ## Notes for reviewers **`test-util-styletext{,-hex}.js` need the real environment.** They assert styleText's own colour decisions against a TTY. Deleting the runner's forced `FORCE_COLOR=0`/`NO_COLOR=1` wasn't enough — `spawnBun` sets `FORCE_COLOR=1` in its own base env, which reached the child. The three colour variables are now set to `undefined` (which `child_process` drops) for those two files. With no colour variables set, bun matches node on all eight TTY cases. ## Known gaps (deliberate) - **`test-util-inspect.js`** — `--expose-internals`; needs `internalBinding('js_stream').JSStream`, whose `_externalStream` must be a real napi external. It gets ~145 assertions in before that line. - **`test-util-format.js`** — one assertion: `[Foo: null prototype]`. V8 keeps the constructor on the object's map; JSC has no equivalent. I tried recovering it from JSC's structure transition chain — `previousID()` yields nothing usable after a prototype change (verified across `setPrototypeOf`, `__proto__ =`, and `Reflect.setPrototypeOf`), so this needs a WebKit-side change. Everything else in the file passes. - **`test-util-isDeepStrictEqual.js`** — needs `Buffer` ≠ `Uint8Array` in strict mode. That's correct (node rejects them, and the matrix pins bun's answer as a known bug), but enabling it turns `test-child-process-advanced-serialization.js` red: bun's advanced IPC is backed by `SerializedScriptValue`, which downgrades `Buffer` to `Uint8Array`, so that test currently passes only because two bugs cancel out: ``` bun: received.buffer → Uint8Array, isBuffer: false node: received.buffer → Buffer, isBuffer: true ``` Preserving `Buffer` through structured clone belongs in its own change. - **`test-util-getcallsites.js`** — asserts `getCallSites().length > 1` at module scope, which holds in node only because node wraps modules in JS functions (8 frames vs bun's 1; bun's loader is native). Not portable. `getCallSites` keeps real coverage in `test-util-getcallsites-preparestacktrace.js`. - `getCallSites`'s `sourceMap` option is validated but not applied. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
|
Closing: this landed on main through #34434, which added the Checked by applying only this PR's test changes ( |
1 participant
Fixes #33074
Problem
Node v26 added a third
skipPrototypeargument toutil.isDeepStrictEqual(a, b, skipPrototype). When truthy, the prototype/class comparison is skipped (recursively) while every other strict rule still applies, so two instances of different classes with identical own properties compare equal.Bun hardcoded the two-argument form and dropped the third argument:
Cause
src/js/node/util.tsdefinedisDeepStrictEqual = (a, b) => Bun.deepEquals(a, b, true), never forwarding the third argument. The strict-mode prototype check lives in the native comparison (Bun__deepEquals/specialObjectsDequalinbindings.cpp), which comparesJSObject::calculatedClassNameand had no way to be told to skip it.Fix
skipPrototypeflag throughBun__deepEqualsandspecialObjectsDequaland bypass the twocalculatedClassNamecomparisons (plain objects and boxedStringobjects) when it is set. The flag defaults tofalse, so all existing callers (expect().toEqual/toStrictEqual,node:assert, loosedeepEquals) are byte-for-byte unchanged.util.isDeepStrictEqualnow forwards the third argument;isDeepStrictEqual.lengthis3, matching Node. The argument is coerced by truthiness, matching Node.The third argument stays an internal extension of the native
Bun.deepEqualsentry point; its public(a, b, strict?)contract is unchanged.Behavior verified against Node v26.3.0
new Foo(1)vsnew Bar(1), skiptrueskip=falsefalse{ x: new Foo(1) }vs{ x: new Bar(1) }, skip (recurses)true{ a: 1 }vsnew Foo(1), skiptrueObject.create(null)vs{}, skiptrue[]vs{}, skip (type tag still checked)falsenew Date(0)vs{}, skip (built-in type still distinct)falsenew Foo(1)vsnew Bar(2), skip (values differ)falsenew Foo(1)vsBar { a: 1, b: 2 }, skip (prop count differs)falsenew MyStr("a")vsnew String("a"), skip (boxed String)truetrueTest
test/js/node/util/util.test.jsadds adescribe("isDeepStrictEqual skipPrototype ...")block covering the matrix above, the recursion paths, and the truthiness coercion.Without the fix, the released binary returns
falsefor the skip cases:With the fix the full file passes (201 tests), and the
expect()matcher suite is unaffected (test/js/bun/test/expect.test.js, 406 pass).[review] gate passed · iteration 8 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 8
evidence per changed file
root cause · written by the author bot
The bug was that
util.isDeepStrictEqualin Bun ignored Node v26's thirdskipPrototypeargument, so objects with identical own properties but different prototypes, such as instances of two structurally identical classes, compared as unequal even when the caller asked for prototypes to be skipped. The native deep equality implementation unconditionally compared the calculated class names of the two values, with no way to opt out. The fix threads askipPrototypetemplate parameter from the JavaScript layer throughBun__deepEqualsand all of its recursive call sites, gating the class na…