Actionable comments posted: 0

crates/stdlib/src/socket.rs (1)

2484-2497: Verify last_os_error() correctness after closesocket on Windows

close_inner now uses std::io::Error::last_os_error() when close(x) fails (ignoring ECONNRESET). On Unix this is the right way to surface errno from close(2). On Windows this maps to closesocket, which reports errors via WinSock (WSAGetLastError). Please double‑check that in your target toolchain std::io::Error::last_os_error() observes the same error code that socket operations set; if not, it may be safer to reuse the existing socket error plumbing (e.g. via errno_io_error() or ErrorExt) here as well.

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

2339-2345: sysconf still treats all -1 returns as errors—consider checking errno

While you’re adjusting sysconf’s error constructor, note that:

let r = unsafe { libc::sysconf(name.0) };
if r == -1 {
    return Err(vm.new_last_errno_error());
}

will raise for any -1, even when sysconf succeeds but returns -1 with errno == 0 (which POSIX/CPython interpret as “no limit”). A more faithful implementation would be:

use nix::errno::Errno;
Errno::clear();
let r = unsafe { libc::sysconf(name.0) };
if r == -1 && Errno::last_raw() != 0 {
    return Err(vm.new_last_errno_error());
}
Ok(r)

This is a pre-existing behavior, but this refactor is a good opportunity to align with POSIX semantics.

crates/common/src/os.rs (1)

36-49: Consider using libc for more direct errno access on non-Windows.

The non-Windows get_errno() implementation creates an io::Error just to extract the errno value, which is slightly indirect. A more direct approach would use libc's errno access.

 #[cfg(not(windows))]
 pub fn get_errno() -> i32 {
-    std::io::Error::last_os_error().posix_errno()
+    unsafe { *libc::__errno_location() }
 }

However, the current implementation is portable and correct, so this is a minor optimization that could be deferred.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 26b3445 and 967bff2.

• Lib/test/test_os.py is excluded by !Lib/**
• Lib/test/test_ssl.py is excluded by !Lib/**
• Lib/test/test_support.py is excluded by !Lib/**

• crates/common/src/crt_fd.rs (2 hunks)
• crates/common/src/fileutils.rs (1 hunks)
• crates/common/src/os.rs (2 hunks)
• crates/stdlib/src/fcntl.rs (7 hunks)
• crates/stdlib/src/multiprocessing.rs (3 hunks)
• crates/stdlib/src/overlapped.rs (7 hunks)
• crates/stdlib/src/resource.rs (1 hunks)
• crates/stdlib/src/socket.rs (5 hunks)
• crates/vm/src/stdlib/io.rs (1 hunks)
• crates/vm/src/stdlib/msvcrt.rs (2 hunks)
• crates/vm/src/stdlib/nt.rs (24 hunks)
• crates/vm/src/stdlib/os.rs (7 hunks)
• crates/vm/src/stdlib/posix.rs (16 hunks)
• crates/vm/src/stdlib/signal.rs (1 hunks)
• crates/vm/src/stdlib/time.rs (2 hunks)
• crates/vm/src/stdlib/winapi.rs (6 hunks)
• crates/vm/src/vm/vm_new.rs (2 hunks)
• crates/vm/src/windows.rs (1 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/stdlib/src/resource.rs
• crates/vm/src/stdlib/msvcrt.rs
• crates/vm/src/windows.rs
• crates/common/src/fileutils.rs
• crates/stdlib/src/fcntl.rs
• crates/vm/src/stdlib/nt.rs
• crates/vm/src/stdlib/posix.rs
• crates/stdlib/src/multiprocessing.rs
• crates/stdlib/src/overlapped.rs
• crates/vm/src/stdlib/winapi.rs
• crates/stdlib/src/socket.rs
• crates/vm/src/stdlib/time.rs
• crates/common/src/os.rs
• crates/vm/src/stdlib/signal.rs
• crates/vm/src/stdlib/os.rs
• crates/common/src/crt_fd.rs
• crates/vm/src/stdlib/io.rs
• crates/vm/src/vm/vm_new.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/stdlib/src/fcntl.rs
• crates/vm/src/stdlib/nt.rs
• crates/vm/src/stdlib/os.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Applied to files:

• crates/stdlib/src/fcntl.rs
• crates/vm/src/stdlib/os.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Applied to files:

• crates/stdlib/src/fcntl.rs
• crates/vm/src/stdlib/os.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/stdlib/os.rs
• crates/vm/src/vm/vm_new.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 Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations

Applied to files:

• crates/vm/src/stdlib/os.rs

crates/common/src/os.rs (2)

• errno_io_error (25-29)
• errno_io_error (32-34)

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

• fs_metadata (12-21)

crates/common/src/os.rs (2)

• errno_io_error (25-29)
• errno_io_error (32-34)

crates/common/src/os.rs (2)

• errno_io_error (25-29)
• errno_io_error (32-34)

crates/common/src/os.rs (2)

• errno_io_error (25-29)
• errno_io_error (32-34)

crates/stdlib/src/fcntl.rs (1)

• fcntl (59-90)

crates/common/src/os.rs (2)

• errno_io_error (25-29)
• errno_io_error (32-34)

• GitHub Check: Run snippets and cpython tests (macos-latest)

crates/stdlib/src/resource.rs (1)

163-163: LGTM!

Using vm.new_last_errno_error() is correct here since libc::getrlimit is a POSIX function that sets errno on failure.

crates/common/src/fileutils.rs (1)

10-21: LGTM!

Using errno_io_error() for libc::fstat failure is appropriate since it's a libc function that sets errno. This aligns with the PR's centralized errno-based error handling approach.

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

293-303: LGTM!

Using vm.new_last_errno_error() is correct here since siginterrupt is a POSIX function that sets errno on failure.

crates/common/src/crt_fd.rs (2)

35-43: LGTM!

Using errno_io_error() is correct for CRT functions that set errno. The added comment clearly explains the distinction between CRT errno and Windows GetLastError().

---

342-354: LGTM!

The comment correctly documents that _get_osfhandle is a CRT function that sets errno rather than GetLastError(), making errno_io_error() the appropriate error constructor.

crates/stdlib/src/multiprocessing.rs (2)

19-29: LGTM!

Using vm.new_last_os_error() is correct for Windows socket functions since they set WSAGetLastError() (which is retrieved via GetLastError()).

---

31-41: LGTM!

Using vm.new_last_os_error() is correct for the send Windows socket function.

crates/vm/src/stdlib/winapi.rs (6)

80-95: LGTM!

Using vm.new_last_os_error() is correct for GetStdHandle since it's a Windows API that sets GetLastError() on failure.

---

157-168: LGTM!

Using vm.new_last_os_error() is correct for GetFileType Windows API.

---

292-310: LGTM!

Using vm.new_last_os_error() is correct for OpenProcess Windows API.

---

416-421: LGTM!

Using vm.new_last_os_error() is correct for the InitializeProcThreadAttributeList failure path.

---

455-463: LGTM!

Using vm.new_last_os_error() is correct for WaitForSingleObject Windows API.

---

516-535: LGTM!

Using vm.new_last_os_error() is correct for OpenMutexW Windows API.

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

3991-3995: LGTM!

Using vm.new_last_errno_error() is correct for the fcntl syscall failure since it's a POSIX function that sets errno. This is consistent with the similar pattern used in crates/stdlib/src/fcntl.rs as shown in the relevant code snippets.

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

81-88: setmode/open_osfhandle now correctly use vm.new_last_errno_error()

Using vm.new_last_errno_error() when _setmode/open_osfhandle return -1 matches their errno-based failure semantics and centralizes POSIX-style error construction in the VM. No issues spotted.

Also applies to: 91-98

crates/stdlib/src/fcntl.rs (1)

11-12: POSIX error paths correctly migrated to vm.new_last_errno_error()

All updated branches (fcntl, ioctl, flock, lockf) now convert ret < 0 into vm.new_last_errno_error(), which is the right choice for errno-based libc calls and keeps behavior consistent across the module. No functional regressions evident.

Also applies to: 58-90, 92-143, 145-215

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

44-55: WindowsSysResult::into_pyresult now correctly reports last OS error

The condition inversion to if !self.is_err() is semantically equivalent, and switching the failure case to vm.new_last_os_error() is appropriate for Win32-style return values (HANDLE/BOOL). This keeps the wrapper aligned with Windows APIs.

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

816-832: Windows time errors now correctly mapped via vm.new_last_os_error()

Using vm.new_last_os_error() for QueryPerformanceFrequency and GetSystemTimeAdjustment failures matches their Win32 “0 == error + GetLastError” contract and aligns this module with the new VM error API.

Also applies to: 857-873

crates/stdlib/src/overlapped.rs (1)

240-261: Overlapped I/O now consistently uses vm.new_last_os_error()

For all the affected Win32 calls (cancel, events, IOCP, GQCS), the new vm.new_last_os_error() branches correctly trigger on 0/NULL or INVALID_HANDLE_VALUE and surface the last OS error via the VM. Resource handling (closing handles, marking completion) is unchanged.

Also applies to: 267-297, 303-323, 325-359, 361-411

crates/stdlib/src/socket.rs (2)

1534-1575: Socket option errors now flow through errno_io_error and IoOrPyException

Routing getsockopt/setsockopt failures through crate::common::os::errno_io_error().into() is consistent with the rest of this module: you get a platform-appropriate io::Error that is then converted to IoOrPyException and ultimately to a Python OSError. Control flow and range checks are unchanged.

Also applies to: 1578-1616

---

2155-2166: The premise about if_indextoname error handling on Windows requires verification against the actual C binding implementation

While the review comment's general principle about distinguishing errno vs. GetLastError on Windows is sound, the specific application to if_indextoname may be questionable. According to Microsoft documentation, if_indextoname on Windows does not set GetLastError when it fails—it simply returns NULL without an error code. This suggests that neither vm.new_last_errno_error() nor vm.new_last_os_error() would capture meaningful error information on Windows for this particular function. Verify whether the C binding actually provides a Windows implementation and, if so, what error reporting mechanism it uses. If if_indextoname is unavailable on Windows or uses a different error model, the current uniform error handling may need a different approach than the one suggested.

crates/vm/src/stdlib/nt.rs (2)

15-23: CRT/errno-based paths correctly switched to vm.new_last_errno_error()

For MSVC/CRT calls that signal failure via errno (_cwait, _wspawnv/_wspawnve, _wexecv/_wexecve, _umask, libc::dup, libc::dup2), the new vm.new_last_errno_error() branches are the right abstraction and keep behavior localized in the VM. The added imports (crt_fd, ToWideString, DirFd, etc.) match actual usage in this file.

Also applies to: 160-179, 278-318, 320-395, 870-878, 987-1015

---

182-208: Win32 API error handling now consistently uses last-OS-error helpers

For the Win32 calls (GenerateConsoleCtrlEvent, OpenProcess/TerminateProcess, GetConsoleScreenBufferInfo and its CONOUT$ fallback, GetFullPathNameW, GetVolumePathNameW, GetDiskFreeSpaceExW, GetHandleInformation, GetLogicalDriveStringsW, FindFirstVolumeW, CreateDirectoryW, CreatePipe), the updated code correctly:

• Detects failure via 0/NULL/INVALID_HANDLE_VALUE.
• Constructs Python exceptions via vm.new_last_os_error() (or io::Error::last_os_error() in the pure-Rust helpers like raw_set_handle_inheritable), which is exactly what the new VM API is for.

No behavioral issues spotted; resource cleanup logic is preserved.

Also applies to: 210-259, 517-547, 549-635, 637-644, 674-688, 691-708, 880-914

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

445-474: POSIX errno-based wrappers now consistently use vm.new_last_errno_error() / vm.new_last_os_error()

Across the updated POSIX helpers:

• Redox symlink and mknod branches now detect < 0/!= 0 and surface errors via vm.new_last_errno_error().
• nice, sched_get_priority_{max,min}, sched_getscheduler, sched_setscheduler, sched_getparam, sched_setparam, kill, _fcopyfile, getpriority, and setpriority all wrap libc/nix calls that use errno on failure and now consistently convert those failures via vm.new_last_errno_error().
• Linux getrandom maps a negative syscall return (caught via the try_into failure) to vm.new_last_os_error(), which on Unix is effectively “last errno” and aligns with the new VM API.

Overall this standardizes the errno-to-Python mapping without changing control flow or return values on success.

Also applies to: 679-737, 747-756, 760-778, 862-903, 911-953, 1728-1736, 1773-1780, 1891-1921, 2451-2473

crates/vm/src/vm/vm_new.rs (2)

10-10: LGTM!

Import correctly expanded to include ToPyException which is needed by the new error conversion methods.

---

205-221: Well-designed API for platform-specific error handling.

The two methods correctly distinguish between Windows API errors (GetLastError) and CRT errno errors. The documentation clearly explains when to use each method, making it easy for callers to choose the appropriate one. The implementation properly delegates to errno_io_error() from common::os which handles the Windows errno-to-winerror translation.

crates/common/src/os.rs (1)

22-34: LGTM!

The platform-specific implementations of errno_io_error() are correct:

• Windows: Reads CRT errno, converts to Windows error code, then creates io::Error
• Non-Windows: Directly uses last_os_error() since POSIX errno is the OS error

crates/vm/src/stdlib/os.rs (5)

5-5: LGTM!

Import correctly updated to remove PyBaseExceptionRef which is now accessed via full path where needed.

---

24-35: LGTM!

The IntoPyException implementations correctly use the full path for the return type and maintain the existing conversion logic.

---

334-348: LGTM!

Correct use of errno_io_error() for libc functions (mkdirat, mkdir) which set errno. The error handling with IOErrorBuilder::with_filename properly preserves the path context.

---

1154-1158: LGTM!

Clean simplification of error handling. Using vm.new_last_os_error() is correct here:

• On Windows: SetFilePointer sets GetLastError(), which new_last_os_error() reads
• On non-Windows: libc::lseek sets errno, and new_last_os_error() is equivalent to new_last_errno_error()

---

1481-1481: LGTM!

Correct use of vm.new_last_errno_error() for Linux syscall error handling. The copy_file_range syscall sets errno on failure, which new_last_errno_error() properly reads.