Repository navigation
Update locale from v3.14.3 - #7348
Merged
Merged
Conversation
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces locale-aware Python number formatting support by adding locale detection infrastructure, extending FormatSpec with locale-specific formatting methods, and integrating locale-aware paths into int, float, and complex Changes
Sequence DiagramsequenceDiagram
actor User
participant PyBuiltin as PyInt/PyFloat/PyComplex
participant FormatSpec
participant LocaleLogic as Locale Detection &<br/>Retrieval
participant Formatter as Locale-Aware<br/>Formatter
participant Result as Formatted String
User->>PyBuiltin: __format__(spec_str)
PyBuiltin->>FormatSpec: parse(spec_str)
FormatSpec-->>PyBuiltin: parsed spec
PyBuiltin->>FormatSpec: has_locale_format()
alt Locale Formatting Detected
FormatSpec-->>PyBuiltin: true
PyBuiltin->>LocaleLogic: get_locale_info()
LocaleLogic-->>PyBuiltin: LocaleInfo (thousands_sep, decimal_point, grouping)
PyBuiltin->>Formatter: format_*_locale(num, locale_info)
Formatter->>Formatter: apply grouping & substitution
Formatter-->>Result: locale-formatted string
else Standard Formatting
FormatSpec-->>PyBuiltin: false
PyBuiltin->>Formatter: format_*(num)
Formatter-->>Result: standard formatted string
end
Result-->>User: formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/locale.py dependencies:
dependent tests: (95 tests)
Legend:
|
youknowone
force-pushed
the
upgrade/locale
branch
from
March 4, 2026 23:54
be96da9 to
3e9df88
Compare
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin upgrade/locale |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/vm/src/format.rs (1)
57-62: Potential portability issue withc_char::MAXcheck.On platforms where
c_charis signed (e.g., x86 Linux),c_char::MAXis 127. On platforms where it's unsigned (e.g., ARM), it's 255. The C standard specifies thatCHAR_MAXin the grouping array terminates grouping, but this value differs by platform. The current check should work correctly since you're comparing againstlibc::c_char::MAX, but verify this matches CPython's behavior across platforms.Additionally, the inner
unsafeblock on line 57 is redundant since the outer function body is already within anunsafeblock.Remove redundant unsafe block
unsafe fn parse_grouping(grouping: *const libc::c_char) -> Vec<u8> { let mut result = Vec::new(); if grouping.is_null() { return result; } - unsafe { - let mut ptr = grouping; - while ![0, libc::c_char::MAX].contains(&*ptr) { - result.push(*ptr as u8); - ptr = ptr.add(1); - } + let mut ptr = grouping; + while ![0, libc::c_char::MAX].contains(&*ptr) { + result.push(*ptr as u8); + ptr = ptr.add(1); } if !result.is_empty() { result.push(0); } result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/format.rs` around lines 57 - 62, The loop currently uses an inner unsafe and compares against libc::c_char::MAX which can be platform-dependent; remove the redundant inner "unsafe" block (the function is already unsafe) and change the termination check to match CPython's behavior by comparing *ptr against 0 and the C CHAR_MAX sentinel (use libc::CHAR_MAX rather than libc::c_char::MAX), keeping the existing pointer iteration (grouping, ptr) and result.push(*ptr as u8) logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/format.rs`:
- Around line 12-38: get_locale_info currently calls the non-thread-safe C
localeconv() directly; change it to use a per-thread cache via thread_local! so
each thread calls localeconv() once and stores a copied LocaleInfo (strings and
grouping) to avoid races. Specifically, introduce a thread_local static (e.g.,
CACHED_LOCALE) holding Option<LocaleInfo>, update get_locale_info to check that
cache and return a clone if present, otherwise call localeconv() inside the
unsafe block, build the LocaleInfo using parse_grouping and owned strings, store
it in the thread-local cache, and return it; alternatively on glibc you can
fallback to using localeconv_l with a per-thread locale object if available.
- Around line 25-30: The code dereferences (*lc).thousands_sep and
(*lc).decimal_point without null checks; add checks before calling
CStr::from_ptr so you don't call it on a null pointer. For example, inspect
(*lc).thousands_sep and (*lc).decimal_point for .is_null() and if null assign a
safe default String (e.g., empty string or a sensible default like "." for
decimal_point) otherwise call
CStr::from_ptr(...).to_string_lossy().into_owned(); update the two lines using
thousands_sep and decimal_point accordingly (keeping the existing variable
names).
---
Nitpick comments:
In `@crates/vm/src/format.rs`:
- Around line 57-62: The loop currently uses an inner unsafe and compares
against libc::c_char::MAX which can be platform-dependent; remove the redundant
inner "unsafe" block (the function is already unsafe) and change the termination
check to match CPython's behavior by comparing *ptr against 0 and the C CHAR_MAX
sentinel (use libc::CHAR_MAX rather than libc::c_char::MAX), keeping the
existing pointer iteration (grouping, ptr) and result.push(*ptr as u8) logic
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9f9d8854-834d-48c8-9e30-ab3832f04f71
⛔ Files ignored due to path filters (4)
Lib/locale.pyis excluded by!Lib/**Lib/test/test_format.pyis excluded by!Lib/**Lib/test/test_locale.pyis excluded by!Lib/**Lib/test/test_types.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/common/src/format.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/format.rs
Comment on lines
+12
to
+38
| /// Get locale information from C `localeconv()` for the 'n' format specifier. | ||
| #[cfg(unix)] | ||
| pub(crate) fn get_locale_info() -> LocaleInfo { | ||
| use core::ffi::CStr; | ||
| unsafe { | ||
| let lc = libc::localeconv(); | ||
| if lc.is_null() { | ||
| return LocaleInfo { | ||
| thousands_sep: String::new(), | ||
| decimal_point: ".".to_string(), | ||
| grouping: vec![], | ||
| }; | ||
| } | ||
| let thousands_sep = CStr::from_ptr((*lc).thousands_sep) | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| let decimal_point = CStr::from_ptr((*lc).decimal_point) | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| let grouping = parse_grouping((*lc).grouping); | ||
| LocaleInfo { | ||
| thousands_sep, | ||
| decimal_point, | ||
| grouping, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Thread safety: localeconv() is not thread-safe.
The C localeconv() function is not required to be thread-safe by POSIX. It returns a pointer to static storage that can be overwritten by subsequent calls to localeconv() or setlocale(). Since get_locale_info() is called from __format__ on int/float/complex without synchronization (as shown in the context snippets), concurrent formatting operations may cause data races or corrupt locale data.
Consider one of these mitigations:
- Cache the locale info at startup or per-thread using
thread_local! - Use a mutex to serialize access to
localeconv() - On glibc, use
localeconv_l()with a per-thread locale object
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/format.rs` around lines 12 - 38, get_locale_info currently
calls the non-thread-safe C localeconv() directly; change it to use a per-thread
cache via thread_local! so each thread calls localeconv() once and stores a
copied LocaleInfo (strings and grouping) to avoid races. Specifically, introduce
a thread_local static (e.g., CACHED_LOCALE) holding Option<LocaleInfo>, update
get_locale_info to check that cache and return a clone if present, otherwise
call localeconv() inside the unsafe block, build the LocaleInfo using
parse_grouping and owned strings, store it in the thread-local cache, and return
it; alternatively on glibc you can fallback to using localeconv_l with a
per-thread locale object if available.
Comment on lines
+25
to
+30
| let thousands_sep = CStr::from_ptr((*lc).thousands_sep) | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| let decimal_point = CStr::from_ptr((*lc).decimal_point) | ||
| .to_string_lossy() | ||
| .into_owned(); |
Contributor
There was a problem hiding this comment.
Missing null checks for thousands_sep and decimal_point pointers.
While you check if lc is null, the thousands_sep and decimal_point fields within the lconv struct could also be null pointers on some platforms or in edge cases. Dereferencing them with CStr::from_ptr() would cause undefined behavior.
Add null checks for string fields
let lc = libc::localeconv();
if lc.is_null() {
return LocaleInfo {
thousands_sep: String::new(),
decimal_point: ".".to_string(),
grouping: vec![],
};
}
- let thousands_sep = CStr::from_ptr((*lc).thousands_sep)
- .to_string_lossy()
- .into_owned();
- let decimal_point = CStr::from_ptr((*lc).decimal_point)
- .to_string_lossy()
- .into_owned();
+ let thousands_sep = if (*lc).thousands_sep.is_null() {
+ String::new()
+ } else {
+ CStr::from_ptr((*lc).thousands_sep)
+ .to_string_lossy()
+ .into_owned()
+ };
+ let decimal_point = if (*lc).decimal_point.is_null() {
+ ".".to_string()
+ } else {
+ CStr::from_ptr((*lc).decimal_point)
+ .to_string_lossy()
+ .into_owned()
+ };
let grouping = parse_grouping((*lc).grouping);📝 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
| let thousands_sep = CStr::from_ptr((*lc).thousands_sep) | |
| .to_string_lossy() | |
| .into_owned(); | |
| let decimal_point = CStr::from_ptr((*lc).decimal_point) | |
| .to_string_lossy() | |
| .into_owned(); | |
| let lc = libc::localeconv(); | |
| if lc.is_null() { | |
| return LocaleInfo { | |
| thousands_sep: String::new(), | |
| decimal_point: ".".to_string(), | |
| grouping: vec![], | |
| }; | |
| } | |
| let thousands_sep = if (*lc).thousands_sep.is_null() { | |
| String::new() | |
| } else { | |
| CStr::from_ptr((*lc).thousands_sep) | |
| .to_string_lossy() | |
| .into_owned() | |
| }; | |
| let decimal_point = if (*lc).decimal_point.is_null() { | |
| ".".to_string() | |
| } else { | |
| CStr::from_ptr((*lc).decimal_point) | |
| .to_string_lossy() | |
| .into_owned() | |
| }; | |
| let grouping = parse_grouping((*lc).grouping); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/format.rs` around lines 25 - 30, The code dereferences
(*lc).thousands_sep and (*lc).decimal_point without null checks; add checks
before calling CStr::from_ptr so you don't call it on a null pointer. For
example, inspect (*lc).thousands_sep and (*lc).decimal_point for .is_null() and
if null assign a safe default String (e.g., empty string or a sensible default
like "." for decimal_point) otherwise call
CStr::from_ptr(...).to_string_lossy().into_owned(); update the two lines using
thousands_sep and decimal_point accordingly (keeping the existing variable
names).
youknowone
force-pushed
the
upgrade/locale
branch
from
March 5, 2026 16:12
3142f99 to
cdc21c8
Compare
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
Lib/locale.pyfrom CPython v3.14.3remodule import for faster startup (movedreimport from module-level toformat_string())Lib/test/test_locale.pyfrom CPython v3.14.3LazyImportTest(cpython_only, skipped in RustPython)is_emscripten or is_wasichecks withlinked_to_musl()test_defaults_UTF8to useEnvironmentVarGuard@expectedFailurefrom 2 tests intest_types.pythat now pass thanks to the lazyreimport fix:test_float__format__localetest_int__format__localeTest plan
test_localeandtest__localepasstest_typespasses (2 previously failing tests now pass)test_builtin,test_calendar,test_decimal,test_float,test_format,test_inspect,test_io,test_re,test_strftime,test_sys,test_types,test_gettext,test_argparse,test_getopt,test_optparse,test_siteSummary by CodeRabbit