| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
CPython rejects digit-only format-string field names that overflow Py_ssize_t at parse time with ValueError: Too many decimal digits in format string (Python/string_parser.c::get_integer). RustPython's FieldName::parse accepted any digit string usize::from_str could parse, producing IndexError or KeyError at lookup instead. Cap the parsed index at isize::MAX (Py_ssize_t::MAX on every platform) inside FieldName::parse. Also reject digits-only strings whose value overflows usize itself (caught when parse_usize returns None on an all-digit input). A new FormatParseError::TooManyDecimalDigits maps to the byte-identical CPython wording. Unmasks test_str.StrTest.test_format_huge_item_number.
📝 Walkthrough
WalkthroughAdds a new TooManyDecimalDigits error variant to FormatParseError enum. Updates FieldName::parse in the common format module to detect when digit-only field segments exceed integer limits and report them with this new error type. The VM layer's exception conversion is extended to map this error variant to a ValueError. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_str.py (TODO: 14) dependencies: dependent tests: (no tests depend on str) Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
crates/common/src/format.rs (1)🤖 Prompt for all review comments with AI agents1186-1204: Logic correctly mirrors CPython's get_integer and handles both overflow regimes.
The two-stage check is correct:
- If parse_usize succeeds, values in (isize::MAX, usize::MAX] are now rejected (this regime is otherwise reachable on 64-bit).
- If parse_usize fails on an all-ASCII-digit segment, it's a usize overflow and is also rejected.
Non-digit inputs (-5, +5 with current Rust semantics, leading whitespace, non-UTF-8 wtf8) still fall through to FieldType::Keyword exactly as before, so no regression.
Optional nit: consider adding unit tests next to test_parse_field_name (line 1729) for the three boundaries — isize::MAX (Index), isize::MAX + 1 (TooManyDecimalDigits), and a usize-overflowing digits-only string (TooManyDecimalDigits) — to lock the behavior in at this layer rather than relying solely on the unmasked test_format_huge_item_number.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/common/src/format.rs` around lines 1186 - 1204, The parsing branch correctly enforces two overflow regimes but lacks explicit unit tests locking this behavior; add tests near test_parse_field_name that assert parse_usize/field-name parsing returns FieldType::Index for isize::MAX, and returns FormatParseError::TooManyDecimalDigits for both isize::MAX + 1 and for a digits-only string that would overflow usize (to cover the second overflow branch that triggers the early error), referencing the existing parse logic around parse_usize, FieldType::Index, FormatParseError::TooManyDecimalDigits and FieldType::Keyword so future changes don’t regress this behavior.
Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@crates/common/src/format.rs`: - Around line 1186-1204: The parsing branch correctly enforces two overflow regimes but lacks explicit unit tests locking this behavior; add tests near test_parse_field_name that assert parse_usize/field-name parsing returns FieldType::Index for isize::MAX, and returns FormatParseError::TooManyDecimalDigits for both isize::MAX + 1 and for a digits-only string that would overflow usize (to cover the second overflow branch that triggers the early error), referencing the existing parse logic around parse_usize, FieldType::Index, FormatParseError::TooManyDecimalDigits and FieldType::Keyword so future changes don’t regress this behavior.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 591ff054-6b9c-455a-9866-62aa6b8606f6
📥 CommitsReviewing files that changed from the base of the PR and between 1d42ee5 and 455519f.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
There was a problem hiding this comment.
🔥
Sorry, something went wrong.
🔥🔥🔥 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Background
CPython rejects digit-only format-string field names that overflow Py_ssize_t at parse time with ValueError: Too many decimal digits in format string (Python/string_parser.c::get_integer). RustPython's FieldName::parse accepted any digit string usize::from_str could parse:
Repro
Fix
In FieldName::parse, after parse_usize returns the digit-only index, reject values above isize::MAX (matching CPython's Py_ssize_t::MAX). When parse_usize itself fails on a digits-only string (overflowing usize), raise the same error. A new FormatParseError::TooManyDecimalDigits maps to the byte-identical CPython wording.
The cap is isize::MAX as usize, which equals Py_ssize_t::MAX on every platform (i64 on 64-bit, i32 on 32-bit). Values up to isize::MAX parse successfully and only fail at lookup, matching CPython.
Nested [index] field-name parts ('{0[huge]}') have the same shape but no upstream regression test; left for a follow-up.
Tests unmasked
Verification
CPython 3.14.4 byte-identical for boundary cases:
Normal field names unchanged: {0}, {abc}, {0[1]}, {abc1} (digit in keyword), {007} (leading zeros), {0.attr}, dynamic '{0:{1}}'.format('x', 5).
No regressions across test_str, test_format, test_fstring.
Summary by CodeRabbit