🐛 Make float/double/typed-array NaN generation non-canonical - #7136
🐛 Make float/double/typed-array NaN generation non-canonical#7136vishnujayvel wants to merge 2 commits into
Conversation
fc.float(), fc.double(), fc.float32Array() and fc.float64Array() always hardcoded Number.NaN as their "extra" NaN value. Since all NaN bit patterns are indistinguishable at the JS language level (Number.isNaN, ===, Object.is...), this meant these arbitraries could never surface the non-canonical NaN bit patterns (signaling NaNs, alternate payloads, negative NaNs...) that only become observable once a value is copied into a Float32Array/Float64Array - exactly the scenario described in dubzzz#6532. Root cause and fix approach per cschanhniem's analysis on the issue: reuse the single index that anyFloat/anyDouble already dedicate to NaN, but map it to a fixed non-canonical bit pattern instead of Number.NaN, rather than drawing extra randomness for a NaN "variant" index. The latter was tried first and reliably destabilized float32Array/float64Array's "shrink without initial context" contract (ArrayArbitrary drops per-element context, and IntegerArbitrary's context-free shrink target depends on the exact index range), so it was discarded in favor of this zero-extra-entropy design. Two already-quiet, non-canonical 32/64-bit patterns are used (one for ranges extending past +0, one for ranges below it), avoiding signaling patterns since some engines quiet them when written into a TypedArray. Adds regression coverage for all four arbitraries and confirms the existing NoRegression snapshot suite is unaffected (same index topology, only the sentinel's value changed).
🦋 Changeset detectedLatest commit: 52480c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Actually so far we don't want to address that issue. Thanks for the suggestion but we might close it until we make it something we really want |
@fast-check/ava
fast-check
@fast-check/jest
@fast-check/packaged
@fast-check/poisoning
@fast-check/vitest
@fast-check/worker
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7136 +/- ##
==========================================
+ Coverage 94.89% 94.90% +0.01%
==========================================
Files 212 212
Lines 5951 5966 +15
Branches 1565 1566 +1
==========================================
+ Hits 5647 5662 +15
Misses 296 296
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for taking a look, and totally understand — no worries if it's not something you want to take on right now. Happy to close it, or leave it open as a reference for later, whatever's easiest for you. Appreciate the time either way. 🙏 |
|
I'm gonna look to it right before opening the 5.x version. It might be a good fit for 5.x as it may break some users with the new behaviour. That said the new behaviour is the one most of people should expect so a minor would be fine too. |
|
Thanks for following up on it. Please keep me posted if you want me to rebase and publish it for new release in the future! |
Description
Fixes #6532
fc.float(),fc.double(),fc.float32Array()andfc.float64Array()always hardcodedNumber.NaNas their "extra" NaN value. Since all NaN values are indistinguishable from each other at the JS language level (Number.isNaN,===,Object.is...), these arbitraries could never surface the non-canonical NaN bit patterns (signaling NaNs, alternate payloads, negative NaNs...) that only become observable once a value is copied into aFloat32Array/Float64Array— exactly the scenario @cschanhniem described in the issue.Root cause and fix approach per @cschanhniem's analysis on the issue: reuse the single index that
anyFloat/anyDoublealready dedicate to NaN, but map it to a fixed non-canonical bit pattern instead ofNumber.NaN, rather than drawing extra randomness for a NaN "variant" index. The latter was tried first and reliably destabilizedfloat32Array/float64Array's "shrink without initial context" contract (ArrayArbitrarydrops per-element context, andIntegerArbitrary's context-free shrink target depends on the exact index range), so it was discarded in favor of this zero-extra-entropy design.Two already-quiet, non-canonical 32/64-bit patterns are used (one for ranges extending past +0, one for ranges below it), avoiding signaling patterns since some engines quiet them when written into a TypedArray.
Impact: patch — no public API changes, only the underlying bit pattern of an already-produced NaN sentinel value changes (which is unobservable except via raw byte inspection, as described above).
Checklist
pnpm run bumpor by following the instructions from the changeset bot🐛(vitest) Something...) when the change targets a package other thanfast-checkTest plan
Ran locally on a fresh fork/clone against current
main:I also confirmed the 4 new regression tests actually catch the bug: reverting only
src/arbitrary/double.ts,src/arbitrary/float.ts, and their_internals/helpers/{Double,Float}Helpers.tsback to the pre-fixNumber.NaNhardcoding (while keeping the new tests) makes exactly those 4 tests fail, confirming they would have failed without this PR:float32Array.spec.ts > should be able to produce non-canonical NaN bit patterns (see #6532)float64Array.spec.ts > should be able to produce non-canonical NaN bit patterns (see #6532)float.spec.ts > should properly convert the extra value to NaNdouble.spec.ts > should properly convert the extra value to NaNThe
NoRegressione2e snapshot suite passes unchanged, confirming the index topology (and thus shrink behavior) is unaffected — only the sentinel's underlying bit pattern changed.AI-assistance disclosure
In keeping with this project's contributing guidance: this contribution was prepared with AI assistance and was reviewed, run, and approved by me before submission — I understand the change and take responsibility for it.
What it does, in my words: the float/double/typed-array generators only ever emitted the canonical NaN bit pattern, so property tests could never exercise non-canonical NaNs (#6532). This maps the existing single NaN sentinel index to a fixed non-canonical bit pattern with zero additional entropy, so the value-index topology — and therefore all shrinking behaviour and the
NoRegressionsnapshots — stays identical; only the sentinel's underlying bits change.What I verified myself: the full test-plan above (229/229 targeted + NoRegression, typecheck/lint/format clean), plus the revert-confirms-fail check on the 4 new tests. Root-cause credit to @cschanhniem.
Happy to adjust anything to fit your preferences.