Integrate OSError creations into OSErrorBuilder #6443
Conversation
WalkthroughIntroduces an OSErrorBuilder and ToOSErrorBuilder trait in the exceptions module, then replaces scattered IOErrorBuilder usage across ospath, stdlib (io/os/posix), and vm_new with the unified OSErrorBuilder-based error construction. Changes
Sequence Diagram(s)sequenceDiagram
participant SysErr as std::io::Error
participant Module as Caller (io/os/posix/ospath)
participant Builder as OSErrorBuilder
participant VM as VirtualMachine / exceptions
participant PyExc as PyOSError (Python)
SysErr->>Module: error returned
Module->>Builder: error.to_os_error_builder(vm) / OSErrorBuilder::with_filename(...)
Builder->>Builder: attach errno/strerror/filename/(winerror)
Builder->>VM: build(vm) / into_pyexception(vm)
VM->>PyExc: construct PyOSError instance
VM-->>Module: PyBaseExceptionRef (upcast)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
b62019a to
9cef05b
Compare
9cef05b to
d7357f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/exceptions.rs (1)
1209-1214: Clean upOSErrorBuilder::with_subtypeand clarify builder invariantsThe builder is generally well‑structured, but there are a couple of small issues worth tightening:
- In
with_subtype,strerroris converted twice:let strerror = strerror.to_pyobject(vm); Self { exc_type, errno, strerror: Some(strerror.to_pyobject(vm)), // second conversion is redundant // ... }You can avoid the extra clone/allocation by storing the first conversion:
- ) -> Self { - let strerror = strerror.to_pyobject(vm); - Self { - exc_type, - errno, - strerror: Some(strerror.to_pyobject(vm)), + ) -> Self { + let strerror = strerror.to_pyobject(vm); + Self { + exc_type, + errno, + strerror: Some(strerror), filename: None, #[cfg(windows)] winerror: None, filename2: None, } }
buildrelies onstrerroralways being populated by the constructors, even though the field type isOption<PyObjectRef>. That’s fine as an internal invariant, but it would be safer either to:
- store
strerroras a plainPyObjectRef(required by the API), or- add a debug/assert to document the expectation before calling
strerror.to_pyobject(vm).When
errnoisNone, anyfilename/filename2set via the builder are currently ignored. That matches CPython’sOSError(msg)behavior (no filename fields) and current call sites, but it would be good to either document this invariant or early‑reject such combinations to avoid surprising future usages.
buildcallspy_new,into_ref_with_type, andslot_initwithexpect("new_os_error usage error"). If these paths can be hit with user‑controlled types in the future, consider returningPyResult<PyRef<PyOSError>>and letting callers decide whether to unwrap, to avoid panics in release builds.Also applies to: 1218-1257, 1265-1327
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/vm/src/exceptions.rs(3 hunks)crates/vm/src/ospath.rs(1 hunks)crates/vm/src/stdlib/io.rs(6 hunks)crates/vm/src/stdlib/os.rs(17 hunks)crates/vm/src/stdlib/posix.rs(9 hunks)crates/vm/src/vm/vm_new.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/io.rscrates/vm/src/ospath.rscrates/vm/src/stdlib/os.rscrates/vm/src/vm/vm_new.rscrates/vm/src/exceptions.rs
🧠 Learnings (2)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/ospath.rscrates/vm/src/vm/vm_new.rscrates/vm/src/exceptions.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/ospath.rscrates/vm/src/vm/vm_new.rs
🧬 Code graph analysis (6)
crates/vm/src/stdlib/posix.rs (1)
crates/vm/src/ospath.rs (1)
with_filename(147-157)
crates/vm/src/stdlib/io.rs (2)
crates/vm/src/exceptions.rs (9)
to_os_error_builder(1219-1219)with_errno(1252-1257)errno(1647-1648)errno(1649-1649)errno(1799-1801)winerror(1279-1282)winerror(1840-1842)filename(1266-1269)filename(1819-1821)crates/vm/src/ospath.rs (3)
with_filename(147-157)filename(85-87)filename(137-142)
crates/vm/src/ospath.rs (2)
crates/vm/src/stdlib/os.rs (2)
error(1565-1571)std(1211-1211)crates/vm/src/exceptions.rs (2)
filename(1266-1269)filename(1819-1821)
crates/vm/src/stdlib/os.rs (1)
crates/vm/src/ospath.rs (1)
with_filename(147-157)
crates/vm/src/vm/vm_new.rs (2)
crates/vm/src/exceptions.rs (4)
with_subtype(1234-1250)errno(1647-1648)errno(1649-1649)errno(1799-1801)crates/stdlib/src/socket.rs (1)
errno(1840-1846)
crates/vm/src/exceptions.rs (2)
crates/vm/src/stdlib/io.rs (3)
to_os_error_builder(22-67)vm(3519-3521)build(2332-2348)crates/vm/src/function/protocol.rs (1)
to_pyobject(184-186)
🔇 Additional comments (11)
crates/vm/src/stdlib/io.rs (5)
21-68: Well-structured platform-specific error handling.The implementation correctly handles platform-specific strerror logic with proper null pointer checks for the unsafe blocks. The
#[allow(unused_mut)]annotation on line 57 is necessary sincebuilderis only mutated on Windows.
70-74: Clean delegation to builder pattern.The
ToPyExceptionimplementation correctly delegates to the newToOSErrorBuildertrait, maintaining a consistent error construction flow.
4332-4335: Imports updated appropriately for the new builder pattern.
4531-4531: Consistent error construction with filename context.The
OSErrorBuilder::with_filenameusage correctly propagates the filename to the OSError, improving error messages for users.
4546-4560: Unified error handling across platform-specific branches.The error construction is consistent across Windows and Unix code paths, using
OSErrorBuilder::with_filenameto include the filename in all error cases. This provides better error messages compared to the previous approach.crates/vm/src/exceptions.rs (1)
1196-1197: Re-exporting OSErrorBuilder/ToOSErrorBuilder fromexceptionslooks goodThis makes the builder and trait conveniently available to the rest of the crate and matches how they’re consumed in other files.
crates/vm/src/vm/vm_new.rs (1)
2-15: UsingOSErrorBuilderinnew_os_subtype_errorkeeps behavior while centralizing constructionThe new
OSErrorBuilder::with_subtype(...).build(self)path preserves the previous semantics (including the basicsize debug‑assert onexc_type) while routing OS error creation through the shared builder. This looks correct for botherrno: None(OSError(msg)) andSome(errno)flows.Also applies to: 110-124
crates/vm/src/stdlib/posix.rs (1)
29-35: Consistent use ofOSErrorBuilder::with_filenamefor POSIX filesystem errorsAll these call sites now route
std::io::Error(or errors converted into it) throughOSErrorBuilder::with_filename, which:
- Preserves the original IO error’s errno and message via
ToOSErrorBuilder.- Attaches the relevant
OsPath/OsPathOrFdasfilename(andfilename2where applicable).- Returns a
PyBaseExceptionRefsuitable forPyResulterror types.The patterns in
access,_chmod,lchmod,rmdir,listdir,scandir,pathconf,truncate, andutime_implare all coherent and should yield better CPython‑styleOSErrorinstances with proper filename context.Also applies to: 415-417, 535-538, 1035-1036, 1097-1098, 360-363, 375-386, 861-871, 2130-2135, 1564-1571, 1339-1344, 1386-1403
crates/vm/src/stdlib/os.rs (2)
153-168: UnifiedOSErrorBuilderusage acrossosfilesystem operationsThe new imports and
map_errsites in this file consistently use:
OSErrorBuilder::with_filename(&err, path_or_fd, vm)for IO errors that originate from a single pathname/FD.vm.new_errno_error(...)(now backed byOSErrorBuilder) when constructing errno‑driven errors.OSErrorBuilder::with_filename(&io::Error::from(Errno::last()), path, vm)for errno‑basedpathconffailures.This centralizes OS error creation and ensures that
errno,strerror, andfilenameare wired the same way acrossopen,remove/unlink,mkdir,rmdir,listdir(path branch),readlink,stat/fstat,chdir,truncate,utime, andpathconf. The patterns are type‑correct and align with howOSErrorBuilder/ToOSErrorBuilderare intended to be used.Also applies to: 235-269, 290-323, 324-352, 359-363, 373-387, 546-553, 861-871, 1088-1092, 1118-1121, 1564-1571, 2130-2135
1128-1137:rename/replaceandlinknow produce fully populatedOSErrorwith both filenamesThe new error handling for
rename/replaceandlink:fs::rename(&src.path, &dst.path).map_err(|err| { let builder = err.to_os_error_builder(vm); let builder = builder.filename(src.filename(vm)); let builder = builder.filename2(dst.filename(vm)); builder.build(vm).upcast() })and similarly for
link, ensures that:
- The underlying IO error is converted via
to_os_error_builder, capturing errno and a platform‑appropriate message.filenameandfilename2are set tosrcanddst(respectingOutputModefor str/bytes).- The final Python exception is of the appropriate OSError subtype as determined by
errno_to_exc_type.This is a nice improvement over ad‑hoc construction and should match CPython’s behavior more closely.
Also applies to: 1221-1229
crates/vm/src/ospath.rs (1)
145-158:OSErrorBuilder::with_filenamehelper is well-factored and matches call sitesThe inherent helper:
pub(crate) fn with_filename<'a>( error: &std::io::Error, filename: impl Into<OsPathOrFd<'a>>, vm: &VirtualMachine, ) -> crate::builtins::PyBaseExceptionRef { use crate::exceptions::ToOSErrorBuilder; let builder = error.to_os_error_builder(vm); let builder = builder.filename(filename.into().filename(vm)); builder.build(vm).upcast() }nicely encapsulates the common “IO error + path →
OSErrorwith filename set” pattern used acrossosandposix. The API surface (taking anyInto<OsPathOrFd<'a>>) keeps call sites clean and avoids duplicating theto_os_error_builder().filename(...).build(vm)sequence everywhere.The TODO about returning
PyRef<PyOSError>instead ofPyBaseExceptionRefcan be addressed later without affecting current users.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/vm/src/stdlib/posix.rs (1)
534-538: Fix the nix::Error to i32 cast.The chown error handling incorrectly casts
nix::Errordirectly toi32:let err = io::Error::from_raw_os_error(err as i32);This cast is unsafe for non-errno
nix::Errorvariants (e.g.,InvalidPath,InvalidUtf8). Since nix supportsInto<io::Error>fornix::Error, use the conversion directly as done in the chroot implementation above:.map_err(|err| { - // Use `From<nix::Error> for io::Error` when it is available - let err = io::Error::from_raw_os_error(err as i32); + let io_err: io::Error = err.into(); - OSErrorBuilder::with_filename(&err, path, vm) + OSErrorBuilder::with_filename(&io_err, path, vm) })
🧹 Nitpick comments (1)
crates/vm/src/exceptions.rs (1)
1259-1263: Remove or uncomment the commented-out errno setter.The commented-out
errnosetter method at lines 1259-1263 should either be removed if not needed, or uncommented with a clear use case. Keeping dead commented code reduces maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/vm/src/exceptions.rs(3 hunks)crates/vm/src/stdlib/posix.rs(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/exceptions.rscrates/vm/src/stdlib/posix.rs
🧠 Learnings (3)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/exceptions.rs
📚 Learning: 2025-11-10T06:27:41.954Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.
Applied to files:
crates/vm/src/exceptions.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/vm/src/stdlib/posix.rs
🧬 Code graph analysis (2)
crates/vm/src/exceptions.rs (1)
crates/vm/src/stdlib/io.rs (3)
to_os_error_builder(22-67)vm(3519-3521)build(2332-2348)
crates/vm/src/stdlib/posix.rs (1)
crates/vm/src/ospath.rs (1)
with_filename(147-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
🔇 Additional comments (12)
crates/vm/src/exceptions.rs (5)
1196-1197: LGTM!The pub(crate) export makes OSErrorBuilder and ToOSErrorBuilder available throughout the crate while maintaining encapsulation.
1218-1220: LGTM!The ToOSErrorBuilder trait provides a clean abstraction for converting errors to the builder pattern.
1222-1230: LGTM!The OSErrorBuilder struct fields are well-designed with appropriate types and platform-specific conditional compilation.
1232-1321: LGTM overall!The builder pattern is correctly implemented with proper chaining, #[must_use] attributes, and appropriate error construction logic.
1323-1327: LGTM!The IntoPyException implementation correctly delegates to the build method and properly upcasts to PyBaseExceptionRef.
crates/vm/src/stdlib/posix.rs (7)
29-29: LGTM!The OSErrorBuilder import is correctly added to support the refactored error handling.
486-492: LGTM!The chroot error handling correctly converts
nix::Errortoio::Errorusing.into(), which properly handles both errno and non-errno error variants.
416-416: LGTM!The access function error handling is correctly updated to use OSErrorBuilder.
1035-1035: LGTM!The chmod error handling is correctly updated to use OSErrorBuilder with the filesystem error.
1097-1097: LGTM!The lchmod error handling correctly captures the last OS error and associates it with the path using OSErrorBuilder.
1558-1558: LGTM!The posix_spawn error handling correctly converts
nix::Errortoio::Errorusing.into()before passing to OSErrorBuilder.Also applies to: 1657-1657
2130-2134: LGTM!The pathconf error handling correctly converts Errno to io::Error and associates it with the path using OSErrorBuilder.
close #6440
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.