upgrade shutil #6558
upgrade shutil #6558
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRelocates Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python caller
participant VM as Rust VM (nt.rs)
participant WinAPI as Windows API / Kernel
Note left of Py: call os.chmod(path_or_fd, mode, follow_symlinks?)
Py->>VM: chmod(args)
alt path argument
VM->>VM: resolve path, preserve existing path-based flow
VM->>WinAPI: GetFileAttributesExW / SetFileAttributesW (existing flow)
WinAPI-->>VM: status
VM-->>Py: map to Ok or OSError
else fd argument
VM->>VM: match OsPathOrFd::Fd(fd)
VM->>WinAPI: _get_osfhandle(fd) → HANDLE
WinAPI-->>VM: HANDLE
VM->>WinAPI: GetFileInformationByHandleEx / SetFileInformationByHandle (toggle read-only)
WinAPI-->>VM: status
VM-->>Py: map to Ok or OSError / ValueError (if follow_symlinks unsupported)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/shutil.pyis excluded by!Lib/**Lib/test/test_shutil.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/posix_compat.rs
🧰 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 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_compat.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/posix.rs
🧬 Code graph analysis (3)
crates/vm/src/stdlib/posix_compat.rs (1)
crates/vm/src/stdlib/posix.rs (2)
access(399-431)remove(480-483)
crates/vm/src/stdlib/nt.rs (3)
crates/vm/src/stdlib/winapi.rs (1)
std(455-455)crates/vm/src/ospath.rs (1)
with_filename(306-316)crates/common/src/crt_fd.rs (3)
borrow_raw(101-106)borrow_raw(248-251)as_handle(342-346)
crates/vm/src/stdlib/posix.rs (1)
crates/vm/src/stdlib/posix_compat.rs (1)
remove(30-33)
🔇 Additional comments (6)
crates/vm/src/stdlib/posix.rs (1)
478-483: LGTM!The
unlinkimplementation correctly validates thedir_fdand provides proper error context by including the filename in the error message viaOSErrorBuilder.crates/vm/src/stdlib/os.rs (1)
564-601: LGTM! Good performance optimization.The early-return optimization using cached
file_typeis well-designed:
- Avoids unnecessary
stat()calls that may fail if the file was removed after the directory scan- Correctly handles the
follow_symlinksparameter: cached data is used when not following symlinks, or when the entry is not a symlink- The pattern is consistently applied in both
is_dir()andis_file()methodscrates/vm/src/stdlib/nt.rs (4)
80-130: LGTM! Windows-specific unlink implementation is well-structured.The implementation correctly handles Windows-specific file types:
- Regular files are removed with
DeleteFileW- Directory symlinks and junctions are removed with
RemoveDirectoryW- Reparse point detection using
FindFirstFileWanddwReserved0aligns with CPython's approach- Proper cleanup with
FindCloseafter checking reparse tags- Error messages include filename context via
OSErrorBuilder::with_filename
207-258: LGTM! File descriptor-based chmod implementation is correct.The
fchmod_implfunction properly implements Windows chmod for file descriptors:
- Correctly obtains the Windows
HANDLEfrom the file descriptor- Uses
GetFileInformationByHandleExwithFileBasicInfoto read current attributes- The readonly attribute logic is correct:
S_IWRITEbit clearsFILE_ATTRIBUTE_READONLY, absence sets it- Properly uses
SetFileInformationByHandleto apply changes- Error handling appropriately returns OS errors
261-278: Good integration of fd support into chmod.The function correctly handles both file descriptor and path inputs:
- File descriptor case is handled early with a direct call to
fchmod_impl- Appropriately rejects
follow_symlinksparameter when used with file descriptor (as it's meaningless for fds)- The error message clearly explains why the combination is invalid
320-320: Correct readonly attribute logic.The expression
mode & S_IWRITE == 0correctly determines when to set the readonly attribute (when write permission is absent), matching the logic infchmod_impl.
| #[pyfunction] | ||
| #[pyfunction(name = "unlink")] | ||
| fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> { | ||
| let [] = dir_fd.0; | ||
| fs::remove_file(&path).map_err(|err| err.into_pyexception(vm)) | ||
| } |
There was a problem hiding this comment.
Consider using OSErrorBuilder for consistency and better error messages.
The error handling here uses IntoPyException directly, while the equivalent function in posix.rs (lines 479-482) uses OSErrorBuilder::with_filename(&err, path, vm) to include the filename in error messages. This inconsistency means users on posix_compat platforms will get less helpful error messages.
🔎 Suggested fix
+use crate::exceptions::OSErrorBuilder;
#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
- fs::remove_file(&path).map_err(|err| err.into_pyexception(vm))
+ fs::remove_file(&path).map_err(|err| OSErrorBuilder::with_filename(&err, path, 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.
| #[pyfunction] | |
| #[pyfunction(name = "unlink")] | |
| fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> { | |
| let [] = dir_fd.0; | |
| fs::remove_file(&path).map_err(|err| err.into_pyexception(vm)) | |
| } | |
| use crate::exceptions::OSErrorBuilder; | |
| #[pyfunction] | |
| #[pyfunction(name = "unlink")] | |
| fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> { | |
| let [] = dir_fd.0; | |
| fs::remove_file(&path).map_err(|err| OSErrorBuilder::with_filename(&err, path, vm)) | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/posix_compat.rs around lines 28 to 33, the remove()
function maps fs::remove_file errors using err.into_pyexception(vm) which omits
the filename; replace that mapping with OSErrorBuilder::with_filename(&err,
path, vm).map_err(...) (i.e., construct the OSError via
OSErrorBuilder::with_filename using the original io::Error and the path, then
return that as the PyErr) so error messages match posix.rs and include the
filename for consistency.
Summary by CodeRabbit
New Features
Performance Improvements
✏️ Tip: You can customize this high-level summary in your review settings.