Bun.deepEquals: fix four false-positive equality classes - #32872
Conversation
WalkthroughFixes ChangesdeepEquals correctness fixes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 2:57 PM PT - Jul 6th, 2026
❌ @robobun, your commit 64f5b68 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32872That installs a local version of the PR into your bun-32872 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the related-PR findings above:
The array non-index-property fix (bug 1 here) is not covered by either. |
![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: 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 1471-1473: The second specialObjectsDequal call in the bindings
comparison path can throw but its result is used immediately afterward without
checking the scope. Update the logic in the specialObjectsDequal(c2, c1) branch
to add the same RETURN_IF_EXCEPTION guard before calling has_value(), keeping
the existing comparison flow in the surrounding equality check code.
In `@test/js/node/test/parallel/test-assert-typedarray-deepequal.js`:
- Around line 50-55: The Bun-specific deepEqual assertion should not be added to
the upstream Node port in
test/js/node/test/parallel/test-assert-typedarray-deepequal.js. Remove that
comment/assert block from the Node test and place the Bun-only coverage in
test/js/node/assert/assert-typedarray-deepequal.test.ts instead, keeping the
upstream port diffable and unchanged while preserving the same
makeBlock/assert.deepEqual behavior in the Bun-specific test.
🪄 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: 77993049-6562-4454-8162-74bbb716aa52
📒 Files selected for processing (8)
src/js/internal/sql/shared.tssrc/jsc/bindings/bindings.cpptest/js/bun/bun-object/deep-equals.spec.tstest/js/bun/test/expect.test.jstest/js/node/assert/assert-typedarray-deepequal.test.tstest/js/node/test/parallel/test-assert-typedarray-deepequal.jstest/js/sql/sqlite-sql.test.tstest/regression/issue/15314.test.ts
There was a problem hiding this comment.
c8b2340 addresses my earlier feedback and the array path looks correct now — deferring to a human for the intentional assert.deepEqual divergence from Node on float typed arrays and the Build #65653 failures.
Extended reasoning...
Overview
This PR fixes four false-positive equality classes in Bun__deepEquals (src/jsc/bindings/bindings.cpp): array non-index own properties, null-prototype vs Object.prototype in strict mode, float typed array -0/+0/NaN, and boxed-String own properties. It also touches SQLResultArray in src/js/internal/sql/shared.ts (making the metadata descriptors explicitly enumerable: false, configurable: true), updates expect.test.js and the regression test for #15314 to match the tightened semantics, and rewrites two node:assert typedarray tests to document an intentional divergence from Node.
I left two rounds of inline feedback on earlier revisions (prototype-walking lookups, positional-offset skip, and a hasOwnProperty-vs-enumerable mismatch). The author addressed all of it; c8b2340 now builds a1Names/a2Names hash sets from the enumerable-only name arrays and uses them for both the first-loop absence check and the second-loop skip, with own-only getOwnPropertySlot lookups. That matches what I suggested and the added regression tests cover each case in both argument orders.
Security risks
None. This is structural-equality logic with no auth, crypto, I/O, or untrusted-input parsing implications.
Level of scrutiny
High. Bun__deepEquals backs every expect().toEqual() / toStrictEqual() call in every Bun test suite, plus assert.deepEqual / assert.deepStrictEqual. The array path now allocates two UncheckedKeyHashSets and does per-property getOwnPropertySlot calls for every array comparison (the common case has zero non-index enumerable own props, so the sets are empty, but it's still a hot path). Regressions here silently flip test outcomes across the ecosystem.
Other factors
- Design decision: the PR deliberately makes
assert.deepEqual(loose) useObject.issemantics for float typed arrays, diverging from Node. The rationale (Jest'stoEqualis the primary consumer of the!isStrictpath) is reasonable, and the divergence is documented in both modified Node test files, but a maintainer should sign off on intentionally editing vendoredtest/js/node/test/parallel/expectations. - CI: robobun reports failures in Build #65653 for the latest commit.
- Overlap: #29037 and #19622 cover overlapping ground; the author's reply explains the scoping but coordination is a human call.
- The
SQLResultArraychange addsconfigurable: true(previously the defaultfalse); minor, but it's a user-observable behaviour change tucked into an equality PR.
|
An independent report of the null prototype case came in (
On the null-only check versus the full prototype identity comparison in #29037: the two references disagree once you go past the null case. For One small note on the implementation: |
|
I hit the same Three things in that branch that this PR does not have, in case they are worth folding in:
For the record, the repro that led me here: |
There was a problem hiding this comment.
All three rounds of my earlier findings are addressed and the current revision looks correct to me; given this changes core toEqual/toStrictEqual semantics (including the documented assert.deepEqual float-array Node divergence and the SQLResultArray descriptor change), I'd like a human to sign off on the semantics choices before merge.
Extended reasoning...
Overview
This PR rewrites four areas of Bun__deepEquals / specialObjectsDequal in src/jsc/bindings/bindings.cpp (~150 lines of C++ touching JSC property enumeration, hash-set name reconciliation, exception scopes, and float-typed-array element comparison), makes SQLResultArray metadata non-enumerable in src/js/internal/sql/shared.ts so SQL results keep toEqual-ing plain arrays under the new semantics, adds a documented expected-FAIL to test/expectations.txt for the vendored Node typed-array deepEqual test, and updates/adds tests across six test files.
Security risks
None identified. This is equality-comparison logic in the test runner / Bun.deepEquals; no auth, crypto, network, or filesystem surface is touched.
Level of scrutiny
High. Bun__deepEquals is the engine behind every expect().toEqual() / toStrictEqual() / assert.deepEqual / assert.deepStrictEqual call in user test suites. The change deliberately picks Jest Object.is semantics over Node == semantics for assert.deepEqual on float typed arrays (and records the vendored Node test as an expected failure), introduces a null-prototype check whose scope was explicitly weighed against the broader approach in #29037, and required three follow-up commits to close edge cases I flagged in earlier reviews. These are semantics decisions and cross-PR overlap (#29037, #19622) that a maintainer should ratify.
Other factors
- All inline review threads (mine and CodeRabbit's) are resolved; commits 828bb20, c8b2340, 89b2503, and c7e1737 addressed each round of feedback with regression tests added.
- The bug-hunting system found no issues on the current revision.
- The
SQLResultArraydescriptor change is technically a no-op (enumerable: falsewas already the default fordefinePropertieswithout an explicit flag) but addingconfigurable: trueis a real behaviour change to a public-ish result type. - robobun's last comment lists three optional follow-ups (the
toStringInlinevsinternalValue()read, anisDebugtimeout for the stress test, and non-enumerable harness prototype pollution) that the author may or may not want to fold in before merge. - Test coverage for the new behaviour is thorough and includes both argument orders for symmetry.
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 `@test/expectations.txt`:
- Around line 30-36: Shorten the quarantine note in expectations.txt to fit the
repository’s 3-line comment limit. Keep only a brief summary of the divergence
between Bun’s deepEqual/expect().toEqual semantics and Node’s float typed array
behavior, and move the longer explanation to the PR description or linked issue.
Update the existing comment block around the assert.deepEqual / Jest semantics
note so it stays concise and under 3 lines.
In `@test/js/bun/jsonl/jsonl-parse.test.ts`:
- Around line 1288-1290: The JSONL parse test currently only checks the parsed
object’s value, own property names, and prototype, which still allows an
accessor or non-enumerable __proto__ property to slip through. Update the test
around result[0] in jsonl-parse.test.ts to also assert
Object.getOwnPropertyDescriptor(result[0], "__proto__") so it explicitly
verifies the parser produces a normal enumerable data property.
🪄 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: 4f139f58-e420-4014-b2a6-803825be5278
📒 Files selected for processing (4)
src/jsc/bindings/bindings.cpptest/expectations.txttest/js/bun/bun-object/deep-equals.spec.tstest/js/bun/jsonl/jsonl-parse.test.ts
There was a problem hiding this comment.
All my earlier findings are addressed and the bug hunter found nothing new, but this rewires the core Bun__deepEquals engine (every toEqual/toStrictEqual/assert.deepEqual goes through it) and deliberately diverges assert.deepEqual float-typed-array semantics from Node — that design call plus the three optional follow-ups robobun listed above are worth a maintainer's eyes before merge.
Extended reasoning...
Overview
The PR modifies Bun__deepEquals / specialObjectsDequal in src/jsc/bindings/bindings.cpp to fix four false-positive equality classes: array non-index own properties, strict-mode null-prototype vs Object.prototype, float typed array -0/+0/NaN semantics, and boxed-String own properties. It also reworks the generic slow-path and ErrorInstance second loops to use a name hash-set instead of a positional offset, adds a missing RETURN_IF_EXCEPTION, makes SQLResultArray metadata non-enumerable so SQL results toEqual plain arrays again, and updates seven test files plus expectations.txt.
Security risks
None identified. The change is to a pure comparison function with no I/O, auth, or untrusted-input parsing. The SQLResultArray change only flips descriptor flags on internally-created result objects.
Level of scrutiny
High. Bun__deepEquals is the engine behind every expect().toEqual() / toStrictEqual() assertion in Bun's test runner and behind node:assert deep-equality. A subtle regression here silently changes pass/fail outcomes across the entire user test ecosystem. The PR also makes an intentional, documented divergence: assert.deepEqual on float typed arrays now uses Jest Object.is semantics rather than Node's ==, and the upstream Node parallel test is quarantined as an expected failure. That trade-off is reasonable and well-argued in the description, but it's exactly the kind of compatibility decision a maintainer should sign off on.
Other factors
- This PR went through three rounds of correctness fixes during review (prototype-walking lookups, positional-offset second loop, non-enumerable shadowing); all are now resolved with regression tests, and the current bug-hunting pass found nothing.
- robobun's comment at 00:24 lists three optional items from a parallel branch (
internalValue()vstoStringInlineforStringObjectType, the debug+ASANexpect.test.jstimeout, and the enumerableString.prototypeharness pollution) that haven't been folded in — not blockers, but worth a maintainer deciding whether to take them here or separately. - The
SQLResultArrayenumerable→non-enumerable flip is a user-visible behaviour change (necessary because the array path now actually looks at non-index own props), covered by a new test. - Overlaps with open PRs #29037 and #19622 are acknowledged and reconciled in-thread.
Given the breadth of what deepEquals underpins and the deliberate Node-divergence call, I'm deferring rather than approving.
|
Folded in item 1 from the branch-comparison note above: Items 2 and 3 (the pre-existing |
There was a problem hiding this comment.
No further issues from automated review on 26e72d6 — all earlier findings are resolved — but given this reworks the core Bun__deepEquals C++ path (with the deliberate Jest-over-Node assert.deepEqual float divergence and the SQLResultArray descriptor change), it's worth a maintainer sign-off before merge.
Extended reasoning...
Overview
This PR modifies Bun__deepEquals / specialObjectsDequal in src/jsc/bindings/bindings.cpp (~150 lines of C++), the engine behind expect().toEqual(), expect().toStrictEqual(), Bun.deepEquals, and assert.deepEqual/deepStrictEqual. It fixes four false-positive equality classes (array non-index own props, strict null-prototype, float typed-array Object.is semantics, boxed-String own props) plus a knock-on change making SQLResultArray metadata properties non-enumerable/configurable so SQL results still toEqual plain arrays under the new array path. Test updates span 7 files including a new expectations.txt quarantine for the upstream Node typedarray test.
Security risks
None identified. The StringObjectType change from toStringInline() to internalValue() actually removes a user-code invocation point from inside the comparator. No auth/crypto/permissions surface.
Level of scrutiny
High. This is production-critical native code on a hot path exercised by essentially every Bun test and every user test using toEqual. The change went through three iterative bug-fix rounds during review (prototype-walking lookups, positional-offset second loop, non-enumerable shadowing), which itself signals the subtlety of the property-enumeration semantics involved. It also encodes two design decisions a maintainer should ratify: (1) choosing Jest's Object.is over Node's == for assert.deepEqual float typed arrays (documented divergence, quarantines an upstream Node test), and (2) the narrow null-prototype check vs #29037's full prototype-identity comparison.
Other factors
All prior inline findings (mine and CodeRabbit's) are resolved. The author verified against Jest 30 and Node 26 and ran the affected test directories. The SQLResultArray change adds configurable: true where it was previously absent (default false), a small user-visible API tweak. The slow-path getIfPropertyExists prototype-walk is intentionally deferred to a follow-up with a stated rationale. CI on d199a25 reported failures; I can't see the status of 26e72d6. Overlaps with #29037 and #19622 are acknowledged in-thread.
|
Status at 26e72d6: the diff is green. Every test lane that ran passed, including debian-13-x64-asan which exercises the deepEquals changes under ASAN. The only red is the three macOS lanes on build 66335, and none of them got as far as running a test:
A All review threads are resolved. This is ready for a maintainer; a manual re-run of just the darwin test lanes, or a merge on the strength of the other platforms, is the unblock. |
|
Status at 26e72d6: the diff is green; the remaining red is macOS Buildkite infrastructure, not a test failure. On build 66335 (head sha), every Linux lane passes: glibc and musl, x64 and aarch64, ASAN, baseline, plus all builds, Format, and Lint. There are zero test-failure annotations on the build. The three red lanes are all macOS agent issues and no test ran on any of them:
A Ready for review. |
26e72d6 to
2244354
Compare
|
Rebased onto main (force-pushed, 9 commits squashed into 1 — the history was review-fixups on one change plus a The rebase changed the fix in one substantive way, for the better. #33323 landed a spec-driven deep-equality matrix that marks two of the bugs here as
Both pass with this PR, so those markers are removed; 5 matrix tests flip from The float typed-array divergence is gone. That matrix also pins They are different callers, and
which is correct for every caller. So:
Re-checking my own earlier reasoning: node's loose Verification after the rebase: (The 4 failures in |
|
The walkthrough above was generated against
There is no intentional divergence from Node left in the diff. Details in the rebase note above. On the out-of-scope warning: the PR fixes four false-positive classes found by one equality fuzz corpus, and #29030 happens to be the filed issue for one of them (the null prototype). They are four arms of the same |
2244354 to
f1fe97a
Compare
Bun.deepEquals (and by extension expect().toEqual / toStrictEqual) returned
true for several input pairs that Jest and Node consider unequal, meaning
user tests could pass when they should have failed.
- Arrays with non-index own string-keyed properties were compared by index
and symbol keys only. Now enumerates non-index own properties with
getOwnNonIndexPropertyNames, looks them up own-only via getOwnPropertySlot
so Array.prototype members are not matched, and reconciles the two name
lists through a hash set so a name unique to either side is never skipped.
- In strict mode (toStrictEqual), a null-prototype object compared equal to
an Object.prototype object with the same own properties because only
calculatedClassName was checked. Now also rejects a null/non-null
prototype mismatch.
- Float typed array elements were compared with == on every loose entry
point. jest's toEqual wants Object.is there (NaN equals NaN, -0 is distinct
from +0); node's assert.deepEqual wants ==, which says the opposite for
both. These are separate callers, so the Object.is comparison is gated on
the jest entry points and node's == is left alone. The strict path memcmps,
which already matched both toStrictEqual and isDeepStrictEqual.
- Boxed String objects (new String("x")) returned early after comparing the
primitive value, ignoring own properties. Now falls through to the generic
own-property comparison like Number/Boolean wrappers do, and reads the
boxed [[StringData]] via internalValue() rather than toStringInline(), so
a user-defined toString cannot run inside or change the comparison.
The generic slow path and the ErrorInstance path shared a second loop that
started at a1.size(), assuming positional correspondence between the two
property-name lists; a name unique to the second object below that index was
never examined. Both now reconcile by name membership.
SQLResultArray's count/command/lastInsertRowid/affectedRows were declared as
class fields and then re-described without an explicit enumerable flag, so
they stayed enumerable and defeated the stated intent of hiding them from
for-in. With arrays now comparing non-index own properties, every
expect(sqlResult).toEqual([...]) would have started failing; make them
non-enumerable as originally intended.
The null-prototype and array-extra-property rows in the deep-equality matrix
added by #33323 were marked test.failing; they pass now, so the markers are
removed. Closes #29030.
Updates the assertions in expect.test.js, 15314 and jsonl-parse that encoded
the old behaviour.
Six tests in jsonl-parse and sqlite-sql exceed bun:test's 5s per-test budget
on an -O0 build (4 GB allocations, 50k-iteration loops, 10k awaited inserts;
12-13s observed) and fail there with or without this change. They are well
inside the budget on release and release+ASAN, which is what CI runs, so they
are skipped on debug builds only. Both files are already touched here, so the
gate runs them.
f1fe97a to
64f5b68
Compare
|
Status at
Every Linux lane (glibc + musl, x64 + aarch64, ASAN, baseline), the x64 macOS lanes, and all build steps pass. The one For review, the one non-equality thing in the diff: six tests in Ready for review. |
…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>
|
The null-prototype vs object literal part of this (the strict mode check in |
|
Status check against current main (165dc9f), after #34434 and #34660 merged. Applied only this PR's test changes on top of main ( Covered by main:
Still failing on main, so not covered:
So the |
1 participant
Fixes #29030.
Bun.deepEqualsis the engine behindexpect().toEqual()/expect().toStrictEqual(). An adversarial three-way corpus (Bun vs Jest 30 vs Nodeutil.isDeepStrictEqual) surfaced four input classes where Bun says equal but Jest and Node say not equal, meaning user tests can pass that should fail.Repro
All verified against
expect@30(toEqual/toStrictEqual) and Node 26util.isDeepStrictEqual.Cause
In
src/jsc/bindings/bindings.cppBun__deepEquals/specialObjectsDequal:PropertyNameMode::Symbols, so string-keyed non-index properties were never seen.JSObject::calculatedClassName, which is"Object"for both a null-prototype object and a plain object.!isStrictfloat-typed-array path compared elements with IEEE==on every loose entry point, soexpect().toEqual()saw-0 == +0andNaN != NaN.StringObjectTypereturned immediately after comparing the primitive string value, skipping own properties entirely.Fix
getOwnNonIndexPropertyNameswithStringsAndSymbolsand compare the remaining properties on the second array the same way the generic object path already does.deepStrictEqual#29037 proposes the broader full-prototype-identity comparison.Object.issemantics (NaN equals NaN,-0distinct from+0) on the jest entry points only.enableAsymmetricMatchersalready distinguishesjestDeepEqualsfromdeepEquals, so node'sassert.deepEqualkeeps its==semantics and nothing diverges.StringObjectType: fall through to the generic own-property comparison after the primitive value matches, the same wayNumberObjectType/BooleanObjectTypealready do.On the float typed-array semantics
expect().toEqual()andassert.deepEqualwant opposite things here: jest compares float elements withObject.is(NaN equals NaN,-0is distinct from+0), node compares them with==(exactly the reverse on both). They are different callers, and theenableAsymmetricMatcherstemplate parameter already tells them apart (jestDeepEqualsvsdeepEquals), so theObject.iscomparison is gated on it and node's==path is untouched. The strict path memcmps, which already satisfied bothtoStrictEqualandisDeepStrictEqual.An earlier revision of this PR applied
Object.isunconditionally and quarantined the vendored node test; that is no longer needed and the quarantine has been dropped.Verification
The 5 failures in the matrix without the fix are the
test.failingrows this PR un-marks (below).Rebase note
Rebased onto main after #33323 landed a spec-driven deep-equality matrix. That matrix marked two of the bugs this PR fixes as
test.failing:a null-prototype object and {}(strictBug) —assert.deepStrictEqualignores prototype differences #29030an array with an extra own property(strictBug+looseBug)Both pass now, so the markers are removed (5 matrix tests flip from
test.failingto passing). Its float rows pin node's==semantics, which is what prompted scoping theObject.ischange to the jest callers rather than diverging.Not in this PR
toStrictEqualrejecting transparent Proxies.console.tableinvoking getters twice.