Fix Array index handling - #6878
Conversation
🦋 Changeset detectedLatest commit: 878bc0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Array NaN index reproduction tests— 6 tests coveringget,getUnsafe,insertAt,replace,modify, andremovewithNumber.NaNas the index argument, asserting that NaN is treated as an invalid/out-of-bounds index per the documented contract.
Each assertion correctly fails against the current implementation — the tests accurately reproduce the core-a-f-array-nan-index-validation bug. The root cause is that isOutOfBounds and insertAt's inline bounds check both rely on i < 0 || i >= N comparisons, which both return false for NaN.
The test file follows @effect/vitest conventions (assert.deepStrictEqual, synchronous it blocks) and imports match the style used throughout the existing test suite.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
df93300 to
878bc0e
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the new commit adds the implementation fix for the Array NaN index bug plus fractional-index flooring.
isOutOfBoundsnow rejects non-finite indexes — the NaN bypass is closed by prepending!Number.isFinite(i) ||to the existing< 0 || >= lengthchecks. This also coversInfinityand-Infinity.insertAt,modify, andremovenow floor the index —Math.floor(i)is applied before bounds checking andsplice/array access, so fractional indexes (e.g.1.5) resolve to integer positions consistently.insertAtbounds check aligned withisOutOfBounds— the old inlinei < 0 || i > out.lengthis replaced withindex !== out.length && isOutOfBounds(index, out), which still permits end-of-array insertion.get,getUnsafe, andreplace— already floored or delegated tomodify, so they benefit from theisOutOfBoundsfix without additional changes.- Tests — the existing 6 NaN reproduction tests now pass, and 2 new fractional-index tests cover
replace(1.5)andmodify(1.5).
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Reproduction only
This PR adds reproduction tests only. No implementation fix is included. CI is expected to fail until the underlying issue is fixed.
Covered audit issues
1.
core-a-f-array-nan-index-validation: NaN bypasses shared index validationModule:
ArrayExpected contract: get safely returns None for an invalid index, getUnsafe throws for one, insertAt, replace, and modify report invalid indexes with None, and remove treats an invalid index as an out-of-bounds no-op. The documented valid insertion range is the integer indexes from 0 through length.
Observed result: All 6 focused assertions failed for the reported values and coercions.
Reproduction command: