Fix toEqual treating NaN elements of float typed arrays as unequal - #34818
Fix toEqual treating NaN elements of float typed arrays as unequal#34818robobun wants to merge 2 commits into
Conversation
…qual Jest's toEqual compares typed array elements with Object.is semantics: NaN equals NaN and +0 differs from -0. The non-strict deepEquals float path used IEEE comparison for all callers, which matches node's loose assert.deepEqual but broke expect(new Float16Array([NaN])).toEqual(...). Keep the IEEE comparison for the node/Bun.deepEquals path and use Object.is semantics when asymmetric matchers are enabled (jest path). Fixes #34815
|
Updated 6:27 AM PT - Jul 20th, 2026
✅ @robobun, your commit 82944b6efcb51357075f8b299251914b32a008b5 passed in 🧪 To try this PR locally: bunx bun-pr 34818That installs a local version of the PR into your bun-34818 --bun |
WalkthroughUpdated float typed-array deep equality to handle ChangesFloat typed-array equality
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/34815.test.ts`:
- Around line 5-6: Remove the explanatory comments near the typed-array
assertions in the regression test, including both comment blocks around lines
5-6 and 32-33, while retaining only the GitHub issue URL comment.
🪄 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: fde7d60e-c9fb-4608-a329-901501e392f6
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/regression/issue/34815.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Confirmed the overlap. #32872 fixes the same float typed array class (same approach: Object.is semantics on the jest path, gated on the asymmetric-matchers template parameter) as part of a broader change covering four deepEquals classes for #29030, and #34434 overlaps both as part of node:util v26 compat work. This PR is the minimal fix for the regression reported in #34815 (toEqual on float typed arrays with NaN, broken since v1.2.11), with a regression test that also pins the node loose/strict semantics. If #32872 or #34434 lands first, this one can be closed; if this lands first, the float hunk in #32872 becomes a trivial rebase. Leaving the choice of merge order to maintainers. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that branches the non-strict float typed-array comparison on the jest template param so toEqual gets Object.is semantics while node's loose deepEqual/Bun.deepEquals keep IEEE ==.
What was reviewed:
- Verified
enableAsymmetricMatcherscorrectly discriminates jest matchers (jestDeepEquals,<false, true>) fromBun.deepEquals/assert.deepEqual(<false, false>) — bindings.cpp:2750-2768. - Traced the ternary against Object.is truth table (NaN/NaN, ±0, mixed) — correct;
signbitcheck is only reachable whenl == r, so non-zero equal values can't spuriously differ. - Confirmed strict path (memcmp at line 1415) is untouched, so
toStrictEqual/deepStrictEqualbehavior is unchanged; test pins that too.
Extended reasoning...
Overview
Single-function change in specialObjectsDequal (src/jsc/bindings/bindings.cpp) inside the typed-array case. The three copy-pasted per-width float loops are collapsed into one generic lambda that widens each element to double and, when enableAsymmetricMatchers is true (jest matchers), applies Object.is semantics (l == r ? signbit(l) != signbit(r) : !(isnan(l) && isnan(r))). The non-jest path keeps the existing IEEE !=. A regression test covers Float16/32/64 for NaN and ±0 under toEqual, toStrictEqual, assert.deepEqual, assert.deepStrictEqual, and Bun.deepEquals.
Security risks
None. Pure numeric comparison logic in the test-runner deep-equals path; no untrusted input parsing, allocation, or side effects.
Level of scrutiny
Low-medium. The change is confined to a !isStrict branch that already existed and only alters element-wise comparison semantics for one of the two template instantiations. I confirmed via the export table at bindings.cpp:2750-2768 that enableAsymmetricMatchers=true maps exactly to jestDeepEquals/jestStrictDeepEquals (and toEqual uses the former via the test_runner crate), while Bun.deepEquals and node assert use false. Widening WTF::Float16/float to double preserves NaN-ness and sign, so the semantics carry through; sizeof(*left) on WTF::Float16 is 2, matching the previous sizeof(WTF::Float16) divisor.
Other factors
- The refactor also deduplicates three near-identical loops, which is the kind of within-diff dedup REVIEW.md asks for.
- Tests are appropriately placed in
test/regression/issue/34815.test.tswith the issue URL, cover all three float widths and both directions (equal/not-equal), and pin the unchanged node/Bun.deepEquals semantics so a future edit that accidentally flips the non-jest path will fail. - The PR notes a deliberate secondary behavior change (
toEqualnow distinguishes ±0 in float arrays) and correctly attributes it to Jest parity — this follows directly from Object.is and is covered by a test. - Existing suites (
expect.test.js,deep-equals.spec.ts,test-assert-typedarray-deepequal.js) reported passing; the change surface is small enough that those are the right regression net.
There was a problem hiding this comment.
LGTM — focused fix that branches float typed-array element comparison on the jest template parameter, giving toEqual Object.is semantics while leaving Bun.deepEquals/node loose deepEqual and strict memcmp paths untouched.
What was reviewed:
- Object.is predicate (
l == r ? signbit differs : !(both NaN)) — correct for NaN-equals-NaN and +0≠-0; widening Float16/float to double preserves NaN and signed-zero. - Template-parameter routing:
jestDeepEquals=<false, true>hits the new branch;deepEquals=<false, false>keeps IEEE!=; both strict variants still fall through to memcmp. - Regression test pins all four contracts (toEqual/toStrictEqual/assert.deepEqual/deepStrictEqual + Bun.deepEquals) across Float16/32/64.
Note: overlaps with #32872/#34434 as already flagged in-thread — merge order is a maintainer call, but this change is self-contained and rebases trivially either way.
Extended reasoning...
Overview
The PR fixes #34815: expect(new Float16Array([NaN])).toEqual(new Float16Array([NaN])) was failing because the non-strict float typed-array path in specialObjectsDequal (bindings.cpp) compared elements with IEEE !=, under which NaN never equals NaN. The fix collapses the three per-width loops into a single generic lambda that widens each element to double and, when enableAsymmetricMatchers is set (the jest path), applies Object.is semantics per element; the node/Bun.deepEquals path keeps the original IEEE comparison. A new <cmath> include supplies std::signbit/std::isnan. Net -7 lines in bindings.cpp plus a 43-line regression test.
Security risks
None. This is a pure comparison-semantics change in the test-matcher deep-equality helper; no untrusted input parsing, allocation sizing, or FFI ownership is touched.
Level of scrutiny
Moderate. Bun__deepEquals backs four user-facing contracts (jest toEqual/toStrictEqual, node assert.deepEqual/deepStrictEqual, and Bun.deepEquals), so I traced the four deepEqualsWrapperImpl<...> instantiations at bindings.cpp:2752-2767 to confirm only <false, true> (jest non-strict) reaches the new Object.is branch, <false, false> still gets IEEE !=, and both <true, *> variants skip the block entirely and hit memcmp. I also walked the Object.is predicate: when l == r under IEEE, signbits can only differ for ±0, so the signbit check is exactly the +0/-0 case; when l != r, only both-NaN survives as equal. Widening Float16/float to double is exact and preserves NaN and signed zero, so the else branch is behavior-identical to the old per-type !=.
Other factors
The regression test covers all three float widths, NaN in isolation and mid-array, +0 vs -0 both directions, toStrictEqual, node assert.deepEqual/deepStrictEqual, and Bun.deepEquals in both modes — so it pins the unchanged paths as well as the fixed one. The PR evidence shows the test failing on the debug/ASAN build without the fix and passing with it (the "release without fix" block in the gate output shows the same commit hash as the fix, which looks like a harness labeling artifact rather than a real pass-without-fix). The author also re-ran expect.test.js, deep-equals.spec.ts, and test-assert-typedarray-deepequal.js clean. The CodeRabbit comment about test comments was resolved. The known overlap with #32872/#34434 is a coordination question the author already surfaced; it doesn't affect correctness of this change.
1 participant
Fixes #34815
Repro
Float32Array and Float64Array were affected the same way.
Cause
Since #19285 (v1.2.11), the non-strict deep-equals path compares float typed array elements with IEEE
!=, so NaN never equals NaN and +0 equals -0. That matches node's looseassert.deepEqual, but the same code also runs for Jest'stoEqual, which compares elements withObject.issemantics (NaN equals NaN, +0 differs from -0).Fix
In
Bun__deepEquals, the float element loop now branches on the jest template parameter: the jest path usesObject.issemantics per element, while the node/Bun.deepEqualspath keeps the existing IEEE comparison. Strict mode (toStrictEqual,deepStrictEqual) is unchanged (byte-wise memcmp).Verification
test/regression/issue/34815.test.tsfails on the released build, passes with the fix; it also pins the node loose/strict semantics as unchanged.test/js/bun/test/expect.test.js(407 pass),test/js/bun/bun-object/deep-equals.spec.ts(44 pass), andtest/js/node/test/parallel/test-assert-typedarray-deepequal.js(40 pass) all pass locally.Note this also aligns
toEqualwith Jest fornew Float64Array([-0])vsnew Float64Array([0]): they are now unequal, as in Jest.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The root cause was that jest's
toEqualon float typed arrays compared elements with plain IEEE!=, under whichNaN != NaN, so two arrays containing NaN were reported as unequal. The fix branches the non-strict float typed-array comparison inspecialObjectsDequalon theenableAsymmetricMatcherstemplate parameter, applying Object.is semantics (NaN equals NaN, and +0 is distinguished from -0) when the comparison comes from jest matchers. Node'sassert.deepEqualandBun.deepEqualsretain the original IEEE comparison, so their behavior is unchanged.