host_env: os.replace for Windows#8212
Conversation
📝 WalkthroughWalkthroughShared POSIX/WASI filesystem helpers were extracted into ChangesRename and filesystem helper unification across platforms
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/posix_windows.rs`:
- Around line 34-36: The unused-variable lint suppression in posix_windows.rs is
using the wrong cfg predicate, so it applies in the wrong builds and can leave
expectations unmet. Update the cfg_attr on the parameters in the Windows POSIX
path helpers to use debug_assertions instead of debug_assert, and make the same
correction for the matching attributes around the later parameter block so the
`expect(unused_variables)` only applies in non-debug builds.
- Around line 60-66: The rename_impl helper has an inconsistent signature and
uses the wrong wide-string conversion for Windows path handling. Update
rename_impl so both from and to use impl AsRef<Path>, and switch the path
conversion to WideCString::from_os_str for each input before invoking
MoveFileExW. Keep the fix localized in rename_impl in posix_windows.rs and
ensure the resulting wide strings are passed through correctly.
In `@crates/vm/src/stdlib/os.rs`:
- Around line 81-84: The generic DirFd parsing currently accepts rename-specific
keywords, which causes the first flattened DirFd in RenameArgs to consume
dst_dir_fd or src_dir_fd incorrectly. Update the DirFd argument handling in
os.rs so it only reads dir_fd, and move src_dir_fd/dst_dir_fd parsing into the
RenameArgs-specific path that owns those two flattened fields. Use the DirFd and
RenameArgs parsing logic to ensure single-fd APIs reject rename-only keywords
and rename calls map each keyword to the correct field.
🪄 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: 31a0e703-2ab5-4cc1-8e15-f21cc06d3e8a
📒 Files selected for processing (7)
crates/host_env/src/lib.rscrates/host_env/src/os.rscrates/host_env/src/posix.rscrates/host_env/src/posix_unix_wasi.rscrates/host_env/src/posix_wasi.rscrates/host_env/src/posix_windows.rscrates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (1)
- crates/host_env/src/os.rs
698bea5 to
41d2d58
Compare
`rename` and `replace` are the same for Unixes but different for Windows in Python. POSIX's `rename` atomically replaces its target; there is no `replace` in POSIX. `renameat2` with `RENAME_NOREPLACE` atomically checks if a file exists and renames if it doesn't, but Python's `rename` and `replace` predate `renameat2`. For Windows, `rename` acts like the `RENAME_NOREPLACE` flag while `replace` functions like `rename`. I implemented both using the Windows API. Both implementations follow CPython's code by using `MoveFileExW`. There are modern Windows APIs that may be worth using in the future. Rust's standard library prefers the modern APIs but falls back to `MoveFileExW` for deprecated Windows versions.
41d2d58 to
99a6517
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/os.rs (1)
1414-1468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared rename/replace path handling.
renameandreplacenow duplicate conversion, dir-fd extraction, andOSErrorconstruction; only the function name and host operation differ.♻️ Proposed refactor
+ fn rename_or_replace<'fd>( + function: &'static str, + args: RenameArgs<'fd>, + vm: &VirtualMachine, + op: impl FnOnce( + &OsPath, + Option<crt_fd::Borrowed<'fd>>, + &OsPath, + Option<crt_fd::Borrowed<'fd>>, + ) -> io::Result<()>, + ) -> PyResult<()> { + let src = PathConverter::new() + .function(function) + .argument("src") + .try_path(args.src, vm)?; + let dst = PathConverter::new() + .function(function) + .argument("dst") + .try_path(args.dst, vm)?; + + #[cfg(any(unix, target_os = "wasi"))] + let src_dir_fd = args.src_dir_fd.get_opt(); + #[cfg(not(any(unix, target_os = "wasi")))] + let src_dir_fd = None; + + #[cfg(any(unix, target_os = "wasi"))] + let dst_dir_fd = args.dst_dir_fd.get_opt(); + #[cfg(not(any(unix, target_os = "wasi")))] + let dst_dir_fd = None; + + op(&src, src_dir_fd, &dst, dst_dir_fd).map_err(|err| { + let builder = err.to_os_error_builder(vm); + let builder = builder.filename(src.filename(vm)); + let builder = builder.filename2(dst.filename(vm)); + builder.build(vm).upcast() + }) + } + #[pyfunction] fn rename(args: RenameArgs<'_>, vm: &VirtualMachine) -> PyResult<()> { - let src = PathConverter::new() - .function("rename") - .argument("src") - .try_path(args.src, vm)?; - let dst = PathConverter::new() - .function("rename") - .argument("dst") - .try_path(args.dst, vm)?; - - #[cfg(any(unix, target_os = "wasi"))] - let src_dir_fd = args.src_dir_fd.get_opt(); - #[cfg(not(any(unix, target_os = "wasi")))] - let src_dir_fd = None; - - #[cfg(any(unix, target_os = "wasi"))] - let dst_dir_fd = args.dst_dir_fd.get_opt(); - #[cfg(not(any(unix, target_os = "wasi")))] - let dst_dir_fd = None; - - crate::host_env::posix::rename(&src, src_dir_fd, &dst, dst_dir_fd).map_err(|err| { - let builder = err.to_os_error_builder(vm); - let builder = builder.filename(src.filename(vm)); - let builder = builder.filename2(dst.filename(vm)); - builder.build(vm).upcast() - }) + rename_or_replace("rename", args, vm, crate::host_env::posix::rename) } #[pyfunction] fn replace(args: RenameArgs<'_>, vm: &VirtualMachine) -> PyResult<()> { - let src = PathConverter::new() - .function("replace") - .argument("src") - .try_path(args.src, vm)?; - let dst = PathConverter::new() - .function("replace") - .argument("dst") - .try_path(args.dst, vm)?; - - #[cfg(any(unix, target_os = "wasi"))] - let src_dir_fd = args.src_dir_fd.get_opt(); - #[cfg(not(any(unix, target_os = "wasi")))] - let src_dir_fd = None; - - #[cfg(any(unix, target_os = "wasi"))] - let dst_dir_fd = args.dst_dir_fd.get_opt(); - #[cfg(not(any(unix, target_os = "wasi")))] - let dst_dir_fd = None; - - crate::host_env::posix::replace(&src, src_dir_fd, &dst, dst_dir_fd).map_err(|err| { - let builder = err.to_os_error_builder(vm); - let builder = builder.filename(src.filename(vm)); - let builder = builder.filename2(dst.filename(vm)); - builder.build(vm).upcast() - }) + rename_or_replace("replace", args, vm, crate::host_env::posix::replace) }As per coding guidelines, “When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code.”
🤖 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/vm/src/stdlib/os.rs` around lines 1414 - 1468, The rename and replace stdlib functions duplicate the same path conversion, dir-fd extraction, and OSError mapping logic in os.rs; extract the shared flow into a helper used by both `rename` and `replace`, with only the operation-specific parts differing. Keep `PathConverter`, `RenameArgs`, and the `crate::host_env::posix::{rename, replace}` calls as the unique inputs, and centralize the error builder setup so filename handling and `to_os_error_builder` are not repeated.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/vm/src/stdlib/os.rs`:
- Around line 1414-1468: The rename and replace stdlib functions duplicate the
same path conversion, dir-fd extraction, and OSError mapping logic in os.rs;
extract the shared flow into a helper used by both `rename` and `replace`, with
only the operation-specific parts differing. Keep `PathConverter`, `RenameArgs`,
and the `crate::host_env::posix::{rename, replace}` calls as the unique inputs,
and centralize the error builder setup so filename handling and
`to_os_error_builder` are not repeated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 527af374-e782-4110-8c34-e7a6458ffe4a
📒 Files selected for processing (7)
crates/host_env/src/lib.rscrates/host_env/src/os.rscrates/host_env/src/posix.rscrates/host_env/src/posix_unix_wasi.rscrates/host_env/src/posix_wasi.rscrates/host_env/src/posix_windows.rscrates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (1)
- crates/host_env/src/os.rs
✅ Files skipped from review due to trivial changes (1)
- crates/host_env/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/host_env/src/posix_unix_wasi.rs
- crates/host_env/src/posix_wasi.rs
- crates/host_env/src/posix.rs
- crates/host_env/src/posix_windows.rs
renameandreplaceare the same for Unixes but different for Windows in Python. POSIX'srenameatomically replaces its target; there is noreplacein POSIX.renameat2withRENAME_NOREPLACEatomically checks if a file exists and renames if it doesn't, but Python'srenameandreplacepredaterenameat2.For Windows,
renameacts like theRENAME_NOREPLACEflag whilereplacefunctions likerename. I implemented both using the Windows API. Both implementations follow CPython's code by usingMoveFileExW. There are modern Windows APIs that may be worth using in the future. Rust's standard library prefers the modern APIs but falls back toMoveFileExWfor deprecated Windows versions.Summary
os.replacefor Windows.Summary by CodeRabbit
New Features
Bug Fixes