fix(intl): #5904 — ResolveLocale for the nu numbering-system extension (NumberFormat/DateTimeFormat/DurationFormat) - #5963
Merged
Conversation
Reconcile the requested locale's `-u-nu-` keyword with an explicit
`options.numberingSystem` per ECMA-402 ResolveLocale, and update the
resolved locale so `resolvedOptions().locale` / `.numberingSystem` reflect
only the *supported* value actually used:
* option present + supported → option wins; the `-u-nu-` keyword survives
in the locale only when it names the same value
* no usable option → fall back to the supported locale extension,
else the `latn` default (keyword dropped)
A numbering system counts as supported when it is `latn` (default Latin/ASCII
digits) or Perry has a transliteration table for it. Base-tag casing is
preserved (`en-US` no longer collapses to `en-us`).
New `intl/numbering_system.rs` module holds the `nu`-extension BCP-47 tag
helpers (split out of `intl.rs` to keep it under the 2,000-line gate). Wired
into NumberFormat, DateTimeFormat, and DurationFormat construction.
Fixes intl402 resolved-numbering-system-unicode-extensions-and-options.js for
NumberFormat, DateTimeFormat, and DurationFormat (3 tests). Zero regressions
across all three slices plus Collator.
Refs #5904 #5899 #5906
📝 WalkthroughWalkthroughThis PR introduces a shared ChangesNumbering system resolution unification
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Constructor as Intl constructor (DateTimeFormat/DurationFormat/NumberFormat)
participant Resolver as resolve_numbering_system
participant LocaleUtil as -u- extension utilities
Constructor->>Constructor: parse and validate numberingSystem option
Constructor->>Resolver: resolve_numbering_system(locale, opt_ns)
Resolver->>LocaleUtil: split_u_extension(locale)
LocaleUtil-->>Resolver: base, keywords, tail
Resolver->>Resolver: reconcile opt_ns with existing nu keyword
Resolver->>LocaleUtil: rebuild_locale / strip or add nu keyword
LocaleUtil-->>Resolver: resolved locale string
Resolver-->>Constructor: (resolved_locale, numbering)
Constructor->>Constructor: store KEY_LOCALE and numbering system key
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/intl/duration_format.rs (1)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
valid_numbering_systemduplicatesis_well_formed_numbering_system.This is byte-for-byte identical to
numbering_system::is_well_formed_numbering_system, whichintl.rsalready imports (so it's reachable here assuper::is_well_formed_numbering_system). Consider dropping the local copy to keep the well-formedness rule single-sourced.🤖 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/duration_format.rs` around lines 107 - 112, The local valid_numbering_system helper duplicates the existing well-formedness check already available as super::is_well_formed_numbering_system. Remove the duplicate implementation in duration_format.rs and update the duration formatting code to call the imported shared helper instead, keeping the rule single-sourced and ensuring any future changes only need to be made in one place.
🤖 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/numbering_system.rs`:
- Around line 54-85: `numbering_system_from_locale` is using a custom scan that
mishandles `-u-` attributes and singleton boundaries, so it can miss `nu` or
overrun into later extensions. Update this function to reuse the same
U-extension decomposition logic as `split_u_extension`, and then read the `nu`
keyword from that parsed structure instead of manually walking subtags. Ensure
the fix preserves correct handling for leading attributes and stops cleanly at
the next singleton extension.
---
Nitpick comments:
In `@crates/perry-runtime/src/intl/duration_format.rs`:
- Around line 107-112: The local valid_numbering_system helper duplicates the
existing well-formedness check already available as
super::is_well_formed_numbering_system. Remove the duplicate implementation in
duration_format.rs and update the duration formatting code to call the imported
shared helper instead, keeping the rule single-sourced and ensuring any future
changes only need to be made in one place.
🪄 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: a4d2db44-78a3-44de-a4e6-1f6aa5d616db
📒 Files selected for processing (4)
crates/perry-runtime/src/intl.rscrates/perry-runtime/src/intl/duration_format.rscrates/perry-runtime/src/intl/number_format_options.rscrates/perry-runtime/src/intl/numbering_system.rs
Comment on lines
+54
to
+85
| pub(super) fn numbering_system_from_locale(locale: &str) -> Option<String> { | ||
| let lower = locale.to_ascii_lowercase(); | ||
| let subtags: Vec<&str> = lower.split('-').collect(); | ||
| let u = subtags.iter().position(|s| *s == "u")?; | ||
| let mut i = u + 1; | ||
| while i < subtags.len() { | ||
| let key = subtags[i]; | ||
| // A keyword key is exactly two chars; everything up to the next key is its value. | ||
| if key.len() == 2 { | ||
| if key == "nu" { | ||
| let mut value = String::new(); | ||
| let mut j = i + 1; | ||
| while j < subtags.len() && subtags[j].len() != 2 { | ||
| if !value.is_empty() { | ||
| value.push('-'); | ||
| } | ||
| value.push_str(subtags[j]); | ||
| j += 1; | ||
| } | ||
| return (!value.is_empty()).then_some(value); | ||
| } | ||
| i += 1; | ||
| while i < subtags.len() && subtags[i].len() != 2 { | ||
| i += 1; | ||
| } | ||
| } else { | ||
| // Hit another singleton extension (e.g. `-t-`); `nu` lives only under `u`. | ||
| break; | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
numbering_system_from_locale mis-parses -u- attributes and singleton boundaries, diverging from split_u_extension.
Two defects in this hand-rolled scan:
- The
elsebranch (lines 79-82) breaks on any subtag whose length isn't 2, so a leading-u-attribute (3–8 chars, e.g.en-u-attr-nu-latn) is treated as "another singleton" and thenukeyword is never found. - The value loop
while subtags[j].len() != 2(line 66) never stops at a following singleton (len 1). For a valid tag likeen-u-nu-arab-x-fooit collectsarab-x-foo, which failsis_supported_numbering_system, soext_nsbecomesNoneand resolution wrongly falls back tolatninstead ofarab.
split_u_extension already handles attributes and singleton boundaries correctly. Delegating to it removes the divergent parser and fixes both cases.
🐛 Proposed fix: reuse the correct decomposition
pub(super) fn numbering_system_from_locale(locale: &str) -> Option<String> {
- let lower = locale.to_ascii_lowercase();
- let subtags: Vec<&str> = lower.split('-').collect();
- let u = subtags.iter().position(|s| *s == "u")?;
- let mut i = u + 1;
- while i < subtags.len() {
- let key = subtags[i];
- // A keyword key is exactly two chars; everything up to the next key is its value.
- if key.len() == 2 {
- if key == "nu" {
- let mut value = String::new();
- let mut j = i + 1;
- while j < subtags.len() && subtags[j].len() != 2 {
- if !value.is_empty() {
- value.push('-');
- }
- value.push_str(subtags[j]);
- j += 1;
- }
- return (!value.is_empty()).then_some(value);
- }
- i += 1;
- while i < subtags.len() && subtags[i].len() != 2 {
- i += 1;
- }
- } else {
- // Hit another singleton extension (e.g. `-t-`); `nu` lives only under `u`.
- break;
- }
- }
- None
+ let (_, keywords, _) = split_u_extension(locale)?;
+ keywords
+ .into_iter()
+ .find(|(key, _)| key == "nu")
+ .map(|(_, value)| value.join("-"))
+ .filter(|v| !v.is_empty())
}📝 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
| pub(super) fn numbering_system_from_locale(locale: &str) -> Option<String> { | |
| let lower = locale.to_ascii_lowercase(); | |
| let subtags: Vec<&str> = lower.split('-').collect(); | |
| let u = subtags.iter().position(|s| *s == "u")?; | |
| let mut i = u + 1; | |
| while i < subtags.len() { | |
| let key = subtags[i]; | |
| // A keyword key is exactly two chars; everything up to the next key is its value. | |
| if key.len() == 2 { | |
| if key == "nu" { | |
| let mut value = String::new(); | |
| let mut j = i + 1; | |
| while j < subtags.len() && subtags[j].len() != 2 { | |
| if !value.is_empty() { | |
| value.push('-'); | |
| } | |
| value.push_str(subtags[j]); | |
| j += 1; | |
| } | |
| return (!value.is_empty()).then_some(value); | |
| } | |
| i += 1; | |
| while i < subtags.len() && subtags[i].len() != 2 { | |
| i += 1; | |
| } | |
| } else { | |
| // Hit another singleton extension (e.g. `-t-`); `nu` lives only under `u`. | |
| break; | |
| } | |
| } | |
| None | |
| } | |
| pub(super) fn numbering_system_from_locale(locale: &str) -> Option<String> { | |
| let (_, keywords, _) = split_u_extension(locale)?; | |
| keywords | |
| .into_iter() | |
| .find(|(key, _)| key == "nu") | |
| .map(|(_, value)| value.join("-")) | |
| .filter(|v| !v.is_empty()) | |
| } |
🤖 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/numbering_system.rs` around lines 54 - 85,
`numbering_system_from_locale` is using a custom scan that mishandles `-u-`
attributes and singleton boundaries, so it can miss `nu` or overrun into later
extensions. Update this function to reuse the same U-extension decomposition
logic as `split_u_extension`, and then read the `nu` keyword from that parsed
structure instead of manually walking subtags. Ensure the fix preserves correct
handling for leading attributes and stops cleanly at the next singleton
extension.
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
Implements the ECMA-402 ResolveLocale step for the
nu(numbering system) Unicode extension key acrossIntl.NumberFormat,Intl.DateTimeFormat, andIntl.DurationFormat. The requested locale's-u-nu-keyword is reconciled with an explicitoptions.numberingSystem, and the resolved locale is rewritten soresolvedOptions().locale/.numberingSystemreflect only the supported value actually used:options.numberingSystemlocalenumberingSystemen-u-nu-arabinvalid(unsupported)en-u-nu-arabaraben-u-nu-invalidinvalid2enlatnen-u-nu-latnarabenaraben-u-nu-arabaraben-u-nu-arabarabRules:
-u-nu-keyword survives in the resolved locale only when it names the same value as the (supported) option.latndefault (keyword dropped from the locale).A numbering system counts as supported when it is
latn(the default Latin/ASCII digits, which need no transliteration table) or Perry has a digit table for it.Root cause
configure_number_format(and the DTF / DurationFormat equivalents) only validated thenumberingSystemoption for well-formedness and never ran ResolveLocale: the resolvedlocalekept the-u-nu-extension verbatim regardless of whether the system was supported or overridden, andresolvedOptions().numberingSystemdidn't reflect the option-vs-extension precedence.Implementation notes
crates/perry-runtime/src/intl/numbering_system.rsmodule holds the-u-nu-BCP-47 tag helpers (resolve_numbering_system,split_u_extension,strip/with_numbering_system_keyword,is_supported_numbering_system, plus the pre-existingnumbering_system_from_locale/is_well_formed_numbering_systemmoved out ofintl.rsto keep it under the 2,000-line gate).en-USno longer collapses toen-us(this was the subtle bug that would otherwise have regressednumbering-system-options.js).numberingSystemGetOption read site, preserving the option-read order thatconstructor-options-order.jsasserts.Before / after (test262, internal Linux box, Node v26.3.0)
intl402/NumberFormatresolved-numbering-system-unicode-extensions-and-options.jsintl402/DateTimeFormatresolved-numbering-system-unicode-extensions-and-options.jsintl402/DurationFormatresolved-numbering-system-unicode-extensions-and-options.js+3 tests, zero regressions (verified across NumberFormat / DateTimeFormat / DurationFormat / Collator, including
numbering-system-options.jsstaying green).cargo fmt --all -- --checkandscripts/check_file_size.shpass.Refs #5904 #5899 #5906
Summary by CodeRabbit
New Features
Bug Fixes
numberingSystemso invalid or unsupported values are normalized correctly.