fix(intl): #5581 — NumberFormat accounting sign, unit validation, NaN locale, de-DE USD position - #5783
Merged
Merged
Conversation
… locale, de-DE USD position
Four root causes addressed, targeting ~30 of the 71 test262 intl402/NumberFormat failures:
1. **accounting currencySign** (`signDisplay-currency-*`, `signDisplay-negative-currency-*`):
`currency_instance_parts()` ignored `r.currency_sign == "accounting"` — negative values
were formatted as `$-987.00` instead of `($987.00)`. Fixed by passing the absolute value
to the numeric renderer and wrapping the assembled parts in `("literal","(")` / `("literal",")")`
when `accounting && is_negative && signDisplay != "never"`.
2. **Locale-specific USD symbol** (`signDisplay-currency-ko-KR/zh-TW`):
USD was always rendered as `"$"`. Korean (ko) and Chinese (zh) locales require `"US$"` per
CLDR. Added `usd_symbol(locale)` helper.
3. **Sanctioned unit identifier validation** (`constructor-unit.js`, `units-invalid.js`):
`is_well_formed_unit_identifier` accepted any ASCII-alphabetic hyphen-separated sequence.
Replaced with ECMA-402 Table 2 sanctioned-unit list check: simple unit OR
`<sanctioned>-per-<sanctioned>` compound. Rejects `"test"` and `"acre-foot"` correctly.
4. **roundingMode Symbol TypeError** (`constructor-options-roundingMode-invalid.js`):
`get_string_option_enum` (uses `coerce_option_string`) silently coerced Symbol values;
ECMA-402 requires a TypeError. Switched to `enum_option_strict` which calls
`get_option_string_coerced` → throws TypeError for Symbol.
5. **de-DE USD symbol position** (`signDisplay-currency-de-DE.js`):
USD was placed before the number (`$-987,00`) for de-DE locale. Added a `de_style`
guard for USD matching the existing EUR guard, placing it after with NBSP (`-987,00 $`).
6. **zh-TW NaN localization** (`signDisplay-zh-TW.js`, `engineering-scientific-zh-TW.js`):
NaN was hardcoded as `"NaN"`. zh-TW expects `"非數值"`. Added `nan_string(locale)` helper.
📝 WalkthroughWalkthroughTwo files in the ChangesNumberFormat locale and validation fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.github.com/in/347564?s=60&v=4){kind=small}
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 1580-1582: The currency formatting path still sends NaN through
format_number_parts, so it bypasses the locale-specific nan_string handling.
Update currency_instance_parts to detect NaN before building numeric parts and
use the same localized NaN branch used by the non-currency formatter, preserving
the existing split_numeric_parts flow for normal numbers. Keep the fix centered
around currency_instance_parts and the number formatting helpers it already
calls.
- Around line 1612-1617: The accounting branch in number formatting currently
wraps every originally negative value unless signDisplay is "never", which
breaks the rounded-zero behavior already handled in number_parts_core. Update
the logic around the accounting sign handling in number_parts_core so
parentheses are only applied when the post-rounding sign still qualifies for the
active sign_display, preserving the existing suppress-sign behavior for
exceptZero and negative when the rounded result is zero.
🪄 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: 5b432c3c-9168-462d-ad6f-c626ab099f4c
📒 Files selected for processing (2)
crates/perry-runtime/src/intl/number_format.rscrates/perry-runtime/src/intl/number_format_options.rs
Comment on lines
+1580
to
1582
| let digits = format_number_parts(format_value, locale, Some(frac_digits), None); | ||
| let mut numeric: Vec<(&'static str, String)> = Vec::new(); | ||
| split_numeric_parts(&digits, locale, &mut numeric); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localize NaN in the currency path too.
currency_instance_parts still routes NaN through format_number_parts, which returns literal "NaN", so zh-TW currency formatting bypasses the new nan_string behavior.
Proposed fix
- let digits = format_number_parts(format_value, locale, Some(frac_digits), None);
let mut numeric: Vec<(&'static str, String)> = Vec::new();
- split_numeric_parts(&digits, locale, &mut numeric);
+ if format_value.is_nan() {
+ numeric.push(("nan", nan_string(locale).to_string()));
+ } else {
+ let digits = format_number_parts(format_value, locale, Some(frac_digits), None);
+ split_numeric_parts(&digits, locale, &mut numeric);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let digits = format_number_parts(format_value, locale, Some(frac_digits), None); | |
| let mut numeric: Vec<(&'static str, String)> = Vec::new(); | |
| split_numeric_parts(&digits, locale, &mut numeric); | |
| let mut numeric: Vec<(&'static str, String)> = Vec::new(); | |
| if format_value.is_nan() { | |
| numeric.push(("nan", nan_string(locale).to_string())); | |
| } else { | |
| let digits = format_number_parts(format_value, locale, Some(frac_digits), None); | |
| split_numeric_parts(&digits, locale, &mut numeric); | |
| } |
🤖 Prompt for 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.
In `@crates/perry-runtime/src/intl/number_format.rs` around lines 1580 - 1582, The
currency formatting path still sends NaN through format_number_parts, so it
bypasses the locale-specific nan_string handling. Update currency_instance_parts
to detect NaN before building numeric parts and use the same localized NaN
branch used by the non-currency formatter, preserving the existing
split_numeric_parts flow for normal numbers. Keep the fix centered around
currency_instance_parts and the number formatting helpers it already calls.
Comment on lines
+1612
to
+1617
| // Accounting sign: negative amounts are wrapped in parentheses (no minus sign). | ||
| // Only applied when signDisplay is not "never". | ||
| if accounting && is_negative && r.sign_display != "never" { | ||
| parts.insert(0, ("literal", "(".to_string())); | ||
| parts.push(("literal", ")".to_string())); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve rounded-zero signDisplay semantics for accounting.
Line 1614 wraps every originally negative value except signDisplay: "never". That regresses the existing rounded-zero behavior in number_parts_core, where exceptZero and negative suppress the sign after rounding to zero.
Proposed fix
split_numeric_parts(&digits, locale, &mut numeric);
let de_style = locale.eq_ignore_ascii_case("de") || locale.starts_with("de-");
+ let has_digits = numeric
+ .iter()
+ .any(|(t, _)| *t == "integer" || *t == "fraction");
+ let rounded_is_zero = has_digits
+ && numeric
+ .iter()
+ .filter(|(t, _)| *t == "integer" || *t == "fraction")
+ .all(|(_, v)| v.bytes().all(|b| b == b'0'));
let mut parts: Vec<(&'static str, String)> = Vec::new();
@@
- if accounting && is_negative && r.sign_display != "never" {
+ let show_accounting_sign = match r.sign_display.as_str() {
+ "never" => false,
+ "exceptZero" | "negative" => is_negative && !rounded_is_zero,
+ _ => is_negative,
+ };
+ if accounting && show_accounting_sign {
parts.insert(0, ("literal", "(".to_string()));
parts.push(("literal", ")".to_string()));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Accounting sign: negative amounts are wrapped in parentheses (no minus sign). | |
| // Only applied when signDisplay is not "never". | |
| if accounting && is_negative && r.sign_display != "never" { | |
| parts.insert(0, ("literal", "(".to_string())); | |
| parts.push(("literal", ")".to_string())); | |
| } | |
| split_numeric_parts(&digits, locale, &mut numeric); | |
| let de_style = locale.eq_ignore_ascii_case("de") || locale.starts_with("de-"); | |
| let has_digits = numeric | |
| .iter() | |
| .any(|(t, _)| *t == "integer" || *t == "fraction"); | |
| let rounded_is_zero = has_digits | |
| && numeric | |
| .iter() | |
| .filter(|(t, _)| *t == "integer" || *t == "fraction") | |
| .all(|(_, v)| v.bytes().all(|b| b == b'0')); | |
| let mut parts: Vec<(&'static str, String)> = Vec::new(); | |
| // Accounting sign: negative amounts are wrapped in parentheses (no minus sign). | |
| // Only applied when signDisplay is not "never". | |
| let show_accounting_sign = match r.sign_display.as_str() { | |
| "never" => false, | |
| "exceptZero" | "negative" => is_negative && !rounded_is_zero, | |
| _ => is_negative, | |
| }; | |
| if accounting && show_accounting_sign { | |
| parts.insert(0, ("literal", "(".to_string())); | |
| parts.push(("literal", ")".to_string())); | |
| } |
🤖 Prompt for 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.
In `@crates/perry-runtime/src/intl/number_format.rs` around lines 1612 - 1617, The
accounting branch in number formatting currently wraps every originally negative
value unless signDisplay is "never", which breaks the rounded-zero behavior
already handled in number_parts_core. Update the logic around the accounting
sign handling in number_parts_core so parentheses are only applied when the
post-rounding sign still qualifies for the active sign_display, preserving the
existing suppress-sign behavior for exceptZero and negative when the rounded
result is zero.
proggeramlug
added a commit
that referenced
this pull request
Jun 29, 2026
…5789/#5793/#5783 (#5806) * fix(runtime/hir/intl): #5800 — fix 29 Temporal/Intl regressions from #5789/#5793/#5783 Cluster A (12 tests): `temporal_subclass_cell` crashed on Linux when processing Proxy ids because the old guard (`obj >= GC_HEADER_SIZE + 0x1000`) passes proxy ids (0xF0000–0xFFFFF) on Linux (HEAP_MIN=0x1000). Switch to `is_plausible_heap_addr` which unconditionally rejects the entire handle band [0, 0x100000). Cluster B (16 tests): `temporal_locale_string` defaults regression from #5789. Restore per-type ECMA-402 defaults: PlainDate→{y,m,d}, PlainDateTime/Instant/ZDT→{y,m,d,h,min,sec}, PlainTime→{h,min,sec}, PlainYearMonth→{y,m}, PlainMonthDay→{m,d}. Tests comparing `Date.toLocaleString(locale, opts)` with a Temporal type newly diverged because #5789 fixed Temporal args-dropping but left Date on the fold path. Install a real `Date.prototype.toLocaleString` thunk (`date_to_locale_string_opts`) that delegates to `temporal_locale_string` with `PlainDateTime` defaults. Remove the `recv_class == Some("Date")` shortcut from the HIR fold so Date calls with args also use the thunk. Cluster C (1 test): `NumberFormat({style:"currency",unit:"test"})` threw RangeError (from the stricter `is_well_formed_unit_identifier` added in #5783) before the TypeError for the missing `currency` field. Move the currency TypeError check ahead of the unit reads to restore spec order. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(intl): move missing-currency TypeError before currencyDisplay/currencySign reads Per CodeRabbit: the TypeError for `{ style: "currency" }` with no currency was firing after the `currencyDisplay` and `currencySign` GetOption calls. A proxy trap on `get currencyDisplay()` would throw a different error first, breaking the spec-mandated observable read order. Move the check to immediately after the `currency` read. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Ralph <ralph@skelpo.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
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
2 participants
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.
Closes #5581 (partial — targets the ~30 mechanically-fixable failures out of 71 without requiring large CLDR data tables).
Root Cause
Six distinct bugs in
perry-runtime/src/intl/number_format*.rscaused failures across theintl402/NumberFormattest262 category:currencySign: Negative values were formatted with a minus sign instead of parentheses. Fixed by detectingaccounting+ negative, formatting the absolute value, then wrapping the result in(…)literal parts.ko-KRandzh-TWrequireUS$instead of$for USD. Added ausd_symbol(locale)helper used at all USD format sites.IsWellFormedUnitIdentifiervalidation: The previous check used a naive prefix scan that admitted non-sanctioned units. Replaced with the ECMA-402 Table 2 CLDR sanctioned-unit list plus the correct-per-compound algorithm.roundingModeSymbol → TypeError:get_string_option_enumsilently converts Symbols;roundingModemust throw a TypeError per spec. Switched toenum_option_strict(which callsget_option_string_coerced).de_stylebranch tocurrency_instance_parts.NaNmust render as非數值in Chinese locales. Added anan_string(locale)helper wired intonumber_parts_core.Before / After
Deferred (require large CLDR data tables, high regression risk): compact notation for de/ja/ko/zh, unit display localization, en-IN Indian grouping.
Test plan
cargo build --release -p perry-runtime— clean build, no new errorscargo fmt --all -- --check— passesbash scripts/check_file_size.sh— passes (both edited files remain under 2000 lines)cargo test --release -p perry-runtime— 1100 passed, 0 failed (two pre-existingobject::testsisolation failures are unrelated to intl)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
roundingIncrementis used with accounting currency formatting.