Conversation
📝 WalkthroughWalkthroughAdds a new Cargo feature flag Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Binary
participant VM as VirtualMachine
participant FS as Filesystem
participant Loader as Importlib/Loader
participant StdLib as stdlib modules
Note over CLI,VM: host_env enabled?
CLI->>VM: run_any_file(path) (host_env)
VM->>FS: stat/read path
FS-->>VM: path type (.py / dir / .pyc)
VM->>Loader: choose loader / set_main_loader
Loader-->>VM: code object
VM->>StdLib: initialize modules (host_env gated)
VM->>VM: execute code
VM-->>CLI: exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin sandbox |
youknowone
force-pushed
the
sandbox
branch
2 times, most recently
from
c1c187b to
5069248
Compare
crates/vm/src/stdlib/builtins.rs
Outdated
|
|
||
| #[pymodule] | ||
| mod builtins { | ||
| use crate::py_io; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/exceptions.rs (1)
2148-2158:⚠️ Potential issue | 🟡 MinorSetter allows storing sentinel value
-1, causing subsequent access to raiseAttributeError.If Python code sets
exc.characters_written = -1, the setter stores it without validation. The getter then treats-1as a sentinel for "not set" and raisesAttributeError. This diverges from CPython, where any explicitly set integer value (including-1) is retrievable as-is. The setter should validate that input values differ from the sentinel before storing.
🤖 Fix all issues with AI agents
In `@crates/vm/src/lib.rs`:
- Around line 70-71: The import of the ospath module in
crates/vm/src/stdlib/io.rs must be conditionally compiled to match the module
declaration (pub mod ospath) which is #[cfg(feature = "host_env")] in lib.rs;
locate the import statement that references ospath (the use/mod of ospath around
line ~5059 in stdlib/io.rs) and prefix it with #[cfg(feature = "host_env")] so
the import is only compiled when the host_env feature is enabled.
In `@crates/vm/src/stdlib/mod.rs`:
- Around line 101-104: The two cfg attributes both enable posix::module_def(ctx)
on WASI causing a duplicate; update the second attribute so it excludes WASI
(e.g. change #[cfg(all(feature = "host_env", not(any(unix, windows)))] to
exclude target_os = "wasi", such as #[cfg(all(feature = "host_env",
not(any(unix, windows, target_os = "wasi"))))]) so posix::module_def(ctx) is
only added once.
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 122-132: The readline implementation currently ignores the
OptionalArg<isize> _size; update TextIOBase::readline (function readline in
sys.rs) to explicitly reject non-default size values instead of ignoring them:
before reading from stdin, check if _size is Present (i.e., Some) and if so
return an appropriate VM error (e.g., a NotImplementedError or ValueError
created via VirtualMachine like vm.new_not_implemented or vm.new_value_error)
with a clear message such as "readline with size not supported in sandbox";
otherwise proceed with the existing fd check and stdin.read_line flow.
- Around line 108-120: The write method currently swallows IO errors; change the
write implementation in the Sys* struct's #[pymethod] fn write(&self, s:
PyStrRef, vm: &VirtualMachine) -> PyResult<usize> to propagate errors from
stdout/stderr by replacing the `let _ = ...` ignores with
`.write_all(bytes).map_err(|e| vm.new_os_error(e.to_string()))?` so any write
failures become Py OSError; likewise implement a #[pymethod] fn flush(&self, vm:
&VirtualMachine) -> PyResult<()> (add the vm parameter) and call the appropriate
stream's `flush().map_err(|e| vm.new_os_error(e.to_string()))?` instead of
ignoring errors, returning Ok(()) on success.
🧹 Nitpick comments (8)
src/lib.rs (1)
176-189: Sandbox fallback design is sound but loses OS error detail.The sandbox path correctly keeps filesystem I/O in the binary (trusted host) while the VM only sees source strings. Two minor observations:
std::fs::read_to_stringassumes UTF-8, which is the Python 3 default source encoding — fine for the common case.- Line 187:
err.to_string()discards the originalio::ErrorKind/errno. Consider preserving it if the VM'snew_os_errorsupports it, though this is low priority for a sandbox fallback.Optional: preserve errno in sandbox error path
- Err(err) => Err(vm.new_os_error(err.to_string())), + Err(err) => { + let msg = format!("Can't open file '{}': {}", path, err); + Err(vm.new_os_error(msg)) + }extra_tests/snippets/sandbox_smoke.py (1)
19-21: Minor:sys.stderris not tested for writability.stdin/stdout are tested but stderr is mentioned in the docstring (line 4) and available via
SandboxStdio. Consider addingassert sys.stderr.writable()for completeness.Optional: add stderr assertion
assert sys.stdout.writable() assert sys.stdin.readable() +assert sys.stderr.writable() assert sys.stdout.fileno() == 1crates/vm/src/vm/python_run.rs (2)
149-155:Read::readmay short-read even on regular files.
file.read(&mut buf)?is not guaranteed to fill the 2-byte buffer in a single call. While exceedingly rare for regular files,Read::read_exactwould be the robust choice here.Suggested fix
- use std::io::Read; - if file.read(&mut buf)? != 2 { + use std::io::Read; + if file.read_exact(&mut buf).is_err() {
109-112: Commented-out deprecation onrun_script.The
#[deprecated]attribute is commented out. If this is intentional for now, consider adding a brief// TODOexplaining when the deprecation should be activated. Otherwise it's easy for this to be forgotten.crates/vm/src/exceptions.rs (4)
1307-1345:build()relies onexpect()for infallible paths — verify they truly can't fail.Lines 1338, 1341, and 1343 each call
.expect("new_os_error usage error"). Whilepy_newalways returnsOk(...)for the current implementation andinto_ref_with_typeshould succeed when using the sameexc_type,slot_initis more complex and could theoretically fail (e.g., on a subclass that overrides__init__). If any of these paths fail, the panic will be hard to diagnose.Consider whether
slot_initcould fail for exotic subclasses and, if so, whether returning aPyResultfrombuildwould be more appropriate. If these are truly infallible, a brief comment on why would help future maintainers.
1320-1335: On non-Windows, a dummyNoneis injected at position 3 (winerror slot) into the args vec.When
errnoisSome, the args vector is always 5 elements:[errno, strerror, filename, winerror_or_none, filename2]. On non-Windows,winerrorisvm.ctx.none(). This meansslot_initreceives 5 args on all platforms — which works becauseslot_initonly processesargs[3]inside a#[cfg(windows)]block. However, this couples the builder's arg layout toslot_init's conditional parsing.This is fine as-is, but a brief comment noting that the 4th position is always
Noneon non-Windows (and ignored byslot_init) would prevent confusion.
1375-1385:libc::strerroris not guaranteed thread-safe.On Unix,
libc::strerrormay return a pointer to a shared static buffer (for unknown errno values). While most modern libc implementations use static string literals for known errno values, the general case is not thread-safe. Since the result is immediately copied viato_string_lossy().into_owned(), the risk is low, but it's worth noting.If thread-safety is a concern,
strerror_r(POSIX) orstrerror_lwould be safer alternatives, though they come with their own portability complications.
1926-1926: Minor: Cloning the atomic ref just to check forNone.
exc.filename.to_owned()clones thePyObjectRefbehind the atomic ref only to checkis_none. Consider using a read/deref approach if available to avoid the unnecessary clone:-let has_filename = exc.filename.to_owned().filter(|f| !vm.is_none(f)).is_some(); +let has_filename = exc.filename.deref().is_some_and(|f| !vm.is_none(f));(Depends on whether
deref()returns a suitable reference — adjust as needed for thePyAtomicRefAPI.)
| #[pymethod] | ||
| fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | ||
| if self.fd != 0 { | ||
| return Err(vm.new_os_error("not readable".to_owned())); | ||
| } | ||
| let mut line = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut line) | ||
| .map_err(|e| vm.new_os_error(e.to_string()))?; | ||
| Ok(line) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython TextIOBase.readline(size) semantics behavior documentation
💡 Result:
io.TextIOBase.readline(size=-1) (text mode) semantics (CPython)
- What it reads: Reads from the current position up to and including the next line ending, or until EOF if no line ending is found. It returns one
str. If already at EOF, it returns""(empty string). [1] sizemeaning: Ifsizeis provided, at mostsizecharacters are read (so you may get a partial line that does not end in a newline). [1]- Newline handling: What counts as a “newline” depends on how the text stream was opened (the
newline=argument toopen()controls which line terminators are recognized / translated). [1]
Contrast: binary streams (BufferedReader.readline)
- In binary mode,
readline(size)reads at mostsizebytes, and line termination is based onb"\n". [2]
Sources:
[1] Python io docs (TextIOBase.readline) (docs.python.org)
[2] Python io docs (IOBase.readline / binary-vs-text note) (docs.python.org)
Citations:
- 1: https://docs.python.org/uk/3.12/library/io.html?utm_source=openai
- 2: https://docs.python.org/es/3.12/library/io.html?utm_source=openai
readline silently ignores the size parameter, diverging from CPython's TextIOBase.readline(size) behavior.
In CPython, the size parameter controls the maximum number of characters to read. When provided, it may result in a partial line without a newline. The current implementation ignores this parameter regardless of its value, making behavior unpredictable for callers expecting standard TextIOBase semantics.
For a sandbox environment where full support may not be feasible, explicitly reject non-default size values rather than silently ignoring them:
Suggested fix
- fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> {
+ fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> {
if self.fd != 0 {
return Err(vm.new_os_error("not readable".to_owned()));
}
+ if let OptionalArg::Present(size) = size {
+ if size >= 0 {
+ return Err(vm.new_not_implemented_error(
+ "size argument is not supported in sandbox stdio",
+ ));
+ }
+ }
let mut line = String::new();
std::io::stdin()
.read_line(&mut line)
.map_err(|e| vm.new_os_error(e.to_string()))?;
Ok(line)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[pymethod] | |
| fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| Ok(line) | |
| } | |
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| if let OptionalArg::Present(size) = size { | |
| if size >= 0 { | |
| return Err(vm.new_not_implemented_error( | |
| "size argument is not supported in sandbox stdio", | |
| )); | |
| } | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| Ok(line) | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/sys.rs` around lines 122 - 132, The readline
implementation currently ignores the OptionalArg<isize> _size; update
TextIOBase::readline (function readline in sys.rs) to explicitly reject
non-default size values instead of ignoring them: before reading from stdin,
check if _size is Present (i.e., Some) and if so return an appropriate VM error
(e.g., a NotImplementedError or ValueError created via VirtualMachine like
vm.new_not_implemented or vm.new_value_error) with a clear message such as
"readline with size not supported in sandbox"; otherwise proceed with the
existing fd check and stdin.read_line flow.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
I like the idea a lot
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/vm/mod.rs (1)
326-394:⚠️ Potential issue | 🔴 CriticalFix conflicting cfg conditions for
make_stdiobranches.The
host_envandstdiofeatures are independent in Cargo.toml (neither depends on the other), but the three cfg branches are not mutually exclusive. Whenhost_envis enabled andstdiois disabled, both the first branch (#[cfg(feature = "host_env")]) and the third branch (#[cfg(not(feature = "stdio"))]) match simultaneously, causing a duplicatemake_stdiobinding and a compile error.Change the third condition from
#[cfg(not(feature = "stdio"))]to#[cfg(all(not(feature = "host_env"), not(feature = "stdio")))]to ensure all three branches are truly mutually exclusive.
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 126-141: The readline implementation in SandboxStdio::readline
treats the size parameter as a byte index and calls line.truncate(size as
usize), which can panic on UTF‑8 boundaries for non‑ASCII input; change the
truncation to operate on character count instead of raw bytes—compute the byte
boundary for the first N characters (size) or rebuild the string with
line.chars().take(size as usize).collect(), then return that safely truncated
String while keeping the existing checks (handle size <= 0 and OptionalArg
unwrap as before).
🧹 Nitpick comments (1)
crates/vm/src/exceptions.rs (1)
1375-1385:libc::strerroris not guaranteed thread-safe on all platforms.On some platforms,
strerrorreturns a pointer to a shared static buffer. While glibc uses thread-local storage, POSIX doesn't guarantee thread-safety. The immediate.to_string_lossy().into_owned()conversion minimizes the risk window, butstrerror_r(or Rust'sstd::io::Error::from_raw_os_error(errno).to_string()) would be a safer alternative.
| #[pymethod] | ||
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | ||
| if self.fd != 0 { | ||
| return Err(vm.new_os_error("not readable".to_owned())); | ||
| } | ||
| let size = size.unwrap_or(-1); | ||
| if size == 0 { | ||
| return Ok(String::new()); | ||
| } | ||
| let mut line = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut line) | ||
| .map_err(|e| vm.new_os_error(e.to_string()))?; | ||
| if size > 0 { | ||
| line.truncate(size as usize); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust String::truncate UTF-8 boundary panic behavior
💡 Result:
String::truncate(new_len) truncates by byte index, but it must leave the String as valid UTF-8.
Panic behavior
truncate panics at runtime if:
new_lenis not on a UTF-8 code point /charboundary (i.e., it points into the middle of a multi-byte character). (web.mit.edu)- (More generally for Rust string boundary-checked operations) the index is past the end / not a valid boundary; Rust enforces these checks to prevent creating invalid UTF-8. (doc.rust-lang.org)
How to avoid the panic
- Check first:
s.is_char_boundary(new_len)(method onstr). (stackoverflow.com) - Or compute a safe boundary (e.g., walk
char_indices()to the last boundary ≤new_len) and truncate there. (gist.github.com)
Citations:
- 1: https://web.mit.edu/rust-lang_v1.26.0/arch/amd64_ubuntu1404/share/doc/rust/html/std/string/struct.String.html?utm_source=openai
- 2: https://doc.rust-lang.org/core/primitive.str.html?utm_source=openai
- 3: https://stackoverflow.com/questions/45837114/why-does-a-truncated-string-rust-print-as-an-empty-pair-of-parenthesis?utm_source=openai
- 4: https://gist.github.com/dginev/f6da5e94335d545e0a7b?utm_source=openai
🏁 Script executed:
# Check the actual file and surrounding context
cat -n crates/vm/src/stdlib/sys.rs | sed -n '120,145p'Repository: RustPython/RustPython
Length of output: 1072
Avoid UTF‑8 boundary panics when honoring size in SandboxStdio.readline.
String::truncate expects a byte index on a UTF‑8 char boundary; using the raw size parameter as a byte index will panic on non‑ASCII input. The method interprets size as a character count, but the code treats it as a byte index directly.
🔧 Suggested fix
- if size > 0 {
- line.truncate(size as usize);
- }
+ if size > 0 {
+ let max_chars = size as usize;
+ if let Some((idx, _)) = line.char_indices().nth(max_chars) {
+ line.truncate(idx);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let size = size.unwrap_or(-1); | |
| if size == 0 { | |
| return Ok(String::new()); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| if size > 0 { | |
| line.truncate(size as usize); | |
| } | |
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let size = size.unwrap_or(-1); | |
| if size == 0 { | |
| return Ok(String::new()); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| if size > 0 { | |
| let max_chars = size as usize; | |
| if let Some((idx, _)) = line.char_indices().nth(max_chars) { | |
| line.truncate(idx); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/sys.rs` around lines 126 - 141, The readline
implementation in SandboxStdio::readline treats the size parameter as a byte
index and calls line.truncate(size as usize), which can panic on UTF‑8
boundaries for non‑ASCII input; change the truncation to operate on character
count instead of raw bytes—compute the byte boundary for the first N characters
(size) or rebuild the string with line.chars().take(size as usize).collect(),
then return that safely truncated String while keeping the existing checks
(handle size <= 0 and OptionalArg unwrap as before).
Introduce a `host_env` feature flag that gates all host environment access (filesystem, network, signals, processes). When disabled, the VM operates in sandbox mode: - _io module always available; FileIO gated by host_env - SandboxStdio provides lightweight stdin/stdout/stderr via Rust std::io - BytesIO/StringIO/BufferedIO/TextIOWrapper work without host_env - open() returns UnsupportedOperation in sandbox - stdlib modules (os, socket, signal, etc.) gated by host_env - CI checks both host_env ON and OFF builds
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 156-167: Update the CI job to build with the compiler feature and
run sandbox snippets using cargo run instead of invoking the binary directly:
add the compiler feature to the build line (the step that currently runs cargo
build --no-default-features --features
stdlib,importlib,stdio,encodings,freeze-stdlib) so it becomes consistent with
the check steps, and replace the two direct binary executions
(target/debug/rustpython extra_tests/snippets/sandbox_smoke.py and
target/debug/rustpython extra_tests/snippets/stdlib_re.py) in the "sandbox smoke
test" step with cargo run --bin rustpython -- <snippet-path> (run in debug mode)
to follow the extra_tests/snippets guideline and ensure faster, reproducible
builds.
In `@crates/vm/src/exceptions.rs`:
- Around line 1354-1398: The impl of ToOSErrorBuilder for std::io::Error
currently always calls OSErrorBuilder::with_errno(errno, msg, vm) even when
posix_errno() returns 0; change to check the errno returned by posix_errno() and
only call with_errno when errno > 0, otherwise call
OSErrorBuilder::with_subtype(py_exception_type_for_os_error, None, msg, vm)
(preserving the msg) so we don't emit OSError(errno=0); update the
to_os_error_builder function around the creation of builder (the let mut builder
= ... and subsequent #[cfg(windows)] winerror handling) to branch on errno
accordingly while keeping the winerror attachment when available.
🧹 Nitpick comments (1)
crates/vm/src/signal.rs (1)
82-83: Nit: inconsistent#[cfg]style withset_interrupt_exbelow.Lines 82–83 use two stacked
#[cfg(...)]attributes, while line 103 combines them into a single#[cfg(all(...))]. Both are semantically equivalent, but using a consistent style improves readability.♻️ Suggested consolidation
-#[cfg(unix)] -#[cfg(feature = "host_env")] +#[cfg(all(unix, feature = "host_env"))] pub(crate) fn clear_after_fork() {
| - name: check compilation without host_env (sandbox mode) | ||
| run: | | ||
| cargo check -p rustpython-vm --no-default-features --features compiler | ||
| cargo check -p rustpython-stdlib --no-default-features --features compiler | ||
| cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib | ||
| if: runner.os == 'Linux' | ||
|
|
||
| - name: sandbox smoke test | ||
| run: | | ||
| target/debug/rustpython extra_tests/snippets/sandbox_smoke.py | ||
| target/debug/rustpython extra_tests/snippets/stdlib_re.py | ||
| if: runner.os == 'Linux' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file and content at the specified lines
head -170 .github/workflows/ci.yaml | tail -25Repository: RustPython/RustPython
Length of output: 1256
🏁 Script executed:
# Search for other uses of extra_tests/snippets in the codebase
rg "extra_tests/snippets" --type yaml --type sh -A 2 -B 2Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check Cargo.toml to understand compiler feature
rg "compiler" Cargo.toml -A 2 -B 2 | head -50Repository: RustPython/RustPython
Length of output: 681
🏁 Script executed:
# Look for how cargo run is typically used in RustPython workflows
rg "cargo run" .github/workflows/ -A 1 -B 1 | head -60Repository: RustPython/RustPython
Length of output: 1057
🏁 Script executed:
# Search for how compiler feature relates to .py execution/compilation
rg "compiler" crates/vm -A 2 -B 2 | head -40Repository: RustPython/RustPython
Length of output: 2602
🏁 Script executed:
# Check if there's any documentation or tests about sandbox mode and compiler feature
rg "sandbox" --type rust --type py -B 3 -A 3 | head -60Repository: RustPython/RustPython
Length of output: 3899
🏁 Script executed:
# Check the feature definitions in Cargo.toml for rustpython-vm
rg "\[features\]" -A 30 rustpython-vm/Cargo.tomlRepository: RustPython/RustPython
Length of output: 129
🏁 Script executed:
# Look for any existing uses of cargo run with sandbox/no-default-features
rg "cargo.*run.*no-default-features" -A 1 -B 1Repository: RustPython/RustPython
Length of output: 231
🏁 Script executed:
# Check the main binary's implementation to see what features it uses for .py execution
cat -n src/lib.rs | head -100Repository: RustPython/RustPython
Length of output: 4289
🏁 Script executed:
# Look for feature guards around .py compilation/execution
rg "feature.*compiler\|feature.*parser" src/lib.rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check what features are needed for the main binary
rg "\[features\]" Cargo.toml -A 50 | head -80Repository: RustPython/RustPython
Length of output: 1885
🏁 Script executed:
# Check the actual file reading logic in sandbox mode
cat -n src/lib.rs | sed -n '100,200p'Repository: RustPython/RustPython
Length of output: 4296
🏁 Script executed:
# Look for how compiler feature is gated in the codebase for .py execution
rg "cfg.*compiler" src/ -B 2 -A 2 | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there's a feature guard around compilation in the interpreter
ast-grep --pattern 'fn.*compile($_)' | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if other CI workflows use cargo run vs direct binary execution
rg "cargo run" .github/workflows/ci.yaml -B 3 -A 3Repository: RustPython/RustPython
Length of output: 979
🏁 Script executed:
# Look for other direct binary execution in the CI
rg "target/debug" .github/workflows/ci.yaml -B 2 -A 2Repository: RustPython/RustPython
Length of output: 286
🏁 Script executed:
# Check if there's any documentation or rationale for cargo run vs direct binary
find . -name "*.md" -o -name "*.txt" | xargs grep -l "cargo run\|direct binary" 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 238
🏁 Script executed:
# Verify the guideline applies to CI/workflows context by checking AGENTS.md
cat -n AGENTS.md | grep -A 5 -B 5 "extra_tests/snippets"Repository: RustPython/RustPython
Length of output: 1029
🏁 Script executed:
# Verify the exact feature syntax by checking if stdlib,importlib,stdio,encodings,freeze-stdlib,compiler is valid
rg "features.*stdlib.*importlib.*stdio" Cargo.toml -A 1 -B 1Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there are any other uses of cargo run with multiple features in CI
rg "cargo run.*--features.*," .github/workflows/ -A 1 -B 1Repository: RustPython/RustPython
Length of output: 47
Run sandbox snippets via cargo run instead of direct binary execution.
The smoke test should follow the project guideline for extra_tests/snippets/** tests: use cargo run with debug mode for faster compilation. Additionally, add the compiler feature to the build step for consistency with the check step above it.
🔧 Suggested update
- name: check compilation without host_env (sandbox mode)
run: |
cargo check -p rustpython-vm --no-default-features --features compiler
cargo check -p rustpython-stdlib --no-default-features --features compiler
- cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib
+ cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler
if: runner.os == 'Linux'
- name: sandbox smoke test
run: |
- target/debug/rustpython extra_tests/snippets/sandbox_smoke.py
- target/debug/rustpython extra_tests/snippets/stdlib_re.py
+ cargo run --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler -- extra_tests/snippets/sandbox_smoke.py
+ cargo run --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler -- extra_tests/snippets/stdlib_re.py
if: runner.os == 'Linux'🤖 Prompt for AI Agents
In @.github/workflows/ci.yaml around lines 156 - 167, Update the CI job to build
with the compiler feature and run sandbox snippets using cargo run instead of
invoking the binary directly: add the compiler feature to the build line (the
step that currently runs cargo build --no-default-features --features
stdlib,importlib,stdio,encodings,freeze-stdlib) so it becomes consistent with
the check steps, and replace the two direct binary executions
(target/debug/rustpython extra_tests/snippets/sandbox_smoke.py and
target/debug/rustpython extra_tests/snippets/stdlib_re.py) in the "sandbox smoke
test" step with cargo run --bin rustpython -- <snippet-path> (run in debug mode)
to follow the extra_tests/snippets guideline and ensure faster, reproducible
builds.
| impl ToOSErrorBuilder for std::io::Error { | ||
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | ||
| use crate::common::os::ErrorExt; | ||
|
|
||
| let errno = self.posix_errno(); | ||
| #[cfg(windows)] | ||
| let msg = 'msg: { | ||
| // Use C runtime's strerror for POSIX errno values. | ||
| // For Windows-specific error codes, fall back to FormatMessage. | ||
| const MAX_POSIX_ERRNO: i32 = 127; | ||
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | ||
| let ptr = unsafe { libc::strerror(errno) }; | ||
| if !ptr.is_null() { | ||
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | ||
| if !s.starts_with("Unknown error") { | ||
| break 'msg s.into_owned(); | ||
| } | ||
| } | ||
| } | ||
| self.to_string() | ||
| }; | ||
| #[cfg(unix)] | ||
| let msg = { | ||
| let ptr = unsafe { libc::strerror(errno) }; | ||
| if !ptr.is_null() { | ||
| unsafe { core::ffi::CStr::from_ptr(ptr) } | ||
| .to_string_lossy() | ||
| .into_owned() | ||
| } else { | ||
| vec![strerror.to_pyobject(vm)] | ||
| }; | ||
| self.to_string() | ||
| } | ||
| }; | ||
| #[cfg(not(any(windows, unix)))] | ||
| let msg = self.to_string(); | ||
|
|
||
| #[allow(unused_mut)] | ||
| let mut builder = OSErrorBuilder::with_errno(errno, msg, vm); | ||
|
|
||
| let payload = PyOSError::py_new(&exc_type, args.clone().into(), vm) | ||
| .expect("new_os_error usage error"); | ||
| let os_error = payload | ||
| .into_ref_with_type(vm, exc_type) | ||
| .expect("new_os_error usage error"); | ||
| PyOSError::slot_init(os_error.as_object().to_owned(), args.into(), vm) | ||
| .expect("new_os_error usage error"); | ||
| os_error | ||
| #[cfg(windows)] | ||
| if let Some(winerror) = self.raw_os_error() { | ||
| builder = builder.winerror(winerror.to_pyobject(vm)); | ||
| } | ||
|
|
||
| builder | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ErrorExt::posix_errno implementation
rg -n "posix_errno" -C5 --type=rustRepository: RustPython/RustPython
Length of output: 4538
🏁 Script executed:
#!/bin/bash
# Find OSErrorBuilder implementation and its methods
rg -n "impl OSErrorBuilder" -A20 --type=rustRepository: RustPython/RustPython
Length of output: 1246
🏁 Script executed:
#!/bin/bash
# Find with_errno method in OSErrorBuilder
rg -n "with_errno" -A10 --type=rustRepository: RustPython/RustPython
Length of output: 1486
🏁 Script executed:
#!/bin/bash
# Find errno_to_exc_type to see how it handles errno values
rg -n "fn errno_to_exc_type" -A30 --type=rustRepository: RustPython/RustPython
Length of output: 4348
🏁 Script executed:
#!/bin/bash
# Check how errno=0 is handled elsewhere and if there are any tests
rg -n "errno.*0" -B2 -A2 --type=rust | head -50Repository: RustPython/RustPython
Length of output: 3710
🏁 Script executed:
#!/bin/bash
# Look for cases where std::io::Error might not have raw_os_error
rg -n "raw_os_error" -B3 -A3 --type=rust | head -80Repository: RustPython/RustPython
Length of output: 5378
🏁 Script executed:
#!/bin/bash
# Check if there are any non-OS errors in std::io::Error that would have raw_os_error() = None
# Look for test cases or examples
rg -n "std::io::Error" -B2 -A5 --type=rust | head -100Repository: RustPython/RustPython
Length of output: 6999
Guard errno > 0 before using with_errno; fall back to with_subtype(..., None, msg, ...) for non-OS errors.
The posix_errno() method returns 0 when std::io::Error has no OS error code (see crates/common/src/os.rs:32), resulting in OSError(errno=0) which is misleading since errno=0 means "Success". This is evident from line 1364 where the code already checks errno > 0 to identify valid POSIX errors. Apply the suggested guard to preserve the message while avoiding the invalid errno=0 state.
Suggested fix
- let mut builder = OSErrorBuilder::with_errno(errno, msg, vm);
+ let mut builder = if errno > 0 {
+ OSErrorBuilder::with_errno(errno, msg, vm)
+ } else {
+ OSErrorBuilder::with_subtype(vm.ctx.exceptions.os_error.to_owned(), None, msg, vm)
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl ToOSErrorBuilder for std::io::Error { | |
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | |
| use crate::common::os::ErrorExt; | |
| let errno = self.posix_errno(); | |
| #[cfg(windows)] | |
| let msg = 'msg: { | |
| // Use C runtime's strerror for POSIX errno values. | |
| // For Windows-specific error codes, fall back to FormatMessage. | |
| const MAX_POSIX_ERRNO: i32 = 127; | |
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | |
| if !s.starts_with("Unknown error") { | |
| break 'msg s.into_owned(); | |
| } | |
| } | |
| } | |
| self.to_string() | |
| }; | |
| #[cfg(unix)] | |
| let msg = { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ffi::CStr::from_ptr(ptr) } | |
| .to_string_lossy() | |
| .into_owned() | |
| } else { | |
| vec![strerror.to_pyobject(vm)] | |
| }; | |
| self.to_string() | |
| } | |
| }; | |
| #[cfg(not(any(windows, unix)))] | |
| let msg = self.to_string(); | |
| #[allow(unused_mut)] | |
| let mut builder = OSErrorBuilder::with_errno(errno, msg, vm); | |
| let payload = PyOSError::py_new(&exc_type, args.clone().into(), vm) | |
| .expect("new_os_error usage error"); | |
| let os_error = payload | |
| .into_ref_with_type(vm, exc_type) | |
| .expect("new_os_error usage error"); | |
| PyOSError::slot_init(os_error.as_object().to_owned(), args.into(), vm) | |
| .expect("new_os_error usage error"); | |
| os_error | |
| #[cfg(windows)] | |
| if let Some(winerror) = self.raw_os_error() { | |
| builder = builder.winerror(winerror.to_pyobject(vm)); | |
| } | |
| builder | |
| } | |
| impl ToOSErrorBuilder for std::io::Error { | |
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | |
| use crate::common::os::ErrorExt; | |
| let errno = self.posix_errno(); | |
| #[cfg(windows)] | |
| let msg = 'msg: { | |
| // Use C runtime's strerror for POSIX errno values. | |
| // For Windows-specific error codes, fall back to FormatMessage. | |
| const MAX_POSIX_ERRNO: i32 = 127; | |
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | |
| if !s.starts_with("Unknown error") { | |
| break 'msg s.into_owned(); | |
| } | |
| } | |
| } | |
| self.to_string() | |
| }; | |
| #[cfg(unix)] | |
| let msg = { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ffi::CStr::from_ptr(ptr) } | |
| .to_string_lossy() | |
| .into_owned() | |
| } else { | |
| self.to_string() | |
| } | |
| }; | |
| #[cfg(not(any(windows, unix)))] | |
| let msg = self.to_string(); | |
| #[allow(unused_mut)] | |
| let mut builder = if errno > 0 { | |
| OSErrorBuilder::with_errno(errno, msg, vm) | |
| } else { | |
| OSErrorBuilder::with_subtype(vm.ctx.exceptions.os_error.to_owned(), None, msg, vm) | |
| }; | |
| #[cfg(windows)] | |
| if let Some(winerror) = self.raw_os_error() { | |
| builder = builder.winerror(winerror.to_pyobject(vm)); | |
| } | |
| builder | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/exceptions.rs` around lines 1354 - 1398, The impl of
ToOSErrorBuilder for std::io::Error currently always calls
OSErrorBuilder::with_errno(errno, msg, vm) even when posix_errno() returns 0;
change to check the errno returned by posix_errno() and only call with_errno
when errno > 0, otherwise call
OSErrorBuilder::with_subtype(py_exception_type_for_os_error, None, msg, vm)
(preserving the msg) so we don't emit OSError(errno=0); update the
to_os_error_builder function around the creation of builder (the let mut builder
= ... and subsequent #[cfg(windows)] winerror handling) to branch on errno
accordingly while keeping the winerror attachment when available.
2 participants
#4210
Summary by CodeRabbit
New Features
Tests / Chores