fix(intl): #5585 — address CodeRabbit review (fmt + correctness)

Ralph · claude · Ralph · commit b02914cb4cd1 · 2026-06-27T01:58:08.000-07:00
- cargo fmt (the lint check failed on formatting).
- Route the CanonicalizeLocaleList branches through `canonicalize_language_tag`
  (feature-aware, matches intl/locales.rs) instead of `canonical_locale`.
- Skip absent indices via HasProperty when walking array-like locale objects
  (`{length:3,0:"en"}` → `["en"]`, not a TypeError on the holes).
- `unicode_extension_keyword` stops at the `x` private-use singleton so
  `en-x-u-kn` is not misread as a `kn` Unicode extension.
- Collator `collation` option rejects the reserved `standard`/`search` values
  (and malformed types); a valid option now wins over the `-u-co-` keyword.
- `supportedLocalesOf` runs CanonicalizeLocaleList(locales) before reading
  `options` (locale errors/side-effects precede option validation).
- Collator compare normalizes with NFC (not NFD) so the Swedish fast path keeps
  its precomposed å/ä/ö; `ignorePunctuation` stripping uses an explicit
  punctuation set instead of whole Latin-1 ranges (no longer eats ª/µ/º/¹²³/¼).
- RTF `format`/`formatToParts` and PluralRules `selectRange` use a ToNumber that
  rejects BigInt with TypeError (`format(1n,"day")` throws).

test262 intl402 across the six dirs: 307/387 pass (79%), up from 297 — 10 more
cases, no regressions.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/crates/perry-runtime/src/intl.rs b/crates/perry-runtime/src/intl.rs
@@ -549,6 +549,14 @@ fn canonicalize_language_tag(tag: &str) -> Option<String> {
     }
 }
 
+/// `HasProperty(O, ToString(index))` — true when the integer-indexed property is
+/// present (own or inherited). Used to skip holes/absent indices in
+/// CanonicalizeLocaleList's array/array-like walk.
+fn js_has_index(obj: f64, index: u32) -> bool {
+    let key = string_value(&index.to_string());
+    crate::object::js_object_has_property(obj, key).to_bits() == crate::value::TAG_TRUE
+}
+
 /// CanonicalizeLocaleList element handler: a present element must be a String or
 /// an Object (an `Intl.Locale` or anything ToString-able), else `TypeError`; the
 /// resulting tag is canonicalized (`RangeError` if structurally invalid) and
@@ -563,7 +571,7 @@ fn push_locale_element(out: &mut Vec<String>, value: f64) {
         // undefined / null / boolean / number / Symbol element → TypeError.
         throw_type_error("locale must be a String or Object");
     };
-    let Some(canonical) = canonical_locale(&tag) else {
+    let Some(canonical) = canonicalize_language_tag(&tag) else {
         throw_invalid_language_tag(&tag);
     };
     if !out.iter().any(|existing| existing == &canonical) {
@@ -584,7 +592,7 @@ fn locales_from_value(locales: f64) -> Vec<String> {
     // A String argument is treated as a single-element list (not iterated by char).
     if js.is_any_string() {
         let tag = string_from_string_value(locales).unwrap_or_default();
-        let Some(canonical) = canonical_locale(&tag) else {
+        let Some(canonical) = canonicalize_language_tag(&tag) else {
             throw_invalid_language_tag(&tag);
         };
         return vec![canonical];
@@ -611,6 +619,11 @@ fn locales_from_value(locales: f64) -> Vec<String> {
         };
         let mut out = Vec::with_capacity(len as usize);
         for i in 0..len {
+            // Skip absent indices (`HasProperty` is false) — e.g.
+            // `{ length: 3, 0: "en" }` yields just `["en"]`, never `undefined`.
+            if !js_has_index(locales, i) {
+                continue;
+            }
             push_locale_element(&mut out, get_field(obj, &i.to_string()));
         }
         return out;
@@ -654,9 +667,14 @@ fn unicode_extension_keyword(locale: &str, key: &str) -> Option<String> {
     let lower = locale.to_ascii_lowercase();
     let key = key.to_ascii_lowercase();
     let mut iter = lower.split('-');
-    // Advance to the `u` singleton.
+    // Advance to the `u` singleton. A `x` singleton starts the private-use
+    // sequence (which must come last); a `u` inside it — e.g. `en-x-u-kn` — is
+    // private data, not a Unicode extension, so stop scanning there.
     let mut in_u = false;
-    while let Some(p) = iter.next() {
+    for p in iter.by_ref() {
+        if p == "x" {
+            return None;
+        }
         if p == "u" {
             in_u = true;
             break;
@@ -1260,27 +1278,34 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // (constructor-options-throwing-getters / resolvedOptions order.js).
             let options = coerce_options_reject_null(options);
             let usage = enum_option_strict(options, "usage", &["sort", "search"], "sort");
-            let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
-            // `collation` is a free-form `type` string (RangeError if malformed);
-            // it has no CLDR effect here, so it resolves to "default".
-            if let Some(collation) = get_option_string_coerced(options, "collation") {
-                if !is_well_formed_numbering_system(&collation) {
+            let _ = enum_option_strict(
+                options,
+                "localeMatcher",
+                &["lookup", "best fit"],
+                "best fit",
+            );
+            // `collation` is a `type` string: malformed, or the reserved `standard`
+            // /`search` values, are a RangeError (the latter are only valid as a
+            // `usage` selector, never an explicit collation). A valid value wins
+            // over any `-u-co-` keyword; absent ⇒ fall back to the extension.
+            let collation_opt = get_option_string_coerced(options, "collation").map(|v| {
+                if !is_well_formed_numbering_system(&v) || v == "standard" || v == "search" {
                     throw_range_error(&format!(
-                        "Value {collation} out of range for Intl options property collation"
+                        "Value {v} out of range for Intl options property collation"
                     ));
                 }
-            }
+                v
+            });
             let numeric_opt = get_bool_option(options, "numeric");
-            let case_first_opt =
-                get_option_string_coerced(options, "caseFirst").map(|v| {
-                    if ["upper", "lower", "false"].contains(&v.as_str()) {
-                        v
-                    } else {
-                        throw_range_error(&format!(
-                            "Value {v} out of range for Intl options property caseFirst"
-                        ))
-                    }
-                });
+            let case_first_opt = get_option_string_coerced(options, "caseFirst").map(|v| {
+                if ["upper", "lower", "false"].contains(&v.as_str()) {
+                    v
+                } else {
+                    throw_range_error(&format!(
+                        "Value {v} out of range for Intl options property caseFirst"
+                    ))
+                }
+            });
             let sensitivity = enum_option_strict(
                 options,
                 "sensitivity",
@@ -1291,20 +1316,21 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // ResolveLocale: when an option is absent, fall back to the matching
             // Unicode (`-u-`) extension keyword in the resolved locale — `kn`
             // (numeric, value-less ⇒ true) and `kf` (caseFirst).
-            let numeric = numeric_opt.unwrap_or_else(|| {
-                match unicode_extension_keyword(&locale, "kn") {
+            let numeric =
+                numeric_opt.unwrap_or_else(|| match unicode_extension_keyword(&locale, "kn") {
                     Some(v) => v != "false",
                     None => false,
-                }
-            });
+                });
             let case_first = case_first_opt.unwrap_or_else(|| {
                 unicode_extension_keyword(&locale, "kf")
                     .filter(|v| ["upper", "lower", "false"].contains(&v.as_str()))
                     .unwrap_or_else(|| "false".to_string())
             });
-            let collation = unicode_extension_keyword(&locale, "co")
-                .filter(|v| !v.is_empty() && v != "standard" && v != "search")
-                .unwrap_or_else(|| "default".to_string());
+            let collation = collation_opt.unwrap_or_else(|| {
+                unicode_extension_keyword(&locale, "co")
+                    .filter(|v| !v.is_empty() && v != "standard" && v != "search")
+                    .unwrap_or_else(|| "default".to_string())
+            });
             set_internal_field(obj, KEY_COL_USAGE, string_value(&usage));
             set_internal_field(obj, KEY_COL_SENSITIVITY, string_value(&sensitivity));
             set_internal_field(obj, KEY_COL_IGNORE_PUNCT, bool_value(ignore_punct));
@@ -1328,7 +1354,12 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // `? ToObject(options)` (null → TypeError), then GetOption in order:
             // localeMatcher, granularity (options-order.js / options-null.js).
             let options = coerce_options_reject_null(options);
-            let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
+            let _ = enum_option_strict(
+                options,
+                "localeMatcher",
+                &["lookup", "best fit"],
+                "best fit",
+            );
             let granularity =
                 normalize_granularity(get_option_string_coerced(options, "granularity"));
             set_internal_field(obj, KEY_GRANULARITY, string_value(&granularity));
@@ -1350,7 +1381,12 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // TypeError), then GetOption: localeMatcher, type, style
             // (options-getoptionsobject.js / options-order.js).
             let options = get_options_object(options);
-            let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
+            let _ = enum_option_strict(
+                options,
+                "localeMatcher",
+                &["lookup", "best fit"],
+                "best fit",
+            );
             let list_type = enum_option_strict(
                 options,
                 "type",
@@ -1383,7 +1419,12 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // `? ToObject(options)` (null → TypeError), then GetOption in order:
             // localeMatcher, numberingSystem, style, numeric (options-order.js).
             let options = coerce_options_reject_null(options);
-            let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
+            let _ = enum_option_strict(
+                options,
+                "localeMatcher",
+                &["lookup", "best fit"],
+                "best fit",
+            );
             if let Some(ns) = get_option_string_coerced(options, "numberingSystem") {
                 if !is_well_formed_numbering_system(&ns) {
                     throw_range_error(&format!(
@@ -1416,7 +1457,12 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
             // (minimumIntegerDigits, min/maxFractionDigits, min/maxSignificantDigits,
             // roundingIncrement, roundingMode, roundingPriority, trailingZeroDisplay).
             let options = get_options_object(options);
-            let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
+            let _ = enum_option_strict(
+                options,
+                "localeMatcher",
+                &["lookup", "best fit"],
+                "best fit",
+            );
             let pr_type = enum_option_strict(options, "type", &["cardinal", "ordinal"], "cardinal");
             set_internal_field(obj, KEY_TYPE, string_value(&pr_type));
             let notation = enum_option_strict(
@@ -1573,17 +1619,26 @@ extern "C" fn plural_rules_constructor_thunk(closure: *const ClosureHeader, rest
 }
 
 fn supported_locales_array(locales: f64, options: f64) -> f64 {
-    // SupportedLocales: when `options` is not undefined, `? ToObject(options)`
-    // (null → TypeError) then `? GetOption(options, "localeMatcher", …)` — so an
-    // invalid localeMatcher is a RangeError even though the matcher choice does
-    // not affect Perry's lookup result.
+    // `supportedLocalesOf(locales, options)`:
+    //   1. requestedLocales = ? CanonicalizeLocaleList(locales)   ← runs FIRST,
+    //      so a malformed locale errors before `options` is touched.
+    //   2. SupportedLocales(..., options): when `options` is not undefined,
+    //      `? ToObject(options)` (null → TypeError) then
+    //      `? GetOption(options, "localeMatcher", …)` — an invalid localeMatcher
+    //      is a RangeError even though the matcher choice does not affect Perry's
+    //      lookup result.
+    let requested = locales_from_value(locales);
     if !JSValue::from_bits(options.to_bits()).is_undefined() {
         let options = coerce_options_reject_null(options);
-        let _ = enum_option_strict(options, "localeMatcher", &["lookup", "best fit"], "best fit");
+        let _ = enum_option_strict(
+            options,
+            "localeMatcher",
+            &["lookup", "best fit"],
+            "best fit",
+        );
     }
     // BestAvailableLocale-filter the canonicalized request list: drop tags whose
     // primary language Perry can't service (e.g. `zxx`), keeping order + dedup.
-    let requested = locales_from_value(locales);
     let mut arr = js_array_alloc(0);
     for locale in requested {
         if is_available_locale(&locale) {
diff --git a/crates/perry-runtime/src/intl/date_collator.rs b/crates/perry-runtime/src/intl/date_collator.rs
@@ -350,7 +350,9 @@ pub(crate) fn swedish_collation_key(s: &str) -> Vec<u32> {
 #[cfg(feature = "string-normalize")]
 fn collation_normalize(s: &str) -> String {
     use unicode_normalization::UnicodeNormalization;
-    s.nfd().collect()
+    // NFC (composition), not NFD: it makes canonical equivalents equal while
+    // keeping precomposed `å/ä/ö` intact for the Swedish fast path below.
+    s.nfc().collect()
 }
 #[cfg(not(feature = "string-normalize"))]
 fn collation_normalize(s: &str) -> String {
@@ -401,20 +403,23 @@ fn strip_ignorable_punctuation(s: &str) -> String {
 }
 
 fn is_punctuation(c: char) -> bool {
-    // ASCII punctuation plus the common Unicode punctuation/symbol ranges that
-    // UCA marks variable; a pragmatic superset of what the parity tests exercise.
+    // ASCII punctuation plus an explicit set of Unicode punctuation code points,
+    // deliberately NOT whole Latin-1 ranges — those contain letters/numbers
+    // (`ª` U+00AA, `µ` U+00B5, `º` U+00BA, the `¹²³` superscripts, `¼½¾`
+    // fractions) that must not be stripped or distinct strings would compare
+    // equal. The General Punctuation block (U+2000–U+206F) and CJK punctuation
+    // (U+3000–U+303F) are all punctuation/spaces and are safe as ranges.
     c.is_ascii_punctuation()
         || matches!(c,
-            '\u{00A1}'..='\u{00BF}'
-            | '\u{2010}'..='\u{2027}'
-            | '\u{2030}'..='\u{205E}'
+            '\u{00A1}' | '\u{00A7}' | '\u{00AB}' | '\u{00B6}' | '\u{00B7}'
+            | '\u{00BB}' | '\u{00BF}'
+            | '\u{2000}'..='\u{206F}'
             | '\u{3000}'..='\u{303F}')
 }
 
 pub(crate) fn collator_compare_object(obj: *const ObjectHeader, left: f64, right: f64) -> f64 {
     let locale = get_string_field(obj, KEY_LOCALE).unwrap_or_else(|| "en-US".to_string());
-    let ignore_punct =
-        get_field(obj, KEY_COL_IGNORE_PUNCT).to_bits() == crate::value::TAG_TRUE;
+    let ignore_punct = get_field(obj, KEY_COL_IGNORE_PUNCT).to_bits() == crate::value::TAG_TRUE;
     let (mut l, mut r) = (value_to_string(left), value_to_string(right));
     if ignore_punct {
         l = strip_ignorable_punctuation(&l);
diff --git a/crates/perry-runtime/src/intl/list_relative_plural.rs b/crates/perry-runtime/src/intl/list_relative_plural.rs
@@ -301,19 +301,30 @@ pub(crate) fn rtf_parts(value: f64, unit: &str) -> Vec<(&'static str, String)> {
     parts
 }
 
+/// `ToNumber(value)` that rejects BigInt with a TypeError, matching the
+/// ECMA-262 abstract operation. `js_number_coerce` alone converts `1n` → `1`
+/// (for `Number(1n)`), but `Intl` `format`/`select*` go through ToNumber, so
+/// `format(1n, "day")` must throw. A Symbol still throws inside `js_number_coerce`,
+/// and an object's `valueOf` is honoured there.
+pub(crate) fn to_number_reject_bigint(value: f64) -> f64 {
+    if JSValue::from_bits(value.to_bits()).is_bigint() {
+        throw_type_error("Cannot convert a BigInt value to a number");
+    }
+    crate::builtins::js_number_coerce(value)
+}
+
 /// Shared steps of `format`/`formatToParts`: `value = ? ToNumber(value)` (a
-/// Symbol throws TypeError; an object's `valueOf` is honoured), then
+/// Symbol or BigInt throws TypeError; an object's `valueOf` is honoured), then
 /// `unit = ? ToString(unit)`, then the RangeError guards for a non-finite value
 /// or an unsanctioned unit. Returns the rendered parts together with the
 /// resolved singular `unit` (the `[[Unit]]` field formatToParts attaches).
 pub(crate) fn rtf_instance_parts_and_unit(
     value: f64,
     unit_arg: f64,
 ) -> (Vec<(&'static str, String)>, &'static str) {
-    // Full ToNumber (not `JSValue::to_number`, which returns NaN for objects and
-    // doesn't reject Symbols): valueOf is invoked, and a Symbol value throws the
-    // expected TypeError *before* the finite-ness RangeError (format/value-symbol.js).
-    let number = crate::builtins::js_number_coerce(value);
+    // ToNumber: a Symbol/BigInt value throws TypeError *before* the finite-ness
+    // RangeError (format/value-symbol.js); an object's valueOf is invoked.
+    let number = to_number_reject_bigint(value);
     let unit_str = value_to_string(unit_arg);
     if !number.is_finite() {
         throw_range_error("Value need to be finite number for Intl.RelativeTimeFormat.format()");
@@ -513,8 +524,8 @@ pub(crate) fn plural_select_range(start: f64, end: f64) -> f64 {
     {
         throw_type_error("Intl.PluralRules.prototype.selectRange: start and end must be defined");
     }
-    let s = crate::builtins::js_number_coerce(start);
-    let e = crate::builtins::js_number_coerce(end);
+    let s = to_number_reject_bigint(start);
+    let e = to_number_reject_bigint(end);
     if s.is_nan() || e.is_nan() {
         throw_range_error("Invalid values for Intl.PluralRules.selectRange()");
     }
@@ -535,16 +546,16 @@ pub(crate) fn plural_rules_resolved_options_object(obj: *const ObjectHeader) ->
         "type",
         string_value(if is_ordinal { "ordinal" } else { "cardinal" }),
     );
-    let notation =
-        get_string_field(obj, KEY_PR_NOTATION).unwrap_or_else(|| "standard".to_string());
+    let notation = get_string_field(obj, KEY_PR_NOTATION).unwrap_or_else(|| "standard".to_string());
     set_field(out, "notation", string_value(&notation));
     // `compactDisplay` surfaces only when notation is "compact".
     if notation == "compact" {
         set_field(
             out,
             "compactDisplay",
             string_value(
-                &get_string_field(obj, KEY_PR_COMPACT_DISPLAY).unwrap_or_else(|| "short".to_string()),
+                &get_string_field(obj, KEY_PR_COMPACT_DISPLAY)
+                    .unwrap_or_else(|| "short".to_string()),
             ),
         );
     }
