literal: reject internal whitespace in complex::parse_str - #8242
Conversation
📝 WalkthroughWalkthroughModifies ChangesComplex Literal Whitespace Validation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
| // parentheses, never inside the numeric token. Reject it here so that | ||
| // `float::parse_str` (which tolerates surrounding whitespace on a part) | ||
| // does not let e.g. "1 +2j" through. | ||
| if s.contains(char::is_whitespace) { |
There was a problem hiding this comment.
Add a test in extra_tests
parse_str split the token then parsed each part with float::parse_str, which tolerates surrounding whitespace, so "1 +2j" parsed as (1+2j). Reject whitespace inside the token after stripping optional parentheses. Add unit tests and complex() snippet tests. Assisted-by: Claude
bc209d9 to
49edd70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extra_tests/snippets/builtin_complex.py (1)
176-178: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a test for whitespace inside parentheses within the numeric token.
The current tests cover internal whitespace rejection without parentheses (lines 176-178) and whitespace around parentheses (line 175), but don't test the interaction:
complex("(1 +2j)")where whitespace is inside parentheses and inside the numeric token. Per the PR logic, stripping parentheses yields"1 +2j"which should raiseValueError. Adding this case would directly validate the parentheses-stripping + whitespace-rejection interaction.💡 Suggested additional test case
assert_raises(ValueError, lambda: complex("1 + 2j")) +assert_raises(ValueError, lambda: complex("(1 +2j)")) +assert_raises(ValueError, lambda: complex("(1+ 2j)"))🤖 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 `@extra_tests/snippets/builtin_complex.py` around lines 176 - 178, Add a regression test in builtin_complex.py for complex("(1 +2j)") to cover whitespace inside parentheses and inside the numeric token. Extend the existing assert_raises(ValueError, ...) cases near the complex string parsing tests so this interaction is explicitly validated after parentheses stripping. Keep the new case alongside the other complex(...) whitespace rejection checks to make the intended behavior clear.
🤖 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.
Nitpick comments:
In `@extra_tests/snippets/builtin_complex.py`:
- Around line 176-178: Add a regression test in builtin_complex.py for
complex("(1 +2j)") to cover whitespace inside parentheses and inside the numeric
token. Extend the existing assert_raises(ValueError, ...) cases near the complex
string parsing tests so this interaction is explicitly validated after
parentheses stripping. Keep the new case alongside the other complex(...)
whitespace rejection checks to make the intended behavior clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bf15f579-9fc3-41e4-a208-6e778ea21f82
📒 Files selected for processing (2)
crates/literal/src/complex.rsextra_tests/snippets/builtin_complex.py
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/literal/src/complex.rs
1 participant
complex::parse_strsplits the token on the central+/-and parses each side withfloat::parse_str, which tolerates surrounding whitespace on a fragment. As a resultcomplex("1 +2j")parsed as(1+2j)instead of raising.Whitespace is only valid around the whole string and the optional parentheses, never inside the numeric token. Reject any whitespace remaining after the parentheses are stripped, matching CPython (
complex("1 +2j")→ValueError).Added unit tests: internal-whitespace rejection, surrounding/paren whitespace acceptance, and basic parsing (incl. underscores).
— assisted by Claude
Summary by CodeRabbit
Bug Fixes
1 +2j,2 -3j), while still allowing whitespace around the full literal and around optional outer parentheses.Tests