Treat two Invalid Dates as equal in deepEquals and fix their failure-message rendering - #34819
Treat two Invalid Dates as equal in deepEquals and fix their failure-message rendering#34819robobun wants to merge 5 commits into
Conversation
…lure-message rendering
Two Dates with NaN time values now compare equal in deepEquals
(toEqual, toStrictEqual, Bun.deepEquals, assert.deepStrictEqual,
util.isDeepStrictEqual), matching Node.js.
The test runner's failure diff printed a sliced 'null' ("ul") for
Invalid Date because JSON.stringify(new Date(NaN)) is unquoted null;
it now prints "Invalid Date" like console.log already does.
Fixes #34816
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesInvalid Invalid Date handling
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)test/js/bun/test/expect.test.jsast-grep timed out on this file Comment |
|
Updated 7:50 AM PT - Jul 20th, 2026
❌ @robobun, your commit 6e737f3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34819That installs a local version of the PR into your bun-34819 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the inline nit about draining stdout in the new subprocess test is worth tidying but not blocking.
Extended reasoning...
Overview
Two small, localized fixes for Invalid Date handling:
src/jsc/bindings/bindings.cpp: the Date arm ofspecialObjectsDequalnow treats two NaN time values as equal (a == b || (isnan(a) && isnan(b))), matching Node'sObject.is(getTime(), getTime())semantics. This flows throughBun.deepEquals,toEqual/toStrictEqual,assert.deepStrictEqual/deepEqual, andutil.isDeepStrictEqual.src/runtime/test_runner/pretty_format.rs: the JSDate branch of the failure-diff formatter now special-cases thenulloutput ofJSON.stringify(new Date(NaN))and printsInvalid Dateinstead of quote-trimming it intoul. This is a byte-for-byte copy of the existing branch insrc/jsc/ConsoleObject.rs:4372.
Tests add matrix rows to three existing files and one new subprocess test asserting the failure message.
Security risks
None. No user-controlled input parsing, no allocation, no I/O — pure comparison/formatting of an already-validated internal double.
Level of scrutiny
Low. Both native changes are a handful of lines each, follow established sibling code exactly, and are covered by tests across every consumer surface (Bun.deepEquals strict/loose, toEqual/toStrictEqual, assert strict/loose, util.isDeepStrictEqual, plus the failure-message rendering). I checked for other Date-equality comparison sites via internalNumber() in src/jsc/bindings/ — this is the only one; the rest are extractions, not comparisons. ECMA-262's TimeClip guarantees the internal value is either an integral finite number or NaN, so the added disjunct doesn't change ±0 behavior.
Other factors
- The one semantic choice — diverging from jest (which also fails
new Date(NaN)equality) in favor of Node parity — is explained in the description, is consistent withexpect(NaN).toEqual(NaN)already passing, and is scoped behindisBunin the shared expect test. - The inline nit (piped-but-undrained stdout in the new subprocess test) is a harness-convention cleanup; the child writes far under the pipe buffer here, so it's not a correctness or flake risk today.
|
Not a duplicate of #33068: that PR is a broader node:assert deep-equality rewrite that happens to include the same one-line NaN-date change in specialObjectsDequal. This PR is a focused fix for #34816 and also fixes the test runner's failure-message rendering for Invalid Date (the "Expected: ul" garbage), which #33068 does not touch. If #33068 lands first, the bindings.cpp hunk here becomes a trivial merge conflict and the rest still applies. |
There was a problem hiding this comment.
LGTM — focused two-line native fix with matching test coverage; the earlier stdout-drain nit was addressed in 3cc58c6.
What was reviewed:
specialObjectsDequalDate arm: NaN-aware compare is the standarda == b || (isnan(a) && isnan(b))shape, matches Node'sObject.is(getTime())semantics.pretty_format.rsDate branch: byte-for-byte the sameb"null" → "Invalid Date"guard already inConsoleObject.rs:4372, so the two formatters now agree.- New tests cover all surfaces sharing the comparison (Bun.deepEquals strict/loose, toEqual/toStrictEqual, assert.deepStrictEqual/deepEqual, util.isDeepStrictEqual) plus invalid-vs-valid inequality and the failure-message rendering.
Extended reasoning...
Overview
Two tiny native changes plus test coverage across four files:
src/jsc/bindings/bindings.cpp(+4/-1): theJSDateTypearm ofspecialObjectsDequalnow treats two NaN time values as equal instead of falling throughNaN == NaN.src/runtime/test_runner/pretty_format.rs(+4/-1): the JSON-stringify Date path now maps the unquotednullfromJSON.stringify(new Date(NaN))toInvalid Datebefore the quote-trim step, exactly mirroring the existing branch insrc/jsc/ConsoleObject.rs:4372-4376.- Test additions in
deep-equals.spec.ts,expect.test.js,deep-equal.test.ts, and a new subprocess test for the failure-message rendering.
Security risks
None. This is equality semantics for Date objects and diagnostic-message formatting. No untrusted input parsing, no auth/crypto/permission surface, no memory management changes. internalNumber() returns a plain double from the DateInstance, and std::isnan on it is side-effect-free.
Level of scrutiny
Low. Each native change is a single conditional following an established pattern:
- The bindings.cpp change is the canonical NaN-aware double compare; it only widens equality for the one case where both sides are NaN and cannot change any other Date comparison result.
- The pretty_format.rs change is a copy of the sibling branch already shipping in the console formatter, applied at the same point in an otherwise-identical code shape (
out_buf == b"null"before thelen() > 2quote-trim). Theb"null"check is on raw JSON output inside thejs_type == JSType::JSDateguard, so it can't collide with a valid ISO string.
Other factors
- Bug-class coverage: the PR covers every consumer of
specialObjectsDequalfor Dates (Bun.deepEquals both modes, toEqual/toStrictEqual, node:assert strict+loose, util.isDeepStrictEqual) and the negative case (invalid vs valid), matching REVIEW.md's variant-matrix guidance. - Jest divergence: the
expect.test.jsaddition is correctly gated onif (isBun)since upstream jest disagrees; the PR description explains the Node-parity rationale. - Prior feedback addressed: my earlier nit about the undrained stdout pipe in
expect-invalid-date-message.test.tswas fixed in 3cc58c6 — the test now uses the canonical three-wayPromise.all([stdout.text(), stderr.text(), exited]). - Harness conventions: the new test file uses
tempDir,bunExe, spreadsbunEnv, setsNO_COLOR, asserts stderr content before the exit code, and asserts a nonzero exit — all in line with the repo's test guidance. - Overlap with #33068: the author already noted the bindings.cpp hunk overlaps a larger open PR; that's a merge-conflict concern, not a correctness one, and this PR additionally fixes the
ulrendering that #33068 doesn't touch.
There was a problem hiding this comment.
LGTM — the earlier stdout-drain nit was addressed in 3cc58c6 and this run found nothing further.
What was reviewed:
specialObjectsDequalDate branch: NaN-aware equality matches Node'sObject.is(getTime())semantics; valid-vs-invalid still returns false.pretty_format.rschange is byte-for-byte the same guard already inConsoleObject.rs:4372, scoped to theJSDatebranch only.- New tests cover all shared entry points (Bun.deepEquals strict/loose, toEqual/toStrictEqual, assert.deepStrictEqual/deepEqual, util.isDeepStrictEqual) plus the failure-message rendering; evidence gate confirms they fail on main and pass here.
Extended reasoning...
Overview
Two tiny production changes fixing #34816: (1) src/jsc/bindings/bindings.cpp — the JSDateType arm of specialObjectsDequal now treats two NaN time values as equal (leftTime == rightTime || (isnan(leftTime) && isnan(rightTime))), matching Node's Object.is semantics for Date comparison; (2) src/runtime/test_runner/pretty_format.rs — the JSON-stringify-then-trim-quotes path for Dates now special-cases the unquoted null that JSON.stringify(new Date(NaN)) produces and prints Invalid Date instead of the sliced garbage ul. Four test files add coverage: Bun.deepEquals matrix, node:assert matrix, expect.test.js toEqual/toStrictEqual (Bun-only, since jest diverges), and a new subprocess test asserting the failure-message text.
Security risks
None. This is deep-equality comparison logic and diagnostic output formatting for the test runner; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The C++ change is a 3-line arithmetic tweak in a well-understood switch arm with no allocation, no exception-throwing calls, and no lifetime concerns — internalNumber() is a plain double read. The Rust change is an exact copy of the guard that already exists at src/jsc/ConsoleObject.rs:4372-4377 (the console formatter that this pretty-formatter is a near-duplicate of), so it follows established precedent rather than introducing new logic. The b"null" check is inside the js_type == JSType::JSDate branch, so it can't affect non-Date JSON output.
Other factors
- The evidence gate in the PR description shows the new tests fail on main (5 failures ASAN, 12 release) and pass with the fix (0 failures both), satisfying the "fails for the right reason" requirement.
- My prior review nit (undrained
stdout: "pipe") was addressed in 3cc58c6 — the current diff shows the canonical three-wayPromise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]). - The PR description explicitly documents the jest divergence and justifies choosing Node parity (consistent with
expect(NaN).toEqual(NaN)already passing), and the expect.test.js additions are gated behindif (isBun)so the file still runs against jest. - The overlap with #33068 was acknowledged; this PR's pretty-format fix is unique to it and the bindings.cpp hunk would be a trivial merge if #33068 lands first.
![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/runtime/test_runner/pretty_format.rs`:
- Around line 1940-1942: Update the JSON date formatting logic around the
out_buf == b"null" branch to render Invalid Date only when the Date’s internal
time value is invalid, rather than for every null result from toJSON(). Preserve
null output for valid Dates whose toJSON() returns null, and add a regression
test covering that case.
In `@test/js/bun/test/expect.test.js`:
- Around line 725-731: Extend the invalid-versus-valid Date assertions in the
isBun block to also use toStrictEqual: assert new Date(NaN) is not strictly
equal to new Date(0), and new Date(0) is not strictly equal to new Date(NaN).
Keep the existing toEqual assertions and invalid-Date equality checks unchanged.
🪄 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: 9fca0639-a12a-4401-a7ce-731642f46734
📒 Files selected for processing (6)
src/jsc/bindings/bindings.cppsrc/runtime/test_runner/pretty_format.rstest/js/bun/bun-object/deep-equals.spec.tstest/js/bun/test/expect-invalid-date-message.test.tstest/js/bun/test/expect.test.jstest/js/node/assert/deep-equal.test.ts
There was a problem hiding this comment.
LGTM — focused fix with good test coverage across all affected surfaces.
What was reviewed:
- Date comparison in
specialObjectsDequal: NaN-handling matches Node'sObject.is(getTime(), getTime())semantics; valid-vs-invalid still rejects. pretty_format.rsb"null"branch: confirmed byte-for-byte match withConsoleObject.rs:4372-4377, so the two formatters agree.- Tests slot into existing
it.each/casesmatrices; subprocess test drains all pipes and asserts exit code 1.
Extended reasoning...
Overview
Two-line native fix in src/jsc/bindings/bindings.cpp (Date branch of specialObjectsDequal) so two Invalid Dates compare equal, plus a three-line branch in src/runtime/test_runner/pretty_format.rs so the failure diff prints Invalid Date instead of the sliced garbage ul. Four test files add coverage: Bun.deepEquals, expect().toEqual/toStrictEqual, assert.deepStrictEqual/deepEqual/util.isDeepStrictEqual, and a subprocess test for the failure-message rendering.
Security risks
None. No user input parsing, allocation, or I/O changes. The bindings.cpp change only adds an std::isnan check on doubles already read from DateInstance::internalNumber(); the formatter change swaps a static byte slice.
Level of scrutiny
Low-to-moderate. The specialObjectsDequal path is shared by toEqual, toStrictEqual, Bun.deepEquals, and the node:assert/util surfaces, so a regression would be visible — but the change is a strict widening (a == b || (isnan(a) && isnan(b))) that only affects the case where both were previously NaN, which was demonstrably wrong. The formatter change is a verbatim copy of the existing ConsoleObject.rs branch (verified at lines 4372-4377), so it brings the two copies into agreement rather than introducing new behavior.
Other factors
- The robobun evidence gate shows the new tests fail on both ASAN-debug and release builds without the fix and pass with it.
- My earlier nit (undrained stdout pipe) was fixed in 3cc58c6; CodeRabbit's toStrictEqual matrix suggestion was applied in 825a3fc; the toJSON-returns-null edge case was reasonably declined (mirrors console formatter, strictly better than the prior
uloutput). All review threads are resolved. - Test additions follow existing file conventions (extending
it.eachtables and thecasesmatrix rather than new ad-hoc blocks). - Overlap with #33068 is a single-line intersection; this PR additionally fixes the failure-message rendering, which #33068 does not.
There was a problem hiding this comment.
LGTM — focused two-line fix with the right semantics and coverage.
What was reviewed:
- Date NaN comparison in
specialObjectsDequal— additive to the existing==, matches Node's Object.is semantics; valid-vs-invalid still unequal. pretty_format.rsb"null"branch mirrorsConsoleObject.rs:4372byte-for-byte, so the two formatters stay in agreement.- Prior review nits (stdout drain,
toStrictEqualnegative cases) are addressed and resolved; tests spanBun.deepEquals,expect, andnode:assert.
Extended reasoning...
Overview
Two tightly scoped changes: (1) src/jsc/bindings/bindings.cpp adds || (std::isnan(leftTime) && std::isnan(rightTime)) to the JSDateType arm of specialObjectsDequal, so two Invalid Dates compare equal under Bun.deepEquals/toEqual/assert.deepStrictEqual; and (2) src/runtime/test_runner/pretty_format.rs special-cases JSON.stringify(new Date(NaN)) === "null" to print Invalid Date instead of the quote-trimmed garbage ul. Four test files add coverage across every surface that shares the comparison.
Security risks
None. No untrusted input parsing, no allocation, no new JS entry points. internalNumber() is a plain double read on an already-type-checked DateInstance; std::isnan is pure. The formatter change compares a local buffer to a literal.
Level of scrutiny
Low-to-moderate. The C++ change is a one-clause boolean OR that is strictly additive to the previous == — every case that returned true before still does, and only NaN/NaN flips from false to true, which is the documented Node behavior. The Rust change is copied verbatim from the sibling formatter at ConsoleObject.rs:4372-4377, so it introduces no divergence between console.log and expect-diff rendering. The PR description includes gate evidence that the new tests fail on main (both ASAN debug and release) and pass with the fix.
Other factors
All three prior review threads are resolved: the stdout-drain harness nit I raised was fixed in 3cc58c6, CodeRabbit's toStrictEqual invalid-vs-valid coverage was added in 825a3fc, and CodeRabbit withdrew the toJSON()→null concern once formatter parity was pointed out. Test coverage hits the full variant matrix — Bun.deepEquals strict/loose, toEqual/toStrictEqual, assert.deepStrictEqual/deepEqual/util.isDeepStrictEqual, plus the negative (invalid vs valid) direction and the failure-message subprocess test. Nothing left outstanding.
1 participant
Fixes #34816
Repro
Also affected the Node compat surface:
util.isDeepStrictEqual(new Date(NaN), new Date(NaN))returnedfalse(Node returnstrue), andassert.deepStrictEqualthrew.Cause
Two bugs:
specialObjectsDequalin bindings.cpp compared Dates withleft->internalNumber() == right->internalNumber(), which isNaN == NaNfor Invalid Dates, so they never compared equal. Node compares Dates withObject.is(getTime(), getTime())semantics, under which two NaN time values are equal.JSON.stringify(new Date(NaN))is the unquotednull, so trimming produced the garbageul. The console formatter already special-cases this and printsInvalid Date; the test runner's copy was missing that branch.Fix
toEqual,toStrictEqual,Bun.deepEquals,assert.deepStrictEqual/deepEqual, andutil.isDeepStrictEqual(they share the same comparison). This matches Node; jest'sexpectdisagrees (it also fails this case), but Node parity plusexpect(NaN).toEqual(NaN)passing makes equal the consistent answer.Invalid Datefor a NaN Date, matchingconsole.log.Verification
New tests fail on the released build and pass with this change:
test/js/bun/bun-object/deep-equals.spec.ts:Bun.deepEquals(new Date(NaN), new Date(NaN))in both strict modes, plus invalid-vs-valid inequalitytest/js/node/assert/deep-equal.test.ts: matrix entries for two invalid dates (strict and loose) and invalid vs validtest/js/bun/test/expect.test.js:toEqual/toStrictEqualon Invalid Dates (Bun-only branch, since jest diverges)test/js/bun/test/expect-invalid-date-message.test.ts: failure message rendersReceived: Invalid Date, notul[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
Deep equality compared two Date objects by checking their internal time values with a plain numeric equality, which fails when both dates are invalid because an invalid date's time value is NaN and NaN never equals itself. The fix updates the Date branch of the native deep-equality routine to treat two NaN time values as equal, matching Node's Object.is semantics, so that
expect(new Date(NaN)).toEqual(new Date(NaN))and the related assert and Bun.deepEquals surfaces now pass while invalid versus valid dates still compare unequal. A companion change in the test runner's pretty formatter ma…