type_methods: read the locale for the 'n' presentation type - #1147
Conversation
`format(x, 'n')` never consulted the locale. The integer path inserted a separator only inside `if let Some(separator) = p.grouping`, which `'n'` never enters — it carries no `,` or `_` — and the group size came from a per-radix constant with no channel for a locale's grouping vector. The float path delegated render, group and pad end-to-end to the shared engine, which treats `'n'` as `'g'`. Under `LC_ALL=en_US.UTF-8`, `format(123456789, 'n')` was `'123456789'` and `format(1234.5, 'n')` was `'1234.5'`. Port the three routines upstream splits this across. `_get_locale` (`newformat.py:642-658`) branches on the presentation code: `'n'` takes the current locale, an explicit `,`/`_` takes that separator at a fixed group size — four for the power-of-two radices, three otherwise — and anything else carries a stop sentinel. `_group_digits` (`:738-778`) and `_fill_digits` (`:727-736`) perform the insertion for all of those, so `separate_integer_digits` goes away and the `,`/`_` specs travel the same route as `'n'`. The zero fill of a `0=` spec belongs inside the grouping rather than after it: `format(1234, '012n')` is `'0,000,001,234'`, thirteen characters from a width of twelve, because the padding digits are separated like any other. That is what `_calc_num_width`'s `n_min_width` (`:695-704`) carries, and both call sites compute it — `width - (sign + prefix)` for integers, `width - (sign + decimal point + remainder)` for floats, matching `extra_length`. The float path grows the split `_format_float` performs (`:1004-1044`): render unpadded, take the sign off, separate the integer digit run alone, re-emit the decimal point as the locale's, and leave the fraction and any exponent in the remainder, so `format(1e300, 'n')` keeps its exponent ungrouped. Zero padding is rejected for complex specs, so the per-lane `complex_component_spec` split reaches `_group_digits` only at `n_min_width` 0 and needs nothing further. `numeric_formatting` (`rlocale.py:173-178`) is new, placed beside the `_locale` module port so it shares that module's raw `localeconv()` walk: the grouping `format(x, 'n')` separates by and the grouping `locale.localeconv()` reports come out of one read. The walk keeps a `CHAR_MAX` element, which `rustpython_host_env::locale`'s reader drops — dropping it collapses "stop" onto "repeat the last group". Off unix, without `host_env`, and under sandbox the C locale's values stand in; upstream declares `localeconv` `sandboxsafe=True` and reads the host locale even there, but pyre's sandbox build replaces `_locale`'s host entry points with raising stubs and `format()` must not acquire a raising path. Six unit tests pin `_group_digits` against values taken from the vendored source: the zero-fill widths, the repeat-the-last-group rule, the stop sentinel, and a multi-byte separator surviving the buffer reverse. None are covered by `test_format.test_locale`, which asserts only that the separator appears — the test that made this visible when #1138 promoted its module into the gated set. Assisted-by: Claude
WalkthroughThe change adds locale numeric metadata access and shared grouping logic. Integer formatting now uses locale grouping and separator data. Float formatting with presentation type ChangesLocale-aware numeric formatting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Formatter as Integer or float formatter
participant Locale as get_locale
participant RLocale as numeric_formatting()
participant Host as libc::localeconv()
Formatter->>Locale: request locale grouping metadata
Locale->>RLocale: read numeric formatting
RLocale->>Host: call localeconv()
Host-->>RLocale: return locale pointers
RLocale-->>Locale: return separators and grouping
Locale-->>Formatter: provide grouping metadata
Formatter->>Formatter: group digits and apply padding
Poem
🚥 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b489a84). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b489a84e865ccfe7d9acf7af481e97ba95f8422b/pyre-interpreter/src/type_methods.rs#L2348-L2349
When LC_NUMERIC uses a legacy non-UTF-8 encoding and its decimal point or thousands separator contains a high byte (for example an NBSP encoded as 0xA0), from_utf8_lossy replaces that byte with U+FFFD, so format(value, 'n') emits replacement characters instead of the locale's separator. Decode these values using the active locale codeset rather than lossy UTF-8; the current conversion is also a deliberate divergence from the required upstream structural parity.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/_locale/interp_locale.rs (1)
206-221: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRead all locale fields from one
localeconv()snapshot.
rustpython_host_env::locale::localeconv_data()preserves separator bytes correctly. Ifsetlocale()changes the locale between itslocaleconv()call and the laterlibc::localeconv()call, the result combines fields from different locales. Reuse one snapshot for all fields.🤖 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 `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs` around lines 206 - 221, Update the locale-data construction around the grouping_of closure to obtain one rustpython_host_env::locale::localeconv_data() snapshot and derive every locale field from it, including grouping bytes, decimal/thousands separators, and monetary fields. Remove the later direct libc::localeconv() read so all returned values come from the same snapshot.
🤖 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 `@pyre/pyre-interpreter/src/type_methods.rs`:
- Around line 2420-2451: Update group_digits and the corresponding vendored
_group_digits implementation to stop immediately when loc_grouping is empty, or
normalize it to the existing 0xFF terminator representation before the loop.
Preserve normal grouping behavior and add a regression test covering a non-empty
loc_thousands with empty loc_grouping, verifying termination without repeated
separators or unbounded output.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs`:
- Around line 206-221: Update the locale-data construction around the
grouping_of closure to obtain one rustpython_host_env::locale::localeconv_data()
snapshot and derive every locale field from it, including grouping bytes,
decimal/thousands separators, and monetary fields. Remove the later direct
libc::localeconv() read so all returned values come from the same snapshot.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7bb6b0f-1443-4f36-a9d1-f554e2194763
📒 Files selected for processing (4)
pyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/_locale/mod.rspyre/pyre-interpreter/src/module/_locale/rlocale.rspyre/pyre-interpreter/src/type_methods.rs
| loop { | ||
| let group = if grouping_state >= loc_grouping.len() { | ||
| previous | ||
| } else { | ||
| let group = i32::from(loc_grouping[grouping_state]); | ||
| if group == 0xFF { | ||
| break; | ||
| } | ||
| grouping_state += 1; | ||
| previous = group; | ||
| group | ||
| }; | ||
| let final_grouping = group.min(left.max(min_width.max(1))); | ||
| let n_zeros = (final_grouping - left).max(0); | ||
| let n_chars = left.min(final_grouping).max(0); | ||
| fill_digits( | ||
| &mut buf, | ||
| digits, | ||
| left, | ||
| n_chars, | ||
| n_zeros, | ||
| need_separator.then_some(loc_thousands), | ||
| ); | ||
| need_separator = true; | ||
| left -= n_chars; | ||
| min_width -= final_grouping; | ||
| if left <= 0 && min_width <= 0 { | ||
| done = true; | ||
| break; | ||
| } | ||
| min_width -= n_ts; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the upstream _group_digits loop and the rlocale numeric_formatting source.
fd -t f 'newformat.py' --exec-batch rg -n -C6 'def _group_digits|def _get_locale|def _fill_digits|previous'
fd -t f 'rlocale.py' --exec-batch rg -n -C6 'def numeric_formatting|grouping'Repository: youknowone/pyre
Length of output: 9214
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Rust implementation and callers ---'
sed -n '2325,2465p' pyre/pyre-interpreter/src/type_methods.rs
printf '%s\n' '--- locale and grouping conversion ---'
sed -n '2500,2605p' pyre/pyre-interpreter/src/type_methods.rs
rg -n -C5 'numeric_formatting|loc_grouping|group_digits|format_rbigint|thousands_sep' pyre/pyre-interpreter/src pypy rpython
printf '%s\n' '--- upstream loop remainder ---'
sed -n '740,785p' pypy/objspace/std/newformat.py
printf '%s\n' '--- behavioral probe ---'
python3 - <<'PY'
def ported(grouping, thousands, left=4, min_width=0, limit=20):
state = 0
previous = 0
separators = 0
for iteration in range(limit):
if state >= len(grouping):
group = previous
else:
group = grouping[state]
if group == 0xFF:
return ("break", iteration, separators)
state += 1
previous = group
final_grouping = min(group, max(left, max(min_width, 1)))
n_chars = max(0, min(left, final_grouping))
left -= n_chars
min_width -= final_grouping
if left <= 0 and min_width <= 0:
return ("done", iteration + 1, separators)
min_width -= len(thousands)
separators += 1 if iteration else 0
return ("nonterminating-within-limit", limit, separators, left, min_width)
for grouping, thousands in [([], ","), ([], ""), ([3], ","), ([0xFF], ",")]:
print(grouping, repr(thousands), ported(grouping, thousands))
PYRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- compact upstream loop ---'
sed -n '747,785p' pypy/objspace/std/newformat.py
printf '%s\n' '--- host locale observations ---'
locale -a 2>/dev/null | head -50 || true
python3 - <<'PY'
import locale
print("localeconv:", locale.localeconv())
PY
printf '%s\n' '--- existing locale/format tests ---'
rg -n -C4 "format\(.*['\"]n|LC_NUMERIC|thousands_sep|grouping" \
pypy/objspace/std/test pyre/pyre-interpreter/src 2>/dev/null | head -250
printf '%s\n' '--- exact loop model ---'
python3 - <<'PY'
def model(grouping, separator, left=4, min_width=0, limit=8):
previous = 0
state = 0
for i in range(limit):
if state >= len(grouping):
group = previous
else:
group = grouping[state]
if group == 0xFF:
return "break"
state += 1
previous = group
final_grouping = min(group, max(left, max(min_width, 1)))
left -= max(0, min(left, final_grouping))
min_width -= final_grouping
if left <= 0 and min_width <= 0:
return ("done", i + 1)
min_width -= len(separator)
return ("no termination", left, min_width)
for grouping, separator in [([], ","), ([], ""), ([3], ","), ([0xFF], ",")]:
print(repr(grouping), repr(separator), model(grouping, separator))
PYRepository: youknowone/pyre
Length of output: 22603
Prevent non-termination for an empty locale grouping.
When get_locale('n', ...) returns a non-empty loc_thousands and an empty loc_grouping, group_digits repeatedly uses previous == 0. It never reduces left, appends another separator on each iteration, and grows memory without bound. The vendored _group_digits has the same defect, so preserve the stop behavior for this edge case by normalizing an empty grouping to vec![0xFF] or exiting before the loop. Add a regression test for this locale shape.
🤖 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 `@pyre/pyre-interpreter/src/type_methods.rs` around lines 2420 - 2451, Update
group_digits and the corresponding vendored _group_digits implementation to stop
immediately when loc_grouping is empty, or normalize it to the existing 0xFF
terminator representation before the loop. Preserve normal grouping behavior and
add a regression test covering a non-empty loc_thousands with empty
loc_grouping, verifying termination without repeated separators or unbounded
output.
1 participant
format(x, 'n')never consulted the locale. WithLC_ALL=en_US.UTF-8:This is the failure behind
test_format.test_locale, which #1138 moved into thegated set when it refreshed 91 baseline entries — the gate has been red on
mainsince (see #1138 (comment)).
Why it could not work
The integer path inserted a separator only inside
if let Some(separator) = p.grouping, which'n'never enters — it carries no,or_— and the group size came from a per-radix constant with no channelfor a locale's grouping vector. The float path delegated render, group and pad
end-to-end to the shared engine, which treats
'n'as'g'.The port
Upstream splits this across three routines and this ports all three:
get_localenewformat.py:642-658 _get_localegroup_digits:738-778 _group_digitsfill_digits:727-736 _fill_digitsnumeric_formattingrlocale.py:173-178(newmodule/_locale/rlocale.rs)_get_localebranches on the presentation code —'n'takes the currentlocale, an explicit
,/_takes that separator at a fixed group size (four forthe power-of-two radices, three otherwise), anything else carries a stop
sentinel — and one insertion routine serves all of them. So
separate_integer_digits(RustPython-shaped) goes away and the,/_specstravel the same route as
'n', which is why upstream has one routine and nottwo.
The zero fill of a
0=spec belongs inside the grouping, not after it.format(1234, '012n')is'0,000,001,234'— thirteen characters from a width oftwelve, because the padding digits are separated like any other. That is what
_calc_num_width'sn_min_width(:695-704) carries, and both call sites nowcompute it:
width - (sign + prefix)for integers,width - (sign + decimal point + remainder)for floats.The float path grows the split
_format_floatperforms (:1004-1044): renderunpadded, take the sign off, separate the integer digit run alone, re-emit the
decimal point as the locale's, leave the fraction and any exponent in the
remainder — so
format(1e300, 'n')keeps its exponent ungrouped. Zero padding isrejected for complex specs, so the per-lane
complex_component_specsplitreaches
_group_digitsonly atn_min_width0 and needs nothing further.numeric_formattingsits beside the_localemodule port so it shares thatmodule's raw
localeconv()walk: the groupingformat(x, 'n')separates by andthe grouping
locale.localeconv()reports come out of one read. The walk keeps aCHAR_MAXelement, whichrustpython_host_env::locale's reader drops —dropping it collapses "stop" onto "repeat the last group". Off unix, without
host_env, and under sandbox the C locale's values stand in; upstream declareslocaleconvsandboxsafe=Trueand reads the host locale even there, but pyre'ssandbox build replaces
_locale's host entry points with raising stubs andformat()must not acquire a raising path.Verification
two locales (
en_US.UTF-8andC), covering the regression paths as well:,/_, every radix,#prefixes, floats, complex, and theValueErrormessages.
PASS 200, FAIL 0, no regressions— the same 200 gatedmodules CI runs, i.e. the check that is currently red.
cargo test -p pyre-interpreter --features dynasm: 511 passed, 0 failed._group_digitsagainst values produced by running thevendored upstream source: the zero-fill widths, the repeat-the-last-group rule,
the stop sentinel, and a multi-byte separator surviving the buffer reverse.
test_format.test_localeasserts only that the separator appears, so it coversnone of those.
Full local
check.pyacross all three backends was still running when this wasopened; CI covers the same ground.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes