Replace malformed unicode error helper calls with the _real variants#8359
Conversation
new_unicode_decode_error / new_unicode_encode_error build the exception from a bare message without running the initializer, so the result has none of the five attributes a unicode error must carry and str() renders as an empty string. Convert the non-Windows call sites in csv, socket, getlogin and the fs-path decoders to the _real constructors, passing the source bytes/str and the failing offset from the captured Utf8Error. The Windows-gated sites (nt, mbcs/oem codecs) and the sites whose source object is not reachable (uname, array) are left for follow-ups. Assisted-by: Claude Code:claude-fable-5
📝 WalkthroughWalkthroughUnicode conversion failures now preserve original bytes, encoding context, and invalid positions when constructing decode or encode exceptions across CSV, socket, filesystem, OS, and POSIX paths. ChangesUnicode Error Reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/stdlib/src/csv.rs`:
- Around line 63-76: Update new_not_utf8_error to calculate the decode-error end
offset from err.error_len(): use err.valid_up_to() plus the reported length when
available, and bytes.len() for truncated sequences when it is None. Pass this
computed end offset to vm.new_unicode_decode_error_real instead of always using
err.valid_up_to() + 1, preserving the complete invalid UTF-8 span.
In `@crates/stdlib/src/socket.rs`:
- Around line 2615-2622: Update both host and port UTF-8 error handlers around
the host and port from_utf8 calls in crates/stdlib/src/socket.rs:2615-2622 and
crates/stdlib/src/socket.rs:2656-2664 to calculate the UnicodeDecodeError end
position using Utf8Error::error_len(), falling back to the remaining input
length when needed, instead of always using valid_up_to() + 1.
In `@crates/vm/src/function/fspath.rs`:
- Around line 128-136: Update the UTF-8 error mappings in
crates/vm/src/function/fspath.rs:128-136, crates/vm/src/stdlib/os.rs:133-141,
and crates/vm/src/stdlib/posix.rs:1734-1742 to use the complete failure span:
derive the end offset from e.error_len(), falling back to the source slice
length when it is None, and use the source length for unexpected EOF instead of
valid_up_to() + 1.
🪄 Autofix (Beta)
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 41bb1195-e3a9-4784-913d-1463dff8e1a0
📒 Files selected for processing (5)
crates/stdlib/src/csv.rscrates/stdlib/src/socket.rscrates/vm/src/function/fspath.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rs
moreal
left a comment
moreal
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Could you see the Coderabbit's reviews about Utf8Error::error_len()?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/vm/vm_new.rs (1)
541-548: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for both UTF-8 span cases.
Cover invalid sequences where
error_len()isSome, truncated sequences where it isNone, and assert the resultingencoding,object,start,end, andreasonattributes. The supplied context does not include these tests, so please verify they exist in the VM test suite.🤖 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 `@crates/vm/src/vm/vm_new.rs` around lines 541 - 548, Add regression tests in the VM test suite for UTF-8 decode errors covering both invalid sequences with error_len() = Some and truncated sequences with error_len() = None. Assert each resulting error’s encoding, object, start, end, and reason attributes, including the expected span boundaries for both cases.
🤖 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 `@crates/vm/src/vm/vm_new.rs`:
- Around line 541-548: Add regression tests in the VM test suite for UTF-8
decode errors covering both invalid sequences with error_len() = Some and
truncated sequences with error_len() = None. Assert each resulting error’s
encoding, object, start, end, and reason attributes, including the expected span
boundaries for both cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef508b01-1f91-4b5e-a1f4-f4783ab06334
📒 Files selected for processing (6)
crates/stdlib/src/csv.rscrates/stdlib/src/socket.rscrates/vm/src/function/fspath.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rscrates/vm/src/vm/vm_new.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/stdlib/posix.rs
- crates/stdlib/src/socket.rs
The conversions used valid_up_to() + 1 for the decode-error end offset, which under-reports multi-byte invalid sequences. Take the span from the Utf8Error instead: valid_up_to() + error_len(), or the input length for a truncated sequence (error_len() == None), matching CPython. Assisted-by: Claude Code:claude-opus-4-8
47b6766 to
b63c408
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
63-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both invalid UTF-8 span cases.
Please ensure tests cover a malformed sequence where
error_len()isSome(n)and a truncated sequence where it isNone, asserting the resulting exception’sobject,start, andendattributes. As per coding guidelines, runcargo fmtandcargo clippybefore completion.🤖 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 `@crates/stdlib/src/csv.rs` around lines 63 - 77, Add regression tests for new_not_utf8_error covering both malformed UTF-8 with error_len() = Some(n) and truncated UTF-8 with error_len() = None, asserting each resulting exception’s object, start, and end attributes. Run cargo fmt and cargo clippy after implementing the tests.Source: Coding guidelines
🤖 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 `@crates/stdlib/src/csv.rs`:
- Around line 63-77: Add regression tests for new_not_utf8_error covering both
malformed UTF-8 with error_len() = Some(n) and truncated UTF-8 with error_len()
= None, asserting each resulting exception’s object, start, and end attributes.
Run cargo fmt and cargo clippy after implementing the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3aa8e043-87c1-4675-9fe2-44935fa949b8
📒 Files selected for processing (5)
crates/stdlib/src/csv.rscrates/stdlib/src/socket.rscrates/vm/src/function/fspath.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/vm/src/function/fspath.rs
- crates/vm/src/stdlib/posix.rs
- crates/vm/src/stdlib/os.rs
- crates/stdlib/src/socket.rs
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
looks good in general.
If you are interested in further development, there is a common pattern for new_unicode_decode_error_real using utf-8 and error info. then creating another wrapper for that kind of simple case will be helpful. e.g. new_utf8_decode_error
| .as_wtf8() | ||
| .code_points() | ||
| .position(|c| c.to_char().is_none()) | ||
| .unwrap(); |
There was a problem hiding this comment.
| .as_wtf8() | |
| .code_points() | |
| .position(|c| c.to_char().is_none()) | |
| .unwrap(); | |
| .as_bytes() | |
| // match surrogate bytes directly using bytes may also work |
|
Thank you! |
3 participants
Part of #8352
Summary
new_unicode_decode_error/new_unicode_encode_errorconstruct a structurally invalid exception: it has none of the five attributes a unicode error must carry (encoding,object,start,end,reason), itsargsis a 1-tuple message, andstr()renders as an empty string. This converts the non-Windows call sites to the_realconstructors:Cause
The broken helpers are generated by
define_exception_fn!, which goes throughnew_exception_msg→PyBaseException::new(args). That only storesargsand never runs the type's initializer, so the unicode-error attributes are never set. The_realconstructors pass the full(encoding, object, start, end, reason)and set every attribute.Fix
Converted call sites, supplying the source bytes/str and the failing offset from the previously discarded
Utf8Error(valid_up_to(), spanstart..start + 1matching the existing_realusages):csv: all 6 sites, via anew_not_utf8_errorhelper next to the existingnew_csv_errorsocket: host/port bytes decode; the port surrogate check now reports the first surrogate position (same scan asPyStr::ensure_valid_utf8)posix.getlogin,FsPath::bytes_as_os_str,os::bytes_as_os_strLeft for follow-ups, per the issue plan:
nt.rs, mbcs/oem codecs) — I can't test Windows locallyuname,array) — these need a small refactor or an error-type decision rather than a mechanical conversionTest
UnicodeDecodeError; also verifiedsocket.getaddrinfo(b"\xff\xfe", 80)→('utf-8', b'\xff\xfe', 0, 1, ...)andsocket.getaddrinfo("localhost", "ab\udcffcd")→UnicodeEncodeError ('utf-8', 'ab\udcffcd', 2, 3, 'surrogates not allowed')(surrogate position reported precisely).cargo run -- -m test test_csv/test_os/test_posix→Tests result: SUCCESS(no unexpected successes).cargo clippy/cargo fmt: clean (no new warnings).Assisted-by: Claude Code:claude-fable-5
Summary by CodeRabbit