fix(intl): #5581 — NumberFormat numbering systems, roundingIncrement, zero-significant hang#5737
Merged
Merged
Conversation
…n + roundingIncrement Two spec-grounded gaps in `Intl.NumberFormat` rendering, both surfaced by the intl402/NumberFormat test262 cluster (#5581): 1. **Numbering systems.** Output digits were always Latin (`latn`); a formatter resolved to a non-Latin system (`-u-nu-arab`, the `numberingSystem` option, etc.) still emitted ASCII digits. Added `numbering_system_digits` (the full 77-system glyph table, generated from test262's `numberingSystemDigits`) and a `transliterate_parts_digits` pass that rewrites the digit glyphs of the `integer`/`fraction`/`exponentInteger` segments — separators, signs, and currency/unit/compact literals keep their locale glyphs. Applied in the shared `number_parts_from_resolved` core so `format`, `formatToParts`, and the DurationFormat consumer all benefit; `latn`/unknown is a no-op. 2. **roundingIncrement.** The option was validated and surfaced in resolvedOptions but never applied during rendering. Added `round_to_increment` (rounds the scaled integer to the nearest multiple of the increment, breaking ties via the shared `round_decision` core) and wired it into the standard notation branch. Refactored the ApplyUnsignedRoundingMode logic out of `rounding_up` into `round_decision` so fraction- and increment-rounding share one mode implementation. Flips 21 intl402/NumberFormat test262 cases (all 14 roundingIncrement tests, numbering-systems.js, and the fraction/significant cases that were gated on the non-Latin digit assertion) with zero regressions across the tree. Partial progress on #5581; the remaining NumberFormat clusters (units, accounting currency, compact-locale tables, formatRange) and the #5580/#5582 Temporal/ DateTimeFormat clusters are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 49 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors rounding behavior, adds non-unit increment rounding, pads zero significant-digit output, and transliterates digit parts for resolved numbering systems. ChangesIntl.NumberFormat rounding and digit rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@crates/perry-runtime/src/intl/number_format.rs`:
- Around line 799-807: The roundingIncrement handling in number_parts_core is
only applied in the standard decimal branch, so configs like nearest 0.05 are
skipped by the currency and scientific/engineering/compact paths. Update the
rounding flow in number_parts_core and the downstream helpers it dispatches to
so rounding_increment is honored before those branches format the number, using
the existing round_to_increment logic (or an equivalent shared step) rather than
only the standard decimal path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f334ba62-8dce-4682-9396-92b78d2450d1
📒 Files selected for processing (1)
crates/perry-runtime/src/intl/number_format.rs
…infinite-looped
`round_to_significant` rendered a zero value as `("0", "")` and then ran the
min-significant-digits padding loop `while significant_count(int, frac) < min_sig
{ frac.push('0') }`. But `significant_count` of an all-zeros decimal is always 0,
so for any value of `0` formatted with significant digits (or compact notation,
which rounds by significant digits) the loop never terminated — `format(0)` hung.
The hang was latent (every locale, not numbering-specific); the preceding
numbering-system commit merely unblocked the test262 rounding-mode cases far
enough past the digit-set gate to reach `format(0)` and expose it.
Render zero as a single "0" padded to `min_sig` total displayed digits
(ECMA-402 ToRawPrecision: minSig 3 → "0.00") and return early, before the
nonzero-only normalization loops.
Flips the 9 intl402/NumberFormat `format-rounding-mode-*` cases plus
format-significant-digits.js (+10); no regressions across NumberFormat /
DurationFormat / PluralRules.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
changed the title
…pact paths (CodeRabbit) Addresses CodeRabbit review on #5737: `roundingIncrement` was only applied in the standard decimal branch, so currency, scientific/engineering, and the fraction-mode compact path still ignored it (e.g. nearest-0.05 configs). Per ECMA-402 roundingIncrement is gated on fixed fraction-digit rounding, not on notation, and Node applies it uniformly — to the currency value, the scientific *mantissa*, and the compact value, all at maxFrac places. Factored the fraction-rounding step into a shared `round_fraction_or_increment` helper and routed the standard, scientific/engineering, and compact branches through it; the currency path (a separate native-float renderer) snaps its magnitude onto the increment grid up front via the same digit-string `round_to_increment`, respecting roundingMode. With `roundingIncrement == 1` (the overwhelmingly common case) the helper is byte-identical to the previous `round_to_fraction` + `trim_fraction`, so existing output is unchanged. Verified perry now matches Node byte-for-byte for currency (nearest 0.05), scientific (mantissa rounding), and compact increment configs. NumberFormat test262 unchanged at 158 pass / identical failure set (these notation+increment combos have no test262 coverage); DurationFormat (92) and PluralRules (39) unchanged — zero regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@crates/perry-runtime/src/intl/number_format.rs`:
- Around line 901-925: The increment-snapping path in number_format.rs is using
the currency default fraction count instead of the resolved fraction width, so
rounding grids can be wrong when minimumFractionDigits/maximumFractionDigits are
customized. Update the rounding logic around the existing frac_digits/value
block to derive the snap scale from the resolved fraction width used by
format_number_parts, and feed that into round_to_increment instead of
currency_fraction_digits. Keep the behavior inside the rounding_increment branch
and preserve the existing rounding_mode handling in set_round_ctx.
- Around line 755-758: In the scientific/engineering formatting path in
number_format.rs, the exponent is computed before rounding, but
round_to_significant and round_fraction_or_increment can still grow the mantissa
into an extra digit (for example 9.9 becoming 10). Update the formatting logic
around this rounding result so that after calling those helpers you detect
mantissa overflow and renormalize the exponent accordingly before emitting the
final scientific string, ensuring cases like 10E0 become 1E1.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c3405b9-c013-43e3-b158-03ed480c3a9d
📒 Files selected for processing (1)
crates/perry-runtime/src/intl/number_format.rs
Reflow the generated numbering-system digit table and round_to_increment to satisfy cargo fmt --check (lint gate). Whitespace only — no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@crates/perry-runtime/src/intl/number_format.rs`:
- Around line 252-256: The overflow path in `round_to_increment` is dropping
`roundingIncrement` when `combined[..cut].parse::<u128>()` fails for large
`maximumFractionDigits` values. Update the fallback handling around the `let
Ok(q) = ... else` branch so it stays increment-aware instead of returning
`fallback()`/`round_to_fraction`; use the existing decimal-string data
(`combined`, `cut`, and `increment`) to classify the remainder and adjust the
value without relying on `u128` parsing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a376620-ffbe-4df3-96af-bf73722c10e0
📒 Files selected for processing (1)
crates/perry-runtime/src/intl/number_format.rs
…rency scale, mantissa carry) Three correctness findings from CodeRabbit on #5737, each verified byte-for-byte against Node: 1. roundingIncrement on wide fraction scales / large values. `round_to_increment` parsed the scaled integer as `u128`, so `maximumFractionDigits` near its 100 ceiling (or very large values) overflowed and silently fell back to plain fraction rounding, dropping the increment. The scaled integer is now kept as a digit string and reduced with small-divisor modular arithmetic (decimal_mod_small / decimal_add_small / decimal_sub_small); the increment (≤ 5000) and the dropped tail (bounded by the shortest decimal) stay small. Parity for halfEven is read off `q mod 2·increment`, avoiding any wide division. 2. Currency increment grid used the default scale. `currency_instance_parts` rounded/snapped on the currency's default fraction digits, ignoring minimum/maximumFractionDigits — so e.g. 3-digit USD snapped on 0.05 instead of 0.005. It now uses the resolved width `r.max_frac` (which already folds in the currency default plus options) for both the increment grid and the display, so grid and precision agree (1.234 → $1.235). Identical to before for the default-width common case. 3. Scientific/engineering mantissa carry. The exponent was computed before rounding, so a carry (9.9 → 10) emitted `10E0` instead of `1E1`. After rounding, a grown mantissa now renormalizes: recompute the exponent from msd+1 and reshape the significand — scientific `1E1`, while engineering keeps the digit when the new magnitude stays in the same power-of-1000 band (`9.9 → 10E0`, `999.9 → 1E3`, `99999 → 100E3`). NumberFormat test262 unchanged at 158 pass / identical failure set (these cases have no test262 coverage in the failing set); DurationFormat (92) and PluralRules (39) unchanged — zero regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three spec-grounded fixes to
Intl.NumberFormatrendering, surfaced by the #5581 test262 cluster (intl402/NumberFormat). 31 test262 cases flip to pass, with zero regressions across the NumberFormat / DurationFormat / PluralRules trees.1. Numbering-system digit transliteration
Output digits were always Latin (
latn). A formatter resolved to a non-Latin numbering system — via-u-nu-arabin the locale or thenumberingSystemoption — still emitted ASCII digits, failing the digit-set gate in test262'stestNumberFormathelper.numbering_system_digits(name)returns the 10 glyphs for a system (the full 77-system table, generated from test262's ownnumberingSystemDigitsso it stays authoritative).transliterate_parts_digitsrewrites only the digit-bearing segments (integer/fraction/exponentInteger); separators, signs, and currency/unit/compact literals keep their locale glyphs.number_parts_from_resolvedcore, soformat,formatToParts, and theIntl.DurationFormatconsumer all pick it up.latn/ unknown systems are a no-op.2. roundingIncrement
The option was validated and surfaced in
resolvedOptionsbut never applied during rendering, so everyroundingIncrement≠ 1 case formatted as if it were 1.round_to_incrementscales the value to an integer and rounds to the nearest multiple of the increment, breaking ties (including the fractional tail) via the shared decision core.rounding_upintoround_decision, so fraction- and increment-rounding share one mode implementation (no behavior change to the existing fraction path).3. Significant-digit formatting of zero infinite-looped
round_to_significant(0, …)produced("0", "")then ranwhile significant_count(int, frac) < min_sig { frac.push('0') }— butsignificant_countof all-zeros is always 0, soformat(0)hung forever under significant-digit or compact notation (every locale, not numbering-specific). Now zero renders as a single"0"padded tomin_sigtotal digits (ECMA-402 ToRawPrecision: minSig 3 →"0.00") and returns early. This latent hang was only reached once fix #1 cleared the digit-set gate in the rounding-mode cases.Verification
scripts/test262_subset.py --dir intl402/NumberFormat(Node v26.3 oracle):Flipped (31): all 14
format-rounding-increment-*, all 9format-rounding-mode-*,numbering-systems.js,format-significant-digits*, and the fraction cases that were gated on the non-Latin digit assertion. Diffed the full tree against the issue's failure list and against the pre-change run — 0 regressions. DurationFormat (92 pass) and PluralRules (39 pass) unchanged.Scope
Partial progress on #5581. The remaining NumberFormat clusters (units, accounting-currency sign, compact-locale tables,
formatRange/formatRangeToParts) and the broader #5580 (TemporaltoLocaleString) / #5582 (DateTimeFormat) clusters are deeper, separate efforts and are not addressed here.Per maintainer convention, no version bump / CHANGELOG entry — left for merge time.
🤖 Generated with Claude Code
Summary by CodeRabbit
roundingIncrementvalues other than1.0, including fixed-fraction increment rounding for general formatting and currency rendering.0with the expected padded fraction length.