Skip to content

host_env: os.replace for Windows#8212

Draft
joshuamegnauth54 wants to merge 1 commit into
RustPython:mainfrom
joshuamegnauth54:host_env-correct-rename-replace
Draft

host_env: os.replace for Windows#8212
joshuamegnauth54 wants to merge 1 commit into
RustPython:mainfrom
joshuamegnauth54:host_env-correct-rename-replace

Conversation

@joshuamegnauth54

@joshuamegnauth54 joshuamegnauth54 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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.

Summary

  • Implement os.replace for Windows.

Summary by CodeRabbit

  • New Features

    • Added/expanded atomic rename and replace support across Unix, WASI, and Windows, including directory-aware operations (when directory fds are provided).
    • Improved directory descriptor handling for rename/replace by supporting additional argument aliases and using the appropriate source/destination descriptor consistently.
  • Bug Fixes

    • Improved cross-platform behavior for directory-based rename/replace, including WASI.
    • Enhanced Windows rename/replace to better match “move/replace” semantics and provide richer error context (including both source and destination names).

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Shared POSIX/WASI filesystem helpers were extracted into posix_unix_wasi, re-exported from posix and posix_wasi, and the old os.rs rename implementation was removed. Windows rename/replace support was added, and vm stdlib os.rs now routes rename/replace through host_env with WASI dir-fd handling.

Changes

Rename and filesystem helper unification across platforms

Layer / File(s) Summary
Shared POSIX/WASI helpers
crates/host_env/src/lib.rs, crates/host_env/src/posix.rs, crates/host_env/src/posix_wasi.rs, crates/host_env/src/posix_unix_wasi.rs
Adds a shared Unix/WASI helper module, exports it from lib.rs, and re-exports the shared helpers and RawMode.
Remove legacy host_env rename
crates/host_env/src/os.rs
Deletes the old host_env rename wrapper and its Unix-only AsFd import.
Windows rename and replace
crates/host_env/src/posix_windows.rs
Switches make_dir to RawMode and adds rename/replace wrappers backed by MoveFileExW.
vm stdlib rename and dir-fd wiring
crates/vm/src/stdlib/os.rs
Adds keyword-typed DirFd parsing, broadens WASI rename dir-fd availability, updates mkdir and DirEntry construction, and routes rename/replace through host_env with both filenames in errors.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: youknowone, coolreader18

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately captures the main change: adding Windows support for os.replace in host_env.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 045a6d5 and 698bea5.

📒 Files selected for processing (7)
  • crates/host_env/src/lib.rs
  • crates/host_env/src/os.rs
  • crates/host_env/src/posix.rs
  • crates/host_env/src/posix_unix_wasi.rs
  • crates/host_env/src/posix_wasi.rs
  • crates/host_env/src/posix_windows.rs
  • crates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (1)
  • crates/host_env/src/os.rs

Comment thread crates/host_env/src/posix_windows.rs Outdated
Comment thread crates/host_env/src/posix_windows.rs Outdated
Comment thread crates/vm/src/stdlib/os.rs Outdated
@joshuamegnauth54 joshuamegnauth54 force-pushed the host_env-correct-rename-replace branch from 698bea5 to 41d2d58 Compare July 6, 2026 01:45
@joshuamegnauth54 joshuamegnauth54 marked this pull request as draft July 6, 2026 01:51
`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.
@joshuamegnauth54 joshuamegnauth54 marked this pull request as ready for review July 7, 2026 02:23
@joshuamegnauth54 joshuamegnauth54 force-pushed the host_env-correct-rename-replace branch from 41d2d58 to 99a6517 Compare July 7, 2026 02:23
@joshuamegnauth54 joshuamegnauth54 marked this pull request as draft July 7, 2026 02:26

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/vm/src/stdlib/os.rs (1)

1414-1468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared rename/replace path handling.

rename and replace now duplicate conversion, dir-fd extraction, and OSError construction; 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

📥 Commits

Reviewing files that changed from the base of the PR and between 41d2d58 and 99a6517.

📒 Files selected for processing (7)
  • crates/host_env/src/lib.rs
  • crates/host_env/src/os.rs
  • crates/host_env/src/posix.rs
  • crates/host_env/src/posix_unix_wasi.rs
  • crates/host_env/src/posix_wasi.rs
  • crates/host_env/src/posix_windows.rs
  • crates/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant