fix(intl): locale-aware group/decimal separators + zero-arg bigint toLocaleString grouping (#7429, #7428) - #7797
Merged
Conversation
proggeramlug
force-pushed
the
fix/7429-7428-locale-separators
branch
from
August 10, 2026 20:26
eed75fb to
e20e65f
Compare
📝 WalkthroughWalkthroughChangesLocale formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BigIntReceiver
participant object_proto
participant bigint_to_locale_string
participant locale_separators
BigIntReceiver->>object_proto: toLocaleString()
object_proto->>bigint_to_locale_string: undefined locale and options
bigint_to_locale_string->>locale_separators: resolve locale separators
locale_separators-->>bigint_to_locale_string: grouping and decimal separators
bigint_to_locale_string-->>object_proto: formatted BigInt string
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 2
🧹 Nitpick comments (1)
changelog.d/7797-locale-separators.md (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the fragment limited to shipped behavior.
Retain a concise user-facing summary. Remove the implementation history, Node version, validation commands, and unrelated intermittent-test detail from lines 3-13.
Based on learnings: changelog fragments must describe the final shipped behavior as one coherent release-note entry and must not include development-slice narratives.
🤖 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 `@changelog.d/7797-locale-separators.md` around lines 1 - 13, Condense the changelog fragment to one concise user-facing entry describing the shipped locale-specific separators and zero-argument BigInt toLocaleString grouping behavior. Remove implementation history, issue-analysis details, Node/version comparisons, test and validation commands, and unrelated intermittent-test information; retain only the final observable behavior.Source: Learnings
🤖 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 903-923: Update locale_separators in
crates/perry-runtime/src/intl/number_format.rs (lines 903-923) to parse regional
overrides and return CLDR separators for fr-CH (apostrophe grouping and "."
decimal), while preserving the existing fr-FR/fr-CA behavior and
primary-language fallback. Extend
test-files/test_gap_intl_locale_separators_7429_7428.ts (lines 33-48) with fr-CH
and the other required regional override cases to guard against regressions.
In `@crates/perry-runtime/src/object/native_call_method/object_proto.rs`:
- Around line 112-117: Update the BigInt branch in the native call path to use
the fast path only when BigInt.prototype.toLocaleString remains the intact
callable built-in; otherwise route zero-argument calls through normal property
dispatch so replacements, deletions, accessors, and non-callable values are
respected. Ensure explicit undefined follows the same override behavior as zero
arguments, and add regression coverage for replacement and deletion.
---
Nitpick comments:
In `@changelog.d/7797-locale-separators.md`:
- Around line 1-13: Condense the changelog fragment to one concise user-facing
entry describing the shipped locale-specific separators and zero-argument BigInt
toLocaleString grouping behavior. Remove implementation history, issue-analysis
details, Node/version comparisons, test and validation commands, and unrelated
intermittent-test information; retain only the final observable behavior.
🪄 Autofix
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: cf474558-b906-4bfd-a4ce-0cb29a78f5c7
📒 Files selected for processing (4)
changelog.d/7797-locale-separators.mdcrates/perry-runtime/src/intl/number_format.rscrates/perry-runtime/src/object/native_call_method/object_proto.rstest-files/test_gap_intl_locale_separators_7429_7428.ts
Comment on lines
+903
to
+923
| 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. | ||
| _ => (',', '.'), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve separator pairs by locale region where CLDR requires it.
locale_separators maps fr-CH to NBSP and ,, but CLDR specifies apostrophe grouping and . decimal for fr_CH. This makes both Number and BigInt locale formatting incorrect for that locale. (unicode.org)
crates/perry-runtime/src/intl/number_format.rs#L903-L923: parse and apply regional separator overrides beyondfr-FRandfr-CA, includingfr-CH.test-files/test_gap_intl_locale_separators_7429_7428.ts#L33-L48: addfr-CHand other regional overrides to prevent primary-language fallback regressions.
📍 Affects 2 files
crates/perry-runtime/src/intl/number_format.rs#L903-L923(this comment)test-files/test_gap_intl_locale_separators_7429_7428.ts#L33-L48
🤖 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 903 - 923,
Update locale_separators in crates/perry-runtime/src/intl/number_format.rs
(lines 903-923) to parse regional overrides and return CLDR separators for fr-CH
(apostrophe grouping and "." decimal), while preserving the existing fr-FR/fr-CA
behavior and primary-language fallback. Extend
test-files/test_gap_intl_locale_separators_7429_7428.ts (lines 33-48) with fr-CH
and the other required regional override cases to guard against regressions.
Comment on lines
+112
to
+117
| #[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()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-runtime/src/object/native_call_method/object_proto.rs \
--items all --match js_object_default_to_locale_string
rg -n -C 5 --glob '*.rs' \
'DateToLocaleString|js_object_default_to_locale_string|bigint_proto_to_locale_string_thunk|builtin_proto_user_method' \
crates
rg -n -C 4 --glob '*.ts' \
'BigInt\.prototype\.toLocaleString|toLocaleString\(' \
test-filesRepository: PerryTS/perry
Length of output: 49783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- object_proto.rs ---'
sed -n '60,180p' crates/perry-runtime/src/object/native_call_method/object_proto.rs
printf '%s\n' '--- common_methods.rs ---'
sed -n '430,515p' crates/perry-runtime/src/object/native_call_method/common_methods.rs
printf '%s\n' '--- proto_dispatch.rs ---'
sed -n '1,260p' crates/perry-runtime/src/object/native_call_method/proto_dispatch.rs
printf '%s\n' '--- BigInt thunk and installation ---'
sed -n '80,125p' crates/perry-runtime/src/object/primitive_proto_thunks.rs
sed -n '360,400p' crates/perry-runtime/src/object/primitive_proto_thunks.rs
printf '%s\n' '--- override helper ---'
sed -n '340,410p' crates/perry-runtime/src/object/native_call_method.rs
printf '%s\n' '--- relevant tests ---'
rg -n -C 5 --glob '*.ts' \
'BigInt\.prototype\.toLocaleString|toLocaleString\s*=\s*|delete\s+BigInt\.prototype' \
test-filesRepository: PerryTS/perry
Length of output: 27430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'pub unsafe fn js_native_call_method|unsafe fn js_native_call_method|fn js_native_call_method' \
crates/perry-runtime/src/object/native_call_method.rs \
crates/perry-runtime/src/object/native_call_method/*.rs
sed -n '300,360p' crates/perry-hir/src/lower/expr_call/url_date_instance.rs
rg -n -C 10 \
'primitive_proto_method_value|try_dispatch.*proto|js_native_call_method\(' \
crates/perry-runtime/src/object/native_call_method.rs \
crates/perry-runtime/src/object/native_call_method/*.rs
python3 - <<'PY'
from pathlib import Path
lowering = Path("crates/perry-hir/src/lower/expr_call/url_date_instance.rs").read_text()
runtime = Path("crates/perry-runtime/src/object/native_call_method/object_proto.rs").read_text()
common = Path("crates/perry-runtime/src/object/native_call_method/common_methods.rs").read_text()
checks = {
"zero_arg_lowering": 'if args.is_empty()' in lowering and
'Expr::DateToLocaleString(Box::new(date_expr))' in lowering,
"bigint_fast_path": 'if jsval.is_bigint()' in runtime and
'bigint_to_locale_string(receiver, undef, undef)' in runtime,
"generic_bigint_exemption": '"toLocaleString" if !jsval.is_bigint()' in common,
}
for name, value in checks.items():
print(f"{name}={value}")
if not all(checks.values()):
raise SystemExit("expected dispatch structure not found")
PYRepository: PerryTS/perry
Length of output: 49847
Preserve BigInt.prototype.toLocaleString dispatch.
This fast path bypasses BigInt.prototype.toLocaleString, so replacements, deletions, accessors, and non-callable values have no effect. Route the zero-argument form through normal property dispatch when the built-in method is not intact. Add regression tests for replacement and deletion. Ensure zero-argument and explicit-undefined calls have identical override behavior.
🤖 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/object/native_call_method/object_proto.rs` around
lines 112 - 117, Update the BigInt branch in the native call path to use the
fast path only when BigInt.prototype.toLocaleString remains the intact callable
built-in; otherwise route zero-argument calls through normal property dispatch
so replacements, deletions, accessors, and non-callable values are respected.
Ensure explicit undefined follows the same override behavior as zero arguments,
and add regression coverage for replacement and deletion.
pull Bot
pushed a commit
to Mu-L/perry
that referenced
this pull request
Aug 11, 2026
Covers the 32 PRs admin-merged in one pass (audited in principle at the maintainer's direction): PerryTS#7768 PerryTS#7772 PerryTS#7779 PerryTS#7784 PerryTS#7785 PerryTS#7786 PerryTS#7788 PerryTS#7789 PerryTS#7797 PerryTS#7798 PerryTS#7801 PerryTS#7802 PerryTS#7804 PerryTS#7805 PerryTS#7806 PerryTS#7807 PerryTS#7808 PerryTS#7810 PerryTS#7811 PerryTS#7815 PerryTS#7816 PerryTS#7818 PerryTS#7819 PerryTS#7820 PerryTS#7821 PerryTS#7822 PerryTS#7823 PerryTS#7824 PerryTS#7825 PerryTS#7826 PerryTS#7827 PerryTS#7828. (PerryTS#7787 closed as already-landed via the PerryTS#7786 stack.) Per-change history lives in each PR's changelog.d fragment as usual. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
This was referenced Aug 11, 2026
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.
toLocaleStringnow 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 whende-DEwas the only non-enlocale 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 —./,forde/es/it/pt/nl/tr, a space plus,forfr/ru/pl/nb/sv/fi/cs/hu/uk, and,/.foren/ja/ko/zh.locale_separatorsnow 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-FRgroups with U+202F (narrow no-break space) whilefr-CAuses 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, slicinglocale.get(..6)against the five-byte"fr-FR", which returnsNoneand 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, whiletoLocaleString(undefined)was already correct. Codegen lowers the zero-arg form toExpr::DateToLocaleString, which lands injs_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 — andBigInt.prototype.toStringhas 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.tscovers 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 exercisesIntl.NumberFormatwith 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.NumberFormatrows match, andtest_gap_intl(7) andtest_gap_bigint(4) stay green.cargo test -p perry-runtime --libis unchanged; the one intermittent failure seen during validation isgc::tests::root_words::bare_address_in_shadow_slot_survives_a_real_collection, which reproduces at 2/10 runs on cleanmainwith these changes reverted and rebuilt — it is #7365, not this change.Summary by CodeRabbit
Bug Fixes
BigInt.prototype.toLocaleString()to apply default-locale grouping consistently.Tests
Documentation