host_env: renameat support for os.rename#8118
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changesos.rename dir_fd support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host_env/src/os.rs`:
- Around line 271-288: The error path in the rename function at line 286 returns
io::Error::other(...) directly, but the function signature specifies a return
type of io::Result<()>, which is Result<(), io::Error>. To fix this type
mismatch, wrap the io::Error::other(...) call with Err() so the error is
properly wrapped in the Result type, making the return statement consistent with
the function's declared return type.
In `@crates/vm/src/stdlib/os.rs`:
- Around line 1953-1954: The rename and replace functions unconditionally
advertise dir_fd support via Some(true) even though it only works on Unix
platforms. Add a new const RENAME_DIR_FD near the other *_DIR_FD constants that
uses cfg!(unix) to conditionally return true only on Unix systems, then update
the SupportFunc::new calls for both the "rename" and "replace" entries to use
Some(RENAME_DIR_FD) instead of Some(true) to accurately represent
platform-specific capability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3982ccbf-2610-4482-8a10-527c4eb2b558
📒 Files selected for processing (2)
crates/host_env/src/os.rscrates/vm/src/stdlib/os.rs
joshuamegnauth54
force-pushed
the
host_env-renameat
branch
3 times, most recently
from
08a0fde to
240d1fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host_env/src/os.rs`:
- Around line 285-287: The error creation in crates/host_env/src/os.rs at the
renameat platform unavailability check uses io::Error::other which discards
error kind information. Replace io::Error::other with ErrorKind::Unsupported to
provide proper error semantics. However, this change alone is insufficient: the
errno value EOPNOTSUPP/ENOTSUP generated by Unsupported is not currently mapped
to NotImplementedError in the errno_to_exc_type function in
crates/vm/src/exceptions.rs. To properly match Python's documented behavior, you
must also either add the EOPNOTSUPP/ENOTSUP to NotImplementedError mapping in
the errno_to_exc_type function, or alternatively add an availability check in
the Python rename() wrapper function in crates/vm/src/stdlib/os.rs (following
the pattern used in lines 90-94 for other unavailable functions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 82b07da5-8c69-429c-a975-8d714d53ed37
📒 Files selected for processing (2)
crates/host_env/src/os.rscrates/vm/src/stdlib/os.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/stdlib/os.rs
| Err(io::Error::other( | ||
| "renameat is not available on this platform", | ||
| )) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how host_env io::Error values are converted into Python exceptions.
# Expected: io::ErrorKind::Unsupported is mapped to NotImplementedError, or the os.rename wrapper
# special-cases this unsupported dir_fd error before building an OSError.
rg -n -C4 --type=rust '\bto_os_error_builder\b|ErrorKind::Unsupported|NotImplementedError|dir_fd.*unavailable|unavailable.*dir_fd'Repository: RustPython/RustPython
Length of output: 50377
🏁 Script executed:
# Inspect the context around lines 285-287 in crates/host_env/src/os.rs
head -n 300 crates/host_env/src/os.rs | tail -n 50Repository: RustPython/RustPython
Length of output: 1724
🏁 Script executed:
# Find the implementation of to_os_error_builder and how it maps ErrorKind::Unsupported
rg -A 20 'impl ToOSErrorBuilder for std::io::Error' crates/vm/src/exceptions.rsRepository: RustPython/RustPython
Length of output: 841
🏁 Script executed:
# Check if ErrorKind::Unsupported has special mapping to NotImplementedError
rg -B 5 -A 5 'ErrorKind::Unsupported' crates/vm/src/exceptions.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Get more of the to_os_error_builder implementation to see how error kinds are mapped
rg -A 40 'impl ToOSErrorBuilder for std::io::Error' crates/vm/src/exceptions.rs | head -n 60Repository: RustPython/RustPython
Length of output: 1503
🏁 Script executed:
# Search for how different error kinds are converted to Python exceptions
rg -n 'ErrorKind' crates/vm/src/exceptions.rs | head -n 30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look for the mapping/conversion logic from io errors to Python exceptions
rg -B 3 -A 10 'fn into_pyexception|match.*kind' crates/vm/src/exceptions.rs | head -n 80Repository: RustPython/RustPython
Length of output: 969
🏁 Script executed:
# Find OSErrorBuilder::with_errno and how it maps errno to exception types
rg -B 5 -A 30 'impl OSErrorBuilder' crates/vm/src/exceptions.rs | grep -A 30 'with_errno'Repository: RustPython/RustPython
Length of output: 473
🏁 Script executed:
# Look for how EOPNOTSUPP or ENOTSUP (which represents Unsupported) maps to exceptions
rg -n 'EOPNOTSUPP|ENOTSUP|posix_errno|EINVAL' crates/vm/src/exceptions.rs | head -n 20Repository: RustPython/RustPython
Length of output: 110
🏁 Script executed:
# Check what errno value ErrorKind::Unsupported produces
rg -B 5 -A 10 'fn posix_errno' crates/host_env/src/os.rsRepository: RustPython/RustPython
Length of output: 889
🏁 Script executed:
# Find the errno_to_exc_type function that maps errno values to exception types
rg -B 5 -A 40 'fn errno_to_exc_type' crates/vm/src/exceptions.rsRepository: RustPython/RustPython
Length of output: 2755
🏁 Script executed:
# Also check for any mapping of EOPNOTSUPP/ENOTSUP which is typically errno 45 or 95
rg -n 'libc::EOPNOTSUPP|libc::ENOTSUP|45|95' crates/vm/src/exceptions.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look at the rename function call in crates/vm/src/stdlib/os.rs around line 1409-1416
sed -n '1400,1420p' crates/vm/src/stdlib/os.rsRepository: RustPython/RustPython
Length of output: 691
🏁 Script executed:
# Check if there's a function that parses dir_fd before calling the host rename
sed -n '1350,1420p' crates/vm/src/stdlib/os.rsRepository: RustPython/RustPython
Length of output: 2285
🏁 Script executed:
# Look for where dir_fd is parsed and validated before host_env::os::rename is called
rg -B 30 'host_env::os::rename' crates/vm/src/stdlib/os.rs | head -n 60Repository: RustPython/RustPython
Length of output: 1045
🏁 Script executed:
# Check what errno value io::Error::other uses and what ErrorKind::Unsupported maps to
rg -n 'fn other' crates/host_env/src/os.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look for EOPNOTSUPP or similar errno constants that might be used for Unsupported
rg -n 'EOPNOTSUPP|ENOTSUP' crates/stdlib/src/errno.rs crates/vm/src/stdlib/errno.rs 2>/dev/null || echo "Not found in those files"Repository: RustPython/RustPython
Length of output: 297
🏁 Script executed:
# Check what raw_os_error returns for io::Error::other vs ErrorKind::Unsupported
python3 << 'PY'
import os
import io
# On Linux, EOPNOTSUPP is typically 95, ENOTSUP is 45
# But io::Error::other might not set any errno
print("Looking for how io::Error::other and ErrorKind::Unsupported map to errno values")
# Check common errno values
print("Common errno values:")
print("EOPNOTSUPP (Operation not supported):", 95) # Linux
print("ENOTSUP:", 45) # Some systems
PYRepository: RustPython/RustPython
Length of output: 219
🏁 Script executed:
# Find how io::Error::other creates an error and what errno it sets
rg -B 5 -A 10 'pub fn other' --type rust | head -n 40Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if errno.rs defines EOPNOTSUPP and ENOTSUP values
rg -A 2 -B 2 'EOPNOTSUPP|ENOTSUP' crates/vm/src/stdlib/errno.rs | head -n 20Repository: RustPython/RustPython
Length of output: 495
🏁 Script executed:
# Look at the actual error mapping in errno constants
cat crates/vm/src/stdlib/errno.rs | grep -A 5 -B 5 'EOPNOTSUPP\|ENOTSUP'Repository: RustPython/RustPython
Length of output: 972
🏁 Script executed:
# Check what happens when errno_to_exc_type returns None (default case)
sed -n '1260,1320p' crates/vm/src/exceptions.rsRepository: RustPython/RustPython
Length of output: 1712
Use ErrorKind::Unsupported instead of io::Error::other for better error semantics, but verify downstream mapping to NotImplementedError.
io::Error::other discards error kind information. However, changing to ErrorKind::Unsupported alone won't suffice: the errno value EOPNOTSUPP (or ENOTSUP) generated by Unsupported is not currently mapped to NotImplementedError in errno_to_exc_type (crates/vm/src/exceptions.rs). To match Python's documented behavior for unavailable dir_fd, you'll also need to add this mapping, or add an availability check in the Python rename() wrapper like other functions do (e.g., lines 90–94 in crates/vm/src/stdlib/os.rs).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/host_env/src/os.rs` around lines 285 - 287, The error creation in
crates/host_env/src/os.rs at the renameat platform unavailability check uses
io::Error::other which discards error kind information. Replace io::Error::other
with ErrorKind::Unsupported to provide proper error semantics. However, this
change alone is insufficient: the errno value EOPNOTSUPP/ENOTSUP generated by
Unsupported is not currently mapped to NotImplementedError in the
errno_to_exc_type function in crates/vm/src/exceptions.rs. To properly match
Python's documented behavior, you must also either add the EOPNOTSUPP/ENOTSUP to
NotImplementedError mapping in the errno_to_exc_type function, or alternatively
add an availability check in the Python rename() wrapper function in
crates/vm/src/stdlib/os.rs (following the pattern used in lines 90-94 for other
unavailable functions).
240d1fb to
05fd0ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host_env/src/os.rs`:
- Line 306: The cfg condition at line 306 using `#[cfg(not(unix))]` creates an
overlap with the condition at line 330 `#[cfg(any(unix, target_os = "wasi"))]`
because WASI is not considered unix in Rust's cfg system, causing both to be
true for WASI targets and resulting in duplicate seek_fd definitions.
Additionally, the implementation at line 306 uses Windows-only APIs
(SetFilePointer, crt_fd::as_handle) that don't exist on WASI. Change the cfg
attribute at line 306 from `#[cfg(not(unix))]` back to `#[cfg(windows)]` to
restrict this implementation to Windows targets only, allowing the WASI
implementation at line 330 to be the sole definition for WASI targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3f4846db-4bdf-470c-bf03-9ff608380c15
📒 Files selected for processing (2)
crates/host_env/src/os.rscrates/vm/src/stdlib/os.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/stdlib/os.rs
joshuamegnauth54
force-pushed
the
host_env-renameat
branch
4 times, most recently
from
4a7ded7 to
8fc363e
Compare
Python's `os.rename` supports renaming a path relative to a directory descriptor - essentially, `renameat`. The syscall and its Rustix wrapper does all of the work, so this patch mainly updates the Python signature to forward to the implementation. As of this patch, `os.rename` is still incorrect for Windows because it always replaces the destination file. I added a small note for future contributors (or myself, since I might tackle it) as a reminder of what's broken.
joshuamegnauth54
force-pushed
the
host_env-renameat
branch
from
8fc363e to
118b9b5
Compare
2 participants
Python's
os.renamesupports renaming a path relative to a directory descriptor - essentially,renameat. The syscall and its Rustix wrapper does all of the work, so this patch mainly updates the Python signature to forward to the implementation.As of this patch,
os.renameis still incorrect for Windows because it always replaces the destination file. I added a small note for future contributors (or myself, since I might tackle it) as a reminder of what's broken.Summary
os.rename. See os.rename.Summary by CodeRabbit
os.rename()andos.replace()to accept optionalsrc_dir_fdanddst_dir_fd, supporting renames relative to directory descriptors.renameandreplaceadvertise directory-fd support when available.src_dir_fd/dst_dir_fdnow defaults to the current working directory.