Preserve integral precision when parsing decimal Duration inputs - #6945
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 7d87dad 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.
Important
The regression test is a good spec, but the implementation fix still needs to land on this branch before the PR can merge.
Reviewed changes
I reviewed the single regression test added in packages/effect/test/DurationParsePrecision.test.ts and ran it against the current main-based implementation. As expected, the test fails because parseNanos in packages/effect/src/Duration.ts converts the whole decimal string through Number before scaling, which corrupts integral digits above Number.MAX_SAFE_INTEGER.
The reproduction command in the PR body confirmed the failure, and the existing Duration.test.ts suite still passes (no source changes yet).
⚠️ Implementation fix is missing
The diff only adds the failing test; packages/effect/src/Duration.ts:41-42 still uses roundTiesAwayFromZero(Number(input) * Number(scale)) for decimal nano/micro strings. Merging this branch as-is will keep CI red.
Technical details
# Missing `parseNanos` precision fix
## Affected sites
- `packages/effect/src/Duration.ts:36-42` — `roundTiesAwayFromZero` and `parseNanos`
- `packages/effect/src/Duration.ts:244-249` — nano/micro parsing call sites
- `packages/effect/test/DurationParsePrecision.test.ts:7-10` — correct regression assertion
## Required outcome
- Decimal nano and micro strings must be parsed into bigint nanoseconds without first coercing the full string through `Number`.
- Rounding rules (ties away from zero) should still apply to the final nanosecond value.
- The existing `Duration.test.ts` assertions for `1.5 nanos`, `1.5 micros`, etc., must continue to pass.
## Suggested approach (optional)
Split `valueStr` on `.`, scale the integral part with `BigInt` arithmetic, scale the fractional part (right-padded/truncated to the unit's scale), and add them before rounding. For example, for micros scale `1_000` and fractional string `1`, compute `(BigInt(int) * 1_000n) + BigInt(frac.padEnd(3, "0").slice(0, 3))`, then round. Handle the sign separately so negative inputs round away from zero correctly.@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review, the branch landed the missing parseNanos implementation fix and consolidated the regression coverage into the main Duration test suite.
- Rewrote
parseNanoswith bigint arithmetic — Decimal nano/micro strings are now split on., scaled exactly withBigInt, and rounded ties-away-from-zero without first coercing throughNumber. - Added constants —
bigint2andbigint10support the new rounding logic. - Expanded regression coverage — Large positive nano/micro values and negative tie/rounding cases were added to
packages/effect/test/Duration.test.ts; the standaloneDurationParsePrecision.test.tsfrom the first commit was removed.
The focused test command and the effect package typecheck both pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Fractional nano and micro strings above Number.MAX_SAFE_INTEGER can change integral digits before nanosecond rounding.
Important
This PR includes the bigint-safe parsing fix and regression coverage in the main Duration test suite.
Decimal nano parsing corrupts integral precision
Module:
DurationAudit ID:
core-a-f-duration-decimal-precisionSeverity / confidence: medium / high
What happens
Fractional nano and micro strings above Number.MAX_SAFE_INTEGER can change integral digits before nanosecond rounding.
Why it happens
parseNanos converts the complete decimal and scale through Number before rounding to bigint, losing precision that remains available in the original string.
Expected behavior
Decimal duration strings are parsed into the nanosecond-backed representation and rounded to integer nanoseconds without corrupting the integral component.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/Duration.ts:36-42packages/effect/src/Duration.ts:241-249View problematic code at
packages/effect/src/Duration.ts:36-42View exact lines on GitHub
View problematic code at
packages/effect/src/Duration.ts:241-249View exact lines on GitHub
Reproduction
Before the fix, parsing
9007199254740993.1 nanosreturned9007199254740994ninstead of9007199254740993n.Regression coverage now lives in the main Duration test suite:
pnpm test --run packages/effect/test/Duration.test.tsImplementation
Decimal nano and micro inputs are scaled and rounded with exact bigint arithmetic, preserving integral digits and ties-away-from-zero behavior. The regression coverage includes large positive values and negative tie cases for both units.
Audit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715fcore-a-f-duration-decimal-precisionCloses EFF-391