Compare prototypes in deepStrictEqual - #29037
Conversation
WalkthroughAdded an early prototype-identity check to the strict branch of Bun__deepEquals so objects with different prototypes are treated as not deeply strict-equal; also added regression tests covering strict and non-strict deep-equality behavior across prototype scenarios. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29030.test.ts`:
- Around line 12-13: Replace the overly specific matcher usage that asserts a
particular error class with the preferred bare matcher: change expectation calls
that use expect(() =>
assert.deepStrictEqual(...)).toThrow(assert.AssertionError) to use plain
expect(() => assert.deepStrictEqual(...)).toThrow() so the tests only assert
that the call throws; update the two occurrences shown (calls to
assert.deepStrictEqual) and apply the same change to the other listed assertion
checks that currently pass assert.AssertionError to .toThrow().
🪄 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: 5fad3456-0bfa-438c-a19c-f038a1a1f1d6
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/regression/issue/29030.test.ts
There was a problem hiding this comment.
LGTM — the prototype check is now correctly placed before and the array branch, addressing all the gaps I flagged earlier.
Extended reasoning...
Overview
The PR adds a prototype () comparison to in strict mode, fixing incorrectly passing. The final commit (e79ea8e) moves the check to before and the array branch dispatch, so it applies to plain objects, array subclasses, Map/Set/Error/Date/RegExp subclasses, and every other object type. Two new test files cover the original bug and the subclass regressions.
Security risks
None. This is a correctness fix for an equality comparison utility with no auth, crypto, or permission implications.
Level of scrutiny
Low. The change is mechanically simple: add a guard block with two calls (both properly wrapped in ) and move two variable declarations. The logic precisely mirrors Node.js behavior and is validated by comprehensive regression tests.
Other factors
The only bug flagged by the hunting system — missing after the second call — is explicitly pre-existing and not introduced or worsened by this PR. The author responded promptly to my earlier review and the follow-up commit fully addresses the concerns raised.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29030.test.ts`:
- Around line 65-66: Update the test comment to remove the incorrect reference
to util.isDeepEqual and mention only assert.deepEqual: replace the existing
comment that references "util.isDeepEqual" with a concise line such as "The
Node.js loose `assert.deepEqual` does NOT check prototypes. Verify we didn't
regress the loose path." so the comment references only assert.deepEqual and, if
needed, the correct API name util.isDeepStrictEqual when discussing strict
equality.
🪄 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: dada59be-fbfb-4005-b317-ef0789031e97
📒 Files selected for processing (1)
test/regression/issue/29030.test.ts
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the prototype check is correctly positioned before and the array branch, covering all object types.
Extended reasoning...
Overview
This PR adds a prototype identity check () to in the strict comparison path, fixing incorrectly passing. Two files changed: (the fix) and (regression tests). The change is small and focused.
What Was Addressed Since My Previous Reviews
My first comment flagged that the prototype check was placed after and the array branch, leaving subclasses of Array/Map/Set/Error/Date/RegExp unguarded. This was fixed in e79ea8e: the check is now placed before the calls and before the array-branch dispatch, so it applies to every object type. My second comment noted a pre-existing missing after the second call; that is pre-existing and outside the scope of this fix.
Security Risks
None. The change is a comparison-semantics fix with no auth, crypto, or permissions involvement.
Level of Scrutiny
The fix is a targeted, well-understood change to a non-security-critical path. The regression test suite covers plain objects, null-prototype objects, class instances, and subclasses of Array/Map/Set/Error — all the edge cases that previously bypassed the check.
Other Factors
The remaining reported bug (specialObjectsDequal skipping own-property checks for Date/RegExp/Map/Set etc.) is explicitly pre-existing and not introduced or worsened by this PR. All reviewer feedback (prototype check placement, bare style) has been addressed.
There was a problem hiding this comment.
LGTM — all prior concerns addressed; the prototype check is correctly placed before all type dispatch.
Extended reasoning...
Overview
This PR fixes issue #29030 by adding a prototype-identity check to the strict branch of Bun__deepEquals in src/bun.js/bindings/bindings.cpp. It also adds a comprehensive regression test suite in test/regression/issue/29030.test.ts covering plain objects, class instances, array subclasses, and Map/Set/Error subclasses.
Correctness of the fix
The prototype check is placed at the top of the object-comparison block, before specialObjectsDequal and before the array-branch dispatch — exactly where Node.js performs its prototype check. This ensures the guard applies uniformly to all object types: plain objects, arrays, Maps, Sets, Dates, RegExps, Errors, and their subclasses. Exception handling uses the correct RETURN_IF_EXCEPTION(scope, false) guards.
Security risks
None. This is a behavioral correctness fix to a deep-equality utility. No auth, crypto, or permission code is touched.
Level of scrutiny
The change is small (19 lines of C++), well-scoped, and follows JSC conventions exactly. The pre-existing issues noted in prior inline comments (missing guard after the second specialObjectsDequal call, and content-only short-circuiting for Date/RegExp/etc.) predate this PR and are separate concerns for separate issues.
Other factors
All prior review comments — including moving the check above the array/special-type branches, switching to bare .toThrow(), and fixing the util.isDeepEqual comment — have been addressed in subsequent commits. All inline comments are resolved.
89d83d6 to
0b6cd64
Compare
There was a problem hiding this comment.
No issues found — the fix looks correct and all earlier feedback is addressed. Deferring to a human since this changes strict-equality semantics in Bun__deepEquals, which also backs expect().toStrictEqual() in bun:test (not just node:assert), so it's worth a maintainer sanity-checking the behavior change.
Extended reasoning...
Overview
Adds a prototype-identity check (getPrototype + ===) to the isStrict branch of Bun__deepEquals in src/jsc/bindings/bindings.cpp, placed before specialObjectsDequal and the array branch so it covers plain objects, arrays, Map/Set/Error/Date/RegExp/ArrayBuffer and their subclasses. Adds a 14-case regression test at test/regression/issue/29030.test.ts. Non-strict (deepEqual/toEqual) is untouched via if constexpr (isStrict).
Security risks
None. No auth, crypto, I/O, or untrusted input handling. getPrototype can invoke a Proxy getPrototypeOf trap, but both calls are guarded with RETURN_IF_EXCEPTION.
Level of scrutiny
Moderate-to-high. Bun__deepEquals is a core, performance-sensitive path that backs assert.deepStrictEqual, util.isDeepStrictEqual, and Bun's own expect().toStrictEqual(). The change is semantically correct (matches both Node's documented behavior and Jest's toStrictEqual), but it tightens strict-equality and could surface as new failures in downstream user test suites that previously passed. That's a user-visible behavior change in the test runner, which I'd rather a maintainer sign off on than auto-approve.
Other factors
- All three rounds of my earlier inline feedback were addressed: the prototype check was moved above
specialObjectsDequal/the array branch, and the two remaining items (missingRETURN_IF_EXCEPTIONafter the secondspecialObjectsDequalcall; own-property enumeration bypass for Date/RegExp/Map/Set/etc.) were correctly identified as pre-existing and deferred to a follow-up. - CodeRabbit nits (bare
.toThrow(), staleutil.isDeepEqualcomment) are resolved. - Asymmetric matchers are handled at lines 660–682, before the new check, so
expect.anything()etc. are unaffected. - Regression tests cover both directions, same-prototype positive cases, the loose path, and Array/Map/Set/Error subclasses.
247bbeb to
17c277c
Compare
022e569 to
2b43fa9
Compare
|
Diff is green locally but CI cannot run: every build on this branch for the last several hours (62986, 62989, 63012, 63014, 63016) fails at agent provisioning with `Image not found: linux-*-2023-amazonlinux-with-docker-v37` (robobun.ts:2594). No test lane ever starts — build 63016 shows 1 passed / 28 waiting_failed / 0 real test failures, the single annotation being the image-not-found error. This is a fleet-wide infra issue unrelated to this diff; a Local verification on the latest commit (
All review threads resolved. Ready for a maintainer to re-run once the CI images are available. |
38591ce to
d3e1daf
Compare
|
Rebased onto main ( The deep-equality fix itself (prototype check in |
There was a problem hiding this comment.
No new issues found after the rebase and all prior threads are resolved, but this changes core deep-equality semantics (new checkPrototypes template axis threaded through Bun__deepEquals/specialObjectsDequal, an undocumented 4th Bun.deepEquals arg, and a node-parallel test moved to expected-fail), so it's worth a maintainer pass before merge.
Extended reasoning...
Overview
This PR adds prototype-identity comparison to the strict path of Bun__deepEquals so assert.deepStrictEqual({}, Object.create(null)) throws like Node. To get there it:
- Adds a third
checkPrototypestemplate parameter toBun__deepEqualsandspecialObjectsDequalinsrc/jsc/bindings/bindings.cpp, threads it through every recursive call site (~15), and emits three explicit instantiations. - Adds an undocumented 4th argument to
Bun.deepEqualsinBunObject.cppsoassert.partialDeepStrictEqualcan opt out of the prototype gate while keeping strict content comparison. - Updates the template signature in
headers-handwritten.h. - Adds
isDeepStrictEqualWithoutPrototypetosrc/js/node/assert.tsand switches sixcompareBranchcall sites to it. - Adds
test-child-process-advanced-serialization.jstotest/expectations.txtas[ FAIL ]on all platforms (the new check correctly surfaces a pre-existing IPC Buffer→Uint8Array prototype-fidelity gap). - Adds a 17-case regression test.
The _http_outgoing.ts edits from earlier revisions were dropped during rebase since main's refactor already returns null-proto from getHeaders().
Security risks
None identified. No auth, crypto, filesystem, or network surface is touched; the prototype check only tightens an equality comparison and cannot be used to bypass anything.
Level of scrutiny
Medium-high. Bun__deepEquals is the engine behind assert.deepStrictEqual, util.isDeepStrictEqual, the public Bun.deepEquals API, and (via a different instantiation) expect().toStrictEqual(). The change is a user-visible behavior change to a public API, adds a new template axis to hot-path C++ comparison code, introduces an internal-but-publicly-reachable 4th argument to Bun.deepEquals, and quarantines a previously-passing node-parallel test. None of that is mechanical.
Other factors
- The PR has been through ~10 rounds of bot review; every thread is resolved and the current bug-hunter pass found nothing.
- The
!enableAsymmetricMatchersgate keepsexpect().toStrictEqual()unchanged, which was the right call given the cross-realm/Jest-compat fallout seen in earlier CI runs — but that design choice (Node-strict semantics forBun.deepEquals(_,_,true)vs. Jest-loose fortoStrictEqual) is worth a human ack. - Marking
test-child-process-advanced-serialization.jsas expected-fail everywhere is reasonable (the gap it exposes is pre-existing), but a maintainer should confirm that's the preferred trade-off vs. fixing IPC Buffer fidelity here. - CI was infra-blocked on the last several builds; the latest build (#63342) is in progress per robobun.
|
Thanks for the pass. Flagging the two intentional design choices for a maintainer, since both came out of earlier CI fallout:
The |
|
CI status: the only hard failure is This is not reachable from this PR's diff:
The deep-equality fix is green everywhere I can verify locally: 17/17 regression tests, the full |
Fixes #29030. Node's assert.deepStrictEqual differentiates {} from Object.create(null) because their prototypes differ (Object.prototype vs null). Bun's Bun__deepEquals only compared the calculated class name, which is "Object" for both, so the mismatch slipped through. Add a prototype === prototype check alongside the class name check in the strict branch.
The prototype check only ran inside the generic JSObject branch, which
was unreachable for arrays (their own branch returns first) and for
types handled by specialObjectsDequal (Map, Set, Date, RegExp, Error,
ArrayBuffer). Move it up so every object path sees it.
class Sub extends Array {} // Sub.prototype !== Array.prototype
assert.deepStrictEqual(new Sub(), []); // now throws
class MyMap extends Map {} // MyMap.prototype !== Map.prototype
assert.deepStrictEqual(new MyMap(), new Map()); // now throws
The fix in 074d466 added a prototype-identity check to Bun__deepEquals' isStrict path. Bun__deepEquals is instantiated with isStrict=true by two callers: - node:assert.deepStrictEqual / util.isDeepStrictEqual — where this matches Node's documented behavior (see issue #29030). - bun:test's expect().toStrictEqual() via jestStrictDeepEquals → Bun__deepEquals<true, true> — where the stricter check broke existing tests that compared Buffer against Uint8Array by content, cross-realm Sets from vm.runInContext, null-prototype vs plain objects returned by various Bun APIs, etc. Issue #29030 is scoped to node:assert; tighten only that path by gating the new check on !enableAsymmetricMatchers. bun:test's toStrictEqual keeps its existing calculatedClassName-based type guard, matching prior Bun behavior, and the earlier valkey/zstd test patches are no longer needed (they've been reverted in this commit).
The `<true, false>` template instantiation covers three user-facing APIs, not two: `node:assert.deepStrictEqual`, `node:util.isDeepStrictEqual`, and the public `Bun.deepEquals(a, b, true)` (see functionBunDeepEquals in BunObject.cpp). Spell out all three in the comment so it doesn't read as if `Bun.deepEquals` is unaffected.
…ent)
The prototype-identity check added for assert.deepStrictEqual also
fired for assert.partialDeepStrictEqual, because compareBranch
(src/js/node/assert.ts) delegates ArrayBuffer/ArrayBufferView, Buffer,
Error/RegExp/Date comparisons straight to Bun.deepEquals(_, _, true).
Node's partial mode does not enforce prototype identity for these, so
partialDeepStrictEqual(Buffer.from([1,2]), new Uint8Array([1,2])) and
partialDeepStrictEqual(new (class extends Error{})('x'), new Error('x'))
regressed from pass to throw.
Add a checkPrototypes template parameter to Bun__deepEquals (default
true, threaded through recursion and specialObjectsDequal). Bun.deepEquals
accepts an optional internal 4th argument to disable it. compareBranch's
leaf-type delegations now call it with the check off, restoring Node's
partial semantics while deepStrictEqual keeps the prototype check.
…ent loops 05c8f54 switched compareBranch's three direct-return delegations to isDeepStrictEqualWithoutPrototype, but the Set-element loop and the two Array-multiset loops still called plain isDeepStrictEqual, which enforces the new prototype gate. So partialDeepStrictEqual with a Buffer vs Uint8Array (or any content-equal, prototype-distinct value) as an array/set *element* still threw, diverging from Node. Switch those three element-matching comparisons to the prototype-skipping variant and cover arrays-of-buffers and sets-of-buffers in the regression test. Verified parity with Node v26.
Match the file's convention (bare .toThrow() for node:assert regression tests) instead of pinning assert.AssertionError.
The deepStrictEqual prototype check (issue #29030) correctly rejects the Buffer message round-tripped through advanced-serialization IPC, which comes back as a plain Uint8Array. The script is an unmodifiable upstream Node test, so a whole-file expectations entry is the only available mechanism (same shape as the test-stream-wrap entries). Unquarantine once IPC preserves the Buffer prototype.
main's deep-equal.test.ts documents Bun's strict-mode gaps as
test.failing. The prototype check in this PR fixes five of them
(x2 surfaces: assert.deepStrictEqual and util.isDeepStrictEqual):
- a null-prototype object and {}
- instances of two distinct anonymous classes
- instances of two distinct identically named classes
- an Array subclass instance and an array
- a Buffer and a Uint8Array with the same bytes
Drop their strictBug markers so they run as regular tests. The
remaining strictBug entries (extra-own-property on Date/RegExp/Map/
typed arrays/arrays, WeakMap/WeakSet) are the pre-existing
specialObjectsDequal content-only gaps this PR does not address.
b112417 to
085c74d
Compare
|
Rebased onto main (
Also dropped the stale |
Same as the deep-equal.test.ts markers: main documents the null-prototype-vs-literal gap as it.failing in deep-equals.spec.ts, and the prototype check in this PR makes it pass, which the runner reports as a failure. Drop the marker so it runs as a regular test.
There was a problem hiding this comment.
No new issues on the latest rebase. Deferring to a maintainer for the two design decisions you already flagged: (1) gating the prototype check on !enableAsymmetricMatchers so Bun.deepEquals(_,_,true) and expect().toStrictEqual() now diverge on plain objects, and (2) whole-file quarantine of test-child-process-advanced-serialization.js. Also worth a maintainer look: the new internal 4th arg to Bun.deepEquals and the checkPrototypes template axis threaded through recursion.
What was reviewed:
- Prototype-check placement (runs before
specialObjectsDequaland the array branch, so Array/Map/Set/Error subclasses are covered) and itsif constexprgating across all four template instantiations. getPrototype()exception handling — both calls haveRETURN_IF_EXCEPTIONguards; ProxygetPrototypeOftraps that throw propagate correctly.compareBranchcall sites inassert.ts— all sixisDeepStrictEqualdelegations now use the without-prototype variant, sopartialDeepStrictEqualkeeps pre-PR content-only semantics for Buffer/Uint8Array in top-level, nested, array-element, and set-element positions.- The five
strictBugmarkers removed fromdeep-equal.test.tseach correspond to a case the new check fixes; remaining markers are the unrelated own-property/WeakMap gaps.
Extended reasoning...
Overview
This PR adds prototype-identity comparison to Bun__deepEquals's strict path, fixing #29030 (assert.deepStrictEqual({}, Object.create(null)) incorrectly passing). It touches:
src/jsc/bindings/bindings.cpp— new prototype check gated onisStrict && !enableAsymmetricMatchers && checkPrototypes, placed beforespecialObjectsDequaland the array branch; newcheckPrototypestemplate parameter threaded through ~15 recursive call sites andspecialObjectsDequal.src/jsc/bindings/BunObject.cpp—Bun.deepEqualsgains an internal 4th boolean arg to selectcheckPrototypes=false.src/jsc/bindings/headers-handwritten.h— template signature update.src/js/node/assert.ts—compareBranch(backingpartialDeepStrictEqual) switched toisDeepStrictEqualWithoutPrototypeat 6 call sites.test/expectations.txt— quarantinestest-child-process-advanced-serialization.js(IPC Buffer→Uint8Array fidelity gap correctly surfaced by the new check).- Tests: new 134-line regression suite, five
strictBugmarkers removed from the deep-equal matrix, oneit.failingflipped indeep-equals.spec.ts.
Security risks
None identified. This is a comparison-semantics change; no auth, crypto, permissions, or untrusted-input parsing. getPrototype() can invoke Proxy traps, but both calls are guarded with RETURN_IF_EXCEPTION and the check runs before any raw pointers into growable state are held.
Level of scrutiny
High. Bun__deepEquals is the core comparison engine backing assert.deepStrictEqual, util.isDeepStrictEqual, Bun.deepEquals, and bun:test's toStrictEqual/toEqual. A behavior change here fans out to every user of those APIs, and this PR already went through 19 iterations precisely because early revisions broke unrelated test suites (vm cross-realm, jsonl, http headers, IPC). The final shape — a third template axis plus an internal API argument — is not mechanical.
Other factors
- Author explicitly requested maintainer sign-off (comment 2026-06-18) on the
!enableAsymmetricMatchersgate and the test quarantine. That alone rules out bot approval. - Behavior divergence introduced:
Bun.deepEquals({}, Object.create(null), true)now returnsfalsewhileexpect({}).toStrictEqual(Object.create(null))still passes. Docs claiming they're equivalent are now stale (author acknowledged, chose to leave for follow-up). - New internal API surface: 4th arg to
Bun.deepEqualsis undocumented/untyped. Reasonable choice, but a maintainer should confirm they're OK with it vs. a separate internal binding. - Whole-file test quarantine: expectations.txt policy header discourages this; author's justification (unmodifiable upstream script) is sound and matches existing
test-stream-wrap*entries, but it's the kind of trade-off a maintainer should ratify. - All 12+ prior inline review threads (mine and CodeRabbit's) are resolved. Test coverage on the new paths is good — the regression file exercises
deepStrictEqual,isDeepStrictEqual,deepEqual, andpartialDeepStrictEqualacross plain objects, subclasses of Array/Map/Set/Error, and container-element positions. - No bugs found by the bug-hunting system on the current revision.
|
Consolidating the maintainer decision points in one place, since review is otherwise clean (all threads resolved, no open findings):
Current CI run on |
|
Re: It is not broken on main; it passes on current HEAD. The failure is caused by this PR's (correct) prototype comparison in Root cause: the test does Node shows the same behavior when the input is unpooled: So |
…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>
|
Status check against current main (165dc9f), after #34434 and #34660 merged. Covered by main:
Not covered by main:
So the |
1 participant
Fixes #29030.
Repro
Node throws
ERR_ASSERTIONbecause the two objects' prototypes differ(
Object.prototypevsnull). Bun printsASSERTION PASSED.Cause
Bun__deepEqualsinsrc/bun.js/bindings/bindings.cpponly comparedJSObject::calculatedClassNamein the strict branch. For both{}andObject.create(null)that's"Object", so the check passed and neitherobject had any own properties for the subsequent enumeration step to
differentiate.
Fix
Compare prototypes with
===next to the class name check in the strictbranch — this mirrors Node's
util.isDeepStrictEqual:Non-strict
deepEqual/toEqualare untouched.Verification
bun bd test test/regression/issue/29030.test.ts— 9/9 pass.USE_SYSTEM_BUN=1 bun test test/regression/issue/29030.test.ts— 2/9 fail,the two
{}vsObject.create(null)cases the bug actually affects.Full
test/js/node/assert/suite (92 tests) andtest/js/bun/bun-object/deep-equals.spec.ts(36 tests) still pass.[review] gate passed · iteration 20 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 20
evidence per changed file
root cause · written by the author bot
The bug was that Bun's deep strict equality comparison did not compare the prototypes of the objects being checked, so values with different prototypes, such as a plain object literal and an object created with Object.create(null), were incorrectly reported as strictly deep equal. Node's assert.deepStrictEqual and util.isDeepStrictEqual require prototype identity as part of strict comparison, so this divergence caused assertions that should throw to pass silently. The fix adds a prototype identity check to the strict-mode deepEquals path, which also resolves several previously documented st…