Repository navigation
Implement locale-aware 'n' format specifier for int, float, complex #7350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,19 @@ use rustpython_literal::format::Case; | |
|
|
||
| use crate::wtf8::{CodePoint, Wtf8, Wtf8Buf}; | ||
|
|
||
| /// Locale information for 'n' format specifier. | ||
| /// Contains thousands separator, decimal point, and grouping pattern | ||
| /// from the C library's `localeconv()`. | ||
| #[derive(Clone, Debug)] | ||
| pub struct LocaleInfo { | ||
| pub thousands_sep: String, | ||
| pub decimal_point: String, | ||
| /// Grouping pattern from `lconv.grouping`. | ||
| /// Each element is a group size. The last non-zero element repeats. | ||
| /// e.g. `[3, 0]` means groups of 3 repeating forever. | ||
| pub grouping: Vec<u8>, | ||
| } | ||
|
|
||
| trait FormatParse { | ||
| fn parse(text: &Wtf8) -> (Option<Self>, &Wtf8) | ||
| where | ||
|
|
@@ -460,6 +473,189 @@ impl FormatSpec { | |
| } | ||
| } | ||
|
|
||
| /// Returns true if this format spec uses the locale-aware 'n' format type. | ||
| pub fn has_locale_format(&self) -> bool { | ||
| matches!(self.format_type, Some(FormatType::Number(Case::Lower))) | ||
| } | ||
|
|
||
| /// Insert locale-aware thousands separators into an integer string. | ||
| /// Follows CPython's GroupGenerator logic for variable-width grouping. | ||
| fn insert_locale_grouping(int_part: &str, locale: &LocaleInfo) -> String { | ||
| if locale.grouping.is_empty() || locale.thousands_sep.is_empty() || int_part.len() <= 1 { | ||
| return int_part.to_string(); | ||
| } | ||
|
|
||
| let mut group_idx = 0; | ||
| let mut group_size = locale.grouping[0] as usize; | ||
|
|
||
| if group_size == 0 { | ||
| return int_part.to_string(); | ||
| } | ||
|
|
||
| // Collect groups of digits from right to left | ||
| let len = int_part.len(); | ||
| let mut groups: Vec<&str> = Vec::new(); | ||
| let mut pos = len; | ||
|
|
||
| loop { | ||
| if pos <= group_size { | ||
| groups.push(&int_part[..pos]); | ||
| break; | ||
| } | ||
|
|
||
| groups.push(&int_part[pos - group_size..pos]); | ||
| pos -= group_size; | ||
|
|
||
| // Advance to next group size | ||
| if group_idx + 1 < locale.grouping.len() { | ||
| let next = locale.grouping[group_idx + 1] as usize; | ||
| if next != 0 { | ||
| group_size = next; | ||
| group_idx += 1; | ||
| } | ||
| // 0 means repeat previous group size forever | ||
| } | ||
| } | ||
|
|
||
| // Groups were collected right-to-left, reverse to get left-to-right | ||
| groups.reverse(); | ||
| groups.join(&locale.thousands_sep) | ||
| } | ||
|
|
||
| /// Apply locale-aware grouping and decimal point replacement to a formatted number. | ||
| fn apply_locale_formatting(magnitude_str: String, locale: &LocaleInfo) -> String { | ||
| let mut parts = magnitude_str.splitn(2, '.'); | ||
| let int_part = parts.next().unwrap(); | ||
| let grouped = Self::insert_locale_grouping(int_part, locale); | ||
|
|
||
| if let Some(frac_part) = parts.next() { | ||
| format!("{grouped}{}{frac_part}", locale.decimal_point) | ||
| } else { | ||
| grouped | ||
| } | ||
| } | ||
|
|
||
| /// Format an integer with locale-aware 'n' format. | ||
| pub fn format_int_locale( | ||
| &self, | ||
| num: &BigInt, | ||
| locale: &LocaleInfo, | ||
| ) -> Result<String, FormatSpecError> { | ||
| self.validate_format(FormatType::Decimal)?; | ||
| let magnitude = num.abs(); | ||
|
|
||
| let raw_magnitude_str = match self.format_type { | ||
| Some(FormatType::Number(Case::Lower)) => self.format_int_radix(magnitude, 10), | ||
| _ => return self.format_int(num), | ||
| }?; | ||
|
|
||
| let magnitude_str = Self::apply_locale_formatting(raw_magnitude_str, locale); | ||
|
|
||
| let format_sign = self.sign.unwrap_or(FormatSign::Minus); | ||
| let sign_str = match num.sign() { | ||
| Sign::Minus => "-", | ||
| _ => match format_sign { | ||
| FormatSign::Plus => "+", | ||
| FormatSign::Minus => "", | ||
| FormatSign::MinusOrSpace => " ", | ||
| }, | ||
| }; | ||
|
|
||
| self.format_sign_and_align(&AsciiStr::new(&magnitude_str), sign_str, FormatAlign::Right) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's look at the AsciiStr implementation
rg -n "impl.*AsciiStr" crates/common/src/format.rs -A 30 | head -100Repository: RustPython/RustPython Length of output: 1208 🏁 Script executed: # Check the specific lines mentioned in the comment
sed -n '560,570p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 362 🏁 Script executed: # Also check lines 604 and 656
sed -n '600,610p' crates/common/src/format.rs
sed -n '652,662p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 774 🏁 Script executed: # Look for the char_len method definition
rg -n "fn char_len" crates/common/src/format.rs -A 5Repository: RustPython/RustPython Length of output: 305 🏁 Script executed: # Check how locale separators are inserted into the magnitude_str
rg -n "thousands_sep\|decimal_point" crates/common/src/format.rs -B 2 -A 2Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Look for locale definition and separator types
rg -n "struct.*Locale\|decimal_point\|thousands_sep" crates/common/src/ -A 3Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Search for Locale struct and locale-related code
find crates -name "*.rs" | xargs grep -l "locale\|Locale" | head -5Repository: RustPython/RustPython Length of output: 204 🏁 Script executed: # Search more broadly for where locale is used in format.rs
rg -n "locale\." crates/common/src/format.rs -B 2 -A 2Repository: RustPython/RustPython Length of output: 1144 🏁 Script executed: # Look for how magnitude_str is constructed
rg -n "magnitude_str.*format\|let.*magnitude_str" crates/common/src/format.rs -B 2 -A 2 | head -50Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Find LocaleInfo definition
rg -n "struct LocaleInfo\|^pub struct.*Locale" crates/common/src/format.rs -A 10Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Check the complete insert_locale_grouping function
sed -n '482,535p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 1970 🏁 Script executed: # Look at the format_float_locale function to see how it constructs magnitude_str
sed -n '567,600p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 1303 🏁 Script executed: # Check the locale module to understand the locale structure
head -100 crates/stdlib/src/locale.rsRepository: RustPython/RustPython Length of output: 3356 🏁 Script executed: # Find format_sign_and_align implementation
rg -n "fn format_sign_and_align" crates/common/src/format.rs -A 20Repository: RustPython/RustPython Length of output: 851 🏁 Script executed: # Check how char_len is used in format_sign_and_align
sed -n '900,1000p' crates/common/src/format.rs | grep -n "char_len\|width"Repository: RustPython/RustPython Length of output: 184 🏁 Script executed: # Find the complete format_sign_and_align function
rg -n "fn format_sign_and_align" crates/common/src/format.rs -A 30Repository: RustPython/RustPython Length of output: 1253 🏁 Script executed: # Let's look for where LocaleInfo is defined
rg -n "struct LocaleInfo\|pub.*LocaleInfo" crates/common/src/ -A 5Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Check if there are other implementations of CharLen
rg -n "impl CharLen" crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 101 🏁 Script executed: # Let's look for a Unicode-aware version or other CharLen implementations
rg -n "impl.*CharLen" crates/ -A 3Repository: RustPython/RustPython Length of output: 1007 🏁 Script executed: # Check if locales can actually have non-ASCII separators
rg -n "thousands_sep\|decimal_point" crates/stdlib/src/locale.rs -B 2 -A 2 | head -30Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Let me check the full context - look for where apply_locale_formatting is called and how its result is used
sed -n '567,610p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 1569 🏁 Script executed: # Let's verify that the magnitude_str can contain non-ASCII characters
# Check what types are used for thousands_sep and decimal_point
rg -n "pub struct LocaleInfo\|thousands_sep:\|decimal_point:" crates/common/src/ -A 2Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Let's trace through what happens - check if the string from apply_locale_formatting
# is used directly or if there's any normalization
sed -n '527,545p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 687 🏁 Script executed: # Check how sign_str.len() is used - does it account for multi-byte characters?
sed -n '985,990p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 331 🏁 Script executed: # Find the LocaleInfo struct definition
rg -n "^pub struct LocaleInfo\|^struct LocaleInfo" crates/common/src/format.rs -A 10Repository: RustPython/RustPython Length of output: 47 🏁 Script executed: # Check where LocaleInfo is created/initialized
rg -n "LocaleInfo {" crates/common/src/format.rs -B 2 -A 5Repository: RustPython/RustPython Length of output: 152 🏁 Script executed: # Let's trace how locale.thousands_sep is set - look at the locale module more carefully
sed -n '1,150p' crates/stdlib/src/locale.rs | grep -A 5 -B 5 "thousands_sep"Repository: RustPython/RustPython Length of output: 559 🏁 Script executed: # Search for LocaleInfo definition differently
rg "LocaleInfo" crates/common/src/format.rs | head -5Repository: RustPython/RustPython Length of output: 312 🏁 Script executed: # Get context around the first LocaleInfo reference
rg -n "LocaleInfo" crates/common/src/format.rs | head -1Repository: RustPython/RustPython Length of output: 91 🏁 Script executed: # Look for where LocaleInfo is defined - broader search
rg -n "struct LocaleInfo" crates/ -A 10Repository: RustPython/RustPython Length of output: 772 🏁 Script executed: # Now let's verify: check what sign_str can be and if it has the same byte-length issue
sed -n '590,605p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 611 🏁 Script executed: # Check the similar code in format_int_locale and format_complex_locale for the same issue
sed -n '620,650p' crates/common/src/format.rsRepository: RustPython/RustPython Length of output: 1283 🏁 Script executed: # Let's also verify if there are other CharLen implementations that correctly handle Unicode
sed -n '1520,1535p' crates/vm/src/builtins/str.rsRepository: RustPython/RustPython Length of output: 429 🏁 Script executed: # Final verification: check if French locale actually uses non-breaking space
python3 << 'EOF'
import locale
# Check what the French locale uses for thousands separator
# Note: this is for documentation purposes - we know from Unicode that:
# - \u{00A0} is non-breaking space (NBSP) - 2 bytes in UTF-8
# - French locale typically uses NBSP as thousands separator
# - This is confirmed by CPython behavior
print("UTF-8 byte length of non-breaking space: ", len("\u00A0".encode('utf-8')))
print("Character count of '1\u00A0234': ", len("1\u00A02 34"))
print("Byte length of '1\u00A0234': ", len("1\u00A0234".encode('utf-8')))
EOFRepository: RustPython/RustPython Length of output: 166 🏁 Script executed: # More direct verification
python3 << 'EOF'
nbsp_string = "1\u00A0234" # 1, NBSP, 2, 3, 4
print(f"String: {repr(nbsp_string)}")
print(f"Character count: {len(nbsp_string)}")
print(f"Byte count: {len(nbsp_string.encode('utf-8'))}")
print(f"Difference: {len(nbsp_string.encode('utf-8')) - len(nbsp_string)} bytes extra")
EOFRepository: RustPython/RustPython Length of output: 142
In 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /// Format a float with locale-aware 'n' format. | ||
| pub fn format_float_locale( | ||
| &self, | ||
| num: f64, | ||
| locale: &LocaleInfo, | ||
| ) -> Result<String, FormatSpecError> { | ||
| self.validate_format(FormatType::FixedPoint(Case::Lower))?; | ||
| let precision = self.precision.unwrap_or(6); | ||
| let magnitude = num.abs(); | ||
|
|
||
| let raw_magnitude_str = match &self.format_type { | ||
| Some(FormatType::Number(case)) => { | ||
| let precision = if precision == 0 { 1 } else { precision }; | ||
| Ok(float::format_general( | ||
| precision, | ||
| magnitude, | ||
| *case, | ||
| self.alternate_form, | ||
| false, | ||
| )) | ||
| } | ||
| _ => return self.format_float(num), | ||
| }?; | ||
|
|
||
| let magnitude_str = Self::apply_locale_formatting(raw_magnitude_str, locale); | ||
|
|
||
| let format_sign = self.sign.unwrap_or(FormatSign::Minus); | ||
| let sign_str = if num.is_sign_negative() && !num.is_nan() { | ||
| "-" | ||
| } else { | ||
| match format_sign { | ||
| FormatSign::Plus => "+", | ||
| FormatSign::Minus => "", | ||
| FormatSign::MinusOrSpace => " ", | ||
| } | ||
| }; | ||
|
|
||
| self.format_sign_and_align(&AsciiStr::new(&magnitude_str), sign_str, FormatAlign::Right) | ||
| } | ||
|
|
||
| /// Format a complex number with locale-aware 'n' format. | ||
| pub fn format_complex_locale( | ||
| &self, | ||
| num: &Complex64, | ||
| locale: &LocaleInfo, | ||
| ) -> Result<String, FormatSpecError> { | ||
| // Reuse format_complex_re_im with 'g' type to get the base formatted parts, | ||
| // then apply locale grouping. This matches CPython's format_complex_internal: | ||
| // 'n' → 'g', add_parens=0, skip_re=0. | ||
| let locale_spec = FormatSpec { | ||
| format_type: Some(FormatType::GeneralFormat(Case::Lower)), | ||
| ..*self | ||
| }; | ||
| let (formatted_re, formatted_im) = locale_spec.format_complex_re_im(num)?; | ||
|
|
||
| // Apply locale grouping to both parts | ||
| let grouped_re = if formatted_re.is_empty() { | ||
| formatted_re | ||
| } else { | ||
| // Split sign from magnitude, apply grouping, recombine | ||
| let (sign, mag) = if formatted_re.starts_with('-') | ||
| || formatted_re.starts_with('+') | ||
| || formatted_re.starts_with(' ') | ||
| { | ||
| formatted_re.split_at(1) | ||
| } else { | ||
| ("", formatted_re.as_str()) | ||
| }; | ||
| format!( | ||
| "{sign}{}", | ||
| Self::apply_locale_formatting(mag.to_string(), locale) | ||
| ) | ||
| }; | ||
|
|
||
| // formatted_im is like "+1234j" or "-1234j" or "1234j" | ||
| // Split sign, magnitude, and 'j' suffix | ||
| let im_str = &formatted_im; | ||
| let (im_sign, im_rest) = if im_str.starts_with('+') || im_str.starts_with('-') { | ||
| im_str.split_at(1) | ||
| } else { | ||
| ("", im_str.as_str()) | ||
| }; | ||
| let im_mag = im_rest.strip_suffix('j').unwrap_or(im_rest); | ||
| let im_grouped = Self::apply_locale_formatting(im_mag.to_string(), locale); | ||
| let grouped_im = format!("{im_sign}{im_grouped}j"); | ||
|
|
||
| // No parentheses for 'n' format (CPython: add_parens=0) | ||
| let magnitude_str = format!("{grouped_re}{grouped_im}"); | ||
|
|
||
| self.format_sign_and_align(&AsciiStr::new(&magnitude_str), "", FormatAlign::Right) | ||
| } | ||
|
Comment on lines
+607
to
+657
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At Line 656, the locale path returns formatted output without enforcing the same checks as 💡 Proposed fix // No parentheses for 'n' format (CPython: add_parens=0)
let magnitude_str = format!("{grouped_re}{grouped_im}");
-
- self.format_sign_and_align(&AsciiStr::new(&magnitude_str), "", FormatAlign::Right)
+ if let Some(FormatAlign::AfterSign) = &self.align {
+ return Err(FormatSpecError::AlignmentFlag);
+ }
+ match &self.fill.unwrap_or(' '.into()).to_char() {
+ Some('0') => Err(FormatSpecError::ZeroPadding),
+ _ => self.format_sign_and_align(&AsciiStr::new(&magnitude_str), "", FormatAlign::Right),
+ }🤖 Prompt for AI Agents |
||
|
|
||
| pub fn format_bool(&self, input: bool) -> Result<String, FormatSpecError> { | ||
| let x = u8::from(input); | ||
| match &self.format_type { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,64 @@ use crate::{ | |
| use crate::common::format::*; | ||
| use crate::common::wtf8::{Wtf8, Wtf8Buf}; | ||
|
|
||
| /// 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, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| pub(crate) fn get_locale_info() -> LocaleInfo { | ||
| LocaleInfo { | ||
| thousands_sep: String::new(), | ||
| decimal_point: ".".to_string(), | ||
| grouping: vec![], | ||
| } | ||
| } | ||
|
|
||
| /// Parse C `lconv.grouping` into a `Vec<u8>`. | ||
| /// Reads bytes until 0 or CHAR_MAX, then appends 0 (meaning "repeat last group"). | ||
| #[cfg(unix)] | ||
| 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); | ||
| } | ||
| } | ||
| if !result.is_empty() { | ||
| result.push(0); | ||
| } | ||
| result | ||
|
Comment on lines
+49
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: In
Notes:
Sources: Linux [1] (man7.org) [2] (man.netbsd.org) [3] (gnu.org) Citations:
🏁 Script executed: # First, let's check if there are any clippy or format issues with the current implementation
cd crates/vm/src && head -70 format.rs | tail -25Repository: RustPython/RustPython Length of output: 674 🏁 Script executed: cd crates/vm && grep -n "parse_grouping\|get_locale_info" src/format.rs | head -20Repository: RustPython/RustPython Length of output: 296 🏁 Script executed: # Verify the suggested fix more carefully by checking the actual current state
cd crates/vm/src && cat -n format.rs | sed -n '49,68p'Repository: RustPython/RustPython Length of output: 761
When the C grouping sequence ends with Suggested fix 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);
- }
- }
- if !result.is_empty() {
+ let mut ptr = grouping;
+ let mut repeat_last = false;
+ while !ptr.is_null() {
+ let v = *ptr;
+ if v == 0 {
+ repeat_last = true;
+ break;
+ }
+ if v == libc::c_char::MAX {
+ repeat_last = false;
+ break;
+ }
+ result.push(v as u8);
+ ptr = ptr.add(1);
+ }
+ if repeat_last && !result.is_empty() {
result.push(0);
}
result
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| impl IntoPyException for FormatSpecError { | ||
| fn into_pyexception(self, vm: &VirtualMachine) -> PyBaseExceptionRef { | ||
| match self { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.