-
Notifications
You must be signed in to change notification settings - Fork 1.4k
use posix error messages under 127 #6420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds platform-conditional message construction in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
059fc98 to
d44baf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/stdlib/io.rs (1)
23-44: Confirmlibc::strerrorsemantics on Windows targets; consider centralizing helper
libc::strerror’s thread-safety and returned “Unknown error” text can differ by CRT/toolchain/locale; please confirm this behaves as intended on the Windows targets RustPython supports (e.g., MSVC/UCRT vs MinGW) and that the"Unknown error"guard is robust enough.- There’s already a
strerror(e: i32) -> Stringhelper incrates/vm/src/stdlib/os.rs:1542-1546(per provided snippet); consider reusing/relocating a shared helper to avoid duplicating FFI + string handling logic.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_argparse.pyis excluded by!Lib/**Lib/test/test_py_compile.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/io.rs(1 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/io.rs
🧠 Learnings (1)
📚 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/stdlib/io.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/io.rs (2)
crates/vm/src/exceptions.rs (2)
errno(1450-1451)errno(1452-1452)crates/vm/src/stdlib/os.rs (2)
strerror(1543-1547)std(1207-1207)
⏰ 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). (9)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
7c59654 to
b093b19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/stdlib/io.rs (2)
23-41: Consider extracting strerror logic to a shared helper.The Windows block correctly handles POSIX errno translation with appropriate bounds checking and fallback logic. However,
libc::strerroris not thread-safe—it returns a pointer to a static buffer that can be overwritten by concurrent calls. This matches the existing pattern inos.rs(lines 1542-1546), so it's consistent with the codebase.Consider consolidating the strerror logic with the existing
strerrorhelper incrates/vm/src/stdlib/os.rsto reduce duplication and centralize any future thread-safety improvements.
42-52: Unix strerror logic duplicates existing helper in os.rs.This block is nearly identical to the
strerrorfunction atcrates/vm/src/stdlib/os.rs:1542-1546. Consider reusing that function to avoid duplication:#[cfg(unix)] let msg = { - let ptr = unsafe { libc::strerror(errno) }; - if !ptr.is_null() { - unsafe { std::ffi::CStr::from_ptr(ptr) } - .to_string_lossy() - .into_owned() - } else { - self.to_string() - } + // Reuse existing strerror helper from os module + crate::stdlib::os::strerror(errno) };Note: This requires making the
strerrorfunction inos.rspublic or moving it to a shared location.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_argparse.pyis excluded by!Lib/**Lib/test/test_py_compile.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/io.rs(1 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/io.rs
🧠 Learnings (1)
📚 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/stdlib/io.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/io.rs (1)
crates/vm/src/stdlib/os.rs (2)
strerror(1543-1547)std(1207-1207)
⏰ 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). (6)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (1)
crates/vm/src/stdlib/io.rs (1)
20-55: Implementation correctly handles platform-specific error messages.The overall approach is sound: using C runtime
strerrorfor better POSIX compatibility on Windows, with appropriate null checks and fallbacks. The labeled block pattern for early return is idiomatic Rust.
b093b19 to
786a65e
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.