Actionable comments posted: 1

crates/vm/src/exceptions.rs (1)

1209-1214: Clean up OSErrorBuilder::with_subtype and clarify builder invariants

The builder is generally well‑structured, but there are a couple of small issues worth tightening:

• In with_subtype, strerror is 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,
             }
         }

• build relies on strerror always being populated by the constructors, even though the field type is Option<PyObjectRef>. That’s fine as an internal invariant, but it would be safer either to:

  • store strerror as a plain PyObjectRef (required by the API), or
  • add a debug/assert to document the expectation before calling strerror.to_pyobject(vm).

• When errno is None, any filename / filename2 set via the builder are currently ignored. That matches CPython’s OSError(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.

• build calls py_new, into_ref_with_type, and slot_init with expect("new_os_error usage error"). If these paths can be hit with user‑controlled types in the future, consider returning PyResult<PyRef<PyOSError>> and letting callers decide whether to unwrap, to avoid panics in release builds.

Also applies to: 1218-1257, 1265-1327

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 550497e and 51b6347.

• 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)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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.rs
• crates/vm/src/stdlib/io.rs
• crates/vm/src/ospath.rs
• crates/vm/src/stdlib/os.rs
• crates/vm/src/vm/vm_new.rs
• crates/vm/src/exceptions.rs

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.rs
• crates/vm/src/vm/vm_new.rs
• crates/vm/src/exceptions.rs

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.rs
• crates/vm/src/vm/vm_new.rs

crates/vm/src/ospath.rs (1)

• with_filename (147-157)

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/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/ospath.rs (1)

• with_filename (147-157)

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/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)

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 since builder is only mutated on Windows.

---

70-74: Clean delegation to builder pattern.

The ToPyException implementation correctly delegates to the new ToOSErrorBuilder trait, 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_filename usage 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_filename to 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 from exceptions looks good

This 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: Using OSErrorBuilder in new_os_subtype_error keeps behavior while centralizing construction

The new OSErrorBuilder::with_subtype(...).build(self) path preserves the previous semantics (including the basicsize debug‑assert on exc_type) while routing OS error creation through the shared builder. This looks correct for both errno: None (OSError(msg)) and Some(errno) flows.

Also applies to: 110-124

crates/vm/src/stdlib/posix.rs (1)

29-35: Consistent use of OSErrorBuilder::with_filename for POSIX filesystem errors

All these call sites now route std::io::Error (or errors converted into it) through OSErrorBuilder::with_filename, which:

• Preserves the original IO error’s errno and message via ToOSErrorBuilder.
• Attaches the relevant OsPath / OsPathOrFd as filename (and filename2 where applicable).
• Returns a PyBaseExceptionRef suitable for PyResult error types.

The patterns in access, _chmod, lchmod, rmdir, listdir, scandir, pathconf, truncate, and utime_impl are all coherent and should yield better CPython‑style OSError instances 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: Unified OSErrorBuilder usage across os filesystem operations

The new imports and map_err sites 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 by OSErrorBuilder) when constructing errno‑driven errors.
• OSErrorBuilder::with_filename(&io::Error::from(Errno::last()), path, vm) for errno‑based pathconf failures.

This centralizes OS error creation and ensures that errno, strerror, and filename are wired the same way across open, remove/unlink, mkdir, rmdir, listdir (path branch), readlink, stat/fstat, chdir, truncate, utime, and pathconf. The patterns are type‑correct and align with how OSErrorBuilder/ToOSErrorBuilder are 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/replace and link now produce fully populated OSError with both filenames

The new error handling for rename/replace and link:

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.
• filename and filename2 are set to src and dst (respecting OutputMode for 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_filename helper is well-factored and matches call sites

The 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 → OSError with filename set” pattern used across os and posix. The API surface (taking any Into<OsPathOrFd<'a>>) keeps call sites clean and avoids duplicating the to_os_error_builder().filename(...).build(vm) sequence everywhere.

The TODO about returning PyRef<PyOSError> instead of PyBaseExceptionRef can be addressed later without affecting current users.

Actionable comments posted: 0

crates/vm/src/stdlib/posix.rs (1)

534-538: Fix the nix::Error to i32 cast.

The chown error handling incorrectly casts nix::Error directly to i32:

let err = io::Error::from_raw_os_error(err as i32);

This cast is unsafe for non-errno nix::Error variants (e.g., InvalidPath, InvalidUtf8). Since nix supports Into<io::Error> for nix::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)
 })

crates/vm/src/exceptions.rs (1)

1259-1263: Remove or uncomment the commented-out errno setter.

The commented-out errno setter 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.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 51b6347 and ccc153e.

• crates/vm/src/exceptions.rs (3 hunks)
• crates/vm/src/stdlib/posix.rs (9 hunks)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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.rs
• crates/vm/src/stdlib/posix.rs

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

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

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

crates/vm/src/stdlib/io.rs (3)

• to_os_error_builder (22-67)
• vm (3519-3521)
• build (2332-2348)

crates/vm/src/ospath.rs (1)

• with_filename (147-157)

• 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

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::Error to io::Error using .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::Error to io::Error using .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.