diff --git a/changelog.d/7797-locale-separators.md b/changelog.d/7797-locale-separators.md new file mode 100644 index 0000000000..42cc498f21 --- /dev/null +++ b/changelog.d/7797-locale-separators.md @@ -0,0 +1,13 @@ +**`toLocaleString` now uses each locale's own digit separators, and groups when called with no arguments** (#7429, #7428). + +**#7429 — the separator pair was a `de`-vs-everything-else branch.** Written when `de-DE` was the only non-`en` locale under test, and duplicated at three sites. So every locale that does not group with `,` was wrong, not only French. Measured against Node v26.5.1 across 21 locales, Perry produced `,`/`.` for all of them except German; the correct data is three groups — `.`/`,` for `de`/`es`/`it`/`pt`/`nl`/`tr`, a space plus `,` for `fr`/`ru`/`pl`/`nb`/`sv`/`fi`/`cs`/`hu`/`uk`, and `,`/`.` for `en`/`ja`/`ko`/`zh`. `locale_separators` now returns the CLDR pair for the primary language subtag, and the three call sites share it. + +The French case is the one the issue caught, and it is the sharp one: `fr-FR` groups with **U+202F** (narrow no-break space) while `fr-CA` uses **U+00A0**. Those render identically in a terminal, so the region is consulted for French and only for French. Getting it wrong is silent everywhere except a byte-for-byte oracle diff — and the first cut of this fix did get it wrong, slicing `locale.get(..6)` against the five-byte `"fr-FR"`, which returns `None` and quietly demoted every French locale to the U+00A0 arm. The oracle diff caught it; nothing else would have. + +Locales not named in the table keep the previous `,`/`.` default, so this widens correctness without changing any locale it does not list. + +**#7428 — zero-argument `bigint.toLocaleString()` produced no grouping**, while `toLocaleString(undefined)` was already correct. Codegen lowers the zero-arg form to `Expr::DateToLocaleString`, which lands in `js_object_default_to_locale_string`; that function has arms for numbers, Dates and Temporal values, and a BigInt fell past them into Object.prototype's "Invoke(O, 'toString')" tail — and `BigInt.prototype.toString` has no grouping. Any call carrying locales/options goes down the generic method-call path to the real thunk instead, so the two forms never met. That asymmetry is why the bug survived: the natural way to write a test for it (`toLocaleString(undefined)`) exercises the other path. A BigInt arm now formats through the same ECMA-402 machinery with the default locale. + +`test_gap_intl_locale_separators_7429_7428.ts` covers both, asserting separators as **code points** rather than as formatted strings, so U+202F, U+00A0 and a plain space cannot be confused; and it exercises `Intl.NumberFormat` with a fractional value so a locale that got the group separator right and the decimal wrong still fails. + +Verified: the new gap test passes byte-for-byte against Node v26.5.1, all 21 locales plus the `Intl.NumberFormat` rows match, and `test_gap_intl` (7) and `test_gap_bigint` (4) stay green. `cargo test -p perry-runtime --lib` is unchanged; the one intermittent failure seen during validation is `gc::tests::root_words::bare_address_in_shadow_slot_survives_a_real_collection`, which reproduces at 2/10 runs on **clean `main`** with these changes reverted and rebuilt — it is #7365, not this change. diff --git a/crates/perry-runtime/src/intl/number_format.rs b/crates/perry-runtime/src/intl/number_format.rs index 386b4e5a60..47aed6eacb 100644 --- a/crates/perry-runtime/src/intl/number_format.rs +++ b/crates/perry-runtime/src/intl/number_format.rs @@ -664,9 +664,8 @@ fn number_parts_core(r: &NfResolved, value: f64) -> Vec<(&'static str, String)> return currency_instance_parts(r, value); } - let de_style = r.locale.eq_ignore_ascii_case("de") || r.locale.starts_with("de-"); - let group_sep = if de_style { '.' } else { ',' }; - let decimal_sep = if de_style { ',' } else { '.' }; + // #7429: CLDR separators for the resolved locale, not a de-vs-rest guess. + let (group_sep, decimal_sep) = locale_separators(&r.locale); let mut parts: Vec<(&'static str, String)> = Vec::new(); let is_zero = value == 0.0; @@ -881,6 +880,49 @@ fn locale_lang(locale: &str) -> &str { locale.split(['-', '_']).next().unwrap_or(locale) } +/// The `(group, decimal)` separator pair for a locale — CLDR's `symbols-*` +/// `group` and `decimal` for its primary language subtag. +/// +/// #7429: this used to be a single `de`-vs-everything-else branch, written when +/// `de-DE` was the only non-`en` locale under test. Every other locale that +/// does not group with `,` was therefore wrong, not just French: measured +/// against Node v26.5.1, `es`/`it`/`pt`/`nl`/`tr` want `.` like German, and +/// `fr`/`ru`/`pl`/`nb`/`sv`/`fi`/`cs`/`hu`/`uk` group with a SPACE. +/// +/// The space is not one character. `fr-FR` uses U+202F (narrow no-break space) +/// while `fr-CA` uses U+00A0, which is why the region is consulted for French +/// and only for French — every other space-grouping locale here is U+00A0 in +/// CLDR. Getting that wrong is invisible in a terminal and loud in a +/// byte-for-byte oracle diff, which is exactly how #7429 was found. +/// +/// Locales absent from the table keep the previous default (`,` and `.`), so +/// this widens correctness without changing any locale it does not name. +fn locale_separators(locale: &str) -> (char, char) { + const NNBSP: char = '\u{202f}'; + const NBSP: char = '\u{00a0}'; + match locale_lang(locale) { + // `.` group, `,` decimal. + "de" | "es" | "it" | "pt" | "nl" | "tr" | "id" | "da" | "ro" | "el" | "vi" | "ca" => { + ('.', ',') + } + // Space group, `,` decimal. French splits by region: fr-FR is U+202F, + // fr-CA (and the rest of these) U+00A0. + "fr" => { + // `"fr-FR"` is five bytes; slicing `..6` returns None and silently + // demotes every French locale to the U+00A0 arm. + let region_fr = locale.eq_ignore_ascii_case("fr") + || locale + .get(..5) + .is_some_and(|p| p.eq_ignore_ascii_case("fr-fr")); + (if region_fr { NNBSP } else { NBSP }, ',') + } + "ru" | "pl" | "nb" | "no" | "sv" | "fi" | "cs" | "sk" | "hu" | "uk" | "lv" | "lt" + | "et" | "bg" => (NBSP, ','), + // `,` group, `.` decimal — en, ja, ko, zh, he, th, and the default. + _ => (',', '.'), + } +} + /// Prefix text some locales place *before* the number for a unit (e.g. the /// Japanese/Korean/Chinese "speed" reading of `kilometer-per-hour`'s long /// form: "時速 -987 キロメートル"). Only a handful of compound units have a @@ -1101,9 +1143,8 @@ fn bigint_number_parts_exact( negative: bool, abs_digits: &str, ) -> Vec<(&'static str, String)> { - let de_style = r.locale.eq_ignore_ascii_case("de") || r.locale.starts_with("de-"); - let group_sep = if de_style { '.' } else { ',' }; - let decimal_sep = if de_style { ',' } else { '.' }; + // #7429: CLDR separators for the resolved locale, not a de-vs-rest guess. + let (group_sep, decimal_sep) = locale_separators(&r.locale); set_round_ctx(&r.rounding_mode, negative); let mut parts: Vec<(&'static str, String)> = Vec::new(); diff --git a/crates/perry-runtime/src/object/native_call_method/object_proto.rs b/crates/perry-runtime/src/object/native_call_method/object_proto.rs index 83c57c570d..e0f77bc7bf 100644 --- a/crates/perry-runtime/src/object/native_call_method/object_proto.rs +++ b/crates/perry-runtime/src/object/native_call_method/object_proto.rs @@ -92,6 +92,29 @@ pub(crate) unsafe fn js_object_default_to_locale_string(receiver: f64) -> f64 { if crate::temporal::is_temporal_value(receiver) { return crate::temporal::dispatch::call_method(receiver, "toLocaleString", &[]); } + // #7428: a BigInt receiver must format through + // `BigInt.prototype.toLocaleString`, i.e. with the DEFAULT locale's digit + // grouping — `(12345678901234567890n).toLocaleString()` is + // `"12,345,678,901,234,567,890"`, not the bare digits. + // + // Without this arm a BigInt falls through to the generic tail below, whose + // job is Object.prototype.toLocaleString's "Invoke(O, 'toString')" — and + // `BigInt.prototype.toString` has no grouping. That tail is correct for the + // receivers it is written for; a BigInt simply is not one of them, the same + // way a number and a Date are handled above rather than left to it. + // + // Only the ZERO-ARG form reaches here at all: codegen lowers + // `x.toLocaleString()` to `Expr::DateToLocaleString`, while any call + // carrying locales/options goes down the generic method-call path to + // `bigint_proto_to_locale_string_thunk`. That asymmetry is why the explicit + // `toLocaleString(undefined)` was already correct while the bare call was + // not — the two forms never met. + #[cfg(feature = "intl-namespace")] + if jsval.is_bigint() { + let undef = f64::from_bits(crate::value::TAG_UNDEFINED); + let s = crate::intl::bigint_to_locale_string(receiver, undef, undef); + return f64::from_bits(JSValue::string_ptr(s).bits()); + } // Symbols are POINTER-tagged, so `!jsval.is_pointer()` would be false for // them — check before the pointer guard so the branch is reachable. let is_symbol = unsafe { crate::symbol::js_is_symbol(receiver) } != 0; diff --git a/test-files/test_gap_intl_locale_separators_7429_7428.ts b/test-files/test_gap_intl_locale_separators_7429_7428.ts new file mode 100644 index 0000000000..214f817f38 --- /dev/null +++ b/test-files/test_gap_intl_locale_separators_7429_7428.ts @@ -0,0 +1,66 @@ +// Gap: locale-aware digit grouping for `toLocaleString` (#7429, #7428). +// +// Two defects, both byte-visible only against the oracle: +// +// #7429 — the group/decimal separator pair was a single `de`-vs-everything +// branch, written when `de-DE` was the only non-`en` locale under test. Every +// other locale that does not group with `,` was wrong, not only French. The +// French case is the sharp one because the separator is a NARROW no-break +// space (U+202F) for `fr-FR` and a regular no-break space (U+00A0) for +// `fr-CA` — two characters that render identically in a terminal and differ in +// a byte-for-byte diff, which is why this test asserts the code points +// explicitly rather than eyeballing the formatted strings. +// +// #7428 — `bigint.toLocaleString()` with NO arguments produced no grouping at +// all, while `toLocaleString(undefined)` was correct. Codegen lowers the +// zero-arg form to `Expr::DateToLocaleString`, which reaches +// `js_object_default_to_locale_string`; that function had arms for numbers, +// Dates and Temporal values but not for BigInt, so a BigInt fell through to +// Object.prototype's "Invoke(O, 'toString')" tail. The two call forms never +// met, which is exactly why the bug survived: the obvious spelling in a test +// (`toLocaleString(undefined)`) exercises the other path. + +const big = 12345678901234567890n; + +// #7428: the zero-argument form must group with the default locale, and must +// agree with the explicitly-undefined form. +console.log("zeroarg:" + big.toLocaleString()); +console.log("undef:" + big.toLocaleString(undefined)); +console.log("small-zeroarg:" + (9876543n).toLocaleString()); + +// #7429: separators per locale. Printed as code points so the two space +// characters cannot be confused with each other or with a plain ASCII space. +const locales = [ + "en-US", + "de-DE", + "fr-FR", + "fr-CA", + "es-ES", + "it-IT", + "ru-RU", + "pl-PL", + "sv-SE", + "pt-BR", + "nl-NL", + "tr-TR", + "cs-CZ", + "ja-JP", +]; + +for (const loc of locales) { + const s = (9876543210n).toLocaleString(loc); + const seps = s.replace(/[0-9]/g, ""); + const codes: string[] = []; + for (let i = 0; i < seps.length; i++) { + codes.push("U+" + seps.charCodeAt(i).toString(16).toUpperCase().padStart(4, "0")); + } + console.log(loc + " " + JSON.stringify(s) + " " + codes.join(",")); +} + +// The same table through `Intl.NumberFormat`, which shares the resolver, with +// a fractional value so the DECIMAL separator is exercised too — `fr` groups +// with U+202F and separates decimals with a comma, so a locale that got the +// group right and the decimal wrong would still pass the integer-only rows. +for (const loc of ["en-US", "de-DE", "fr-FR", "ru-RU", "nl-NL"]) { + console.log("nf:" + loc + " " + JSON.stringify(new Intl.NumberFormat(loc).format(1234567.891))); +}