windows umask, win32_xstat_slow_impl, fake EXT_SUFFIX#6340
Conversation
Sorry, something went wrong.
WalkthroughAdds Windows-specific stat/xstat implementation and helpers, exposes a winsock initializer, adds an FFI-backed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Sorry, something went wrong.
youknowone
force-pushed
the
nt
branch
2 times, most recently
from
8dcbecb to
0c70492
Compare
December 7, 2025 16:56
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/vm/src/windows.rs (2)
400-408: Type mismatch: using local struct with windows_sys enum constant.Line 405 uses
FileAttributeTagInfowhich is imported as aFILE_INFO_BY_HANDLE_CLASSenum variant from windows_sys (line 283), but you're using it as both the enum value (line 405) and casting to a local struct type (line 401, 406). This works because the enum value happens to match the expected constant, but the struct being zeroed is the localFileAttributeTagInfo, not the windows_sys structure.This is fragile and confusing. Consider:
+// Use explicit constant for clarity +const FILE_ATTRIBUTE_TAG_INFO_CLASS: i32 = 9; // FileAttributeTagInfo enum value let ret = unsafe { GetFileInformationByHandleEx( h_file, - FileAttributeTagInfo, + FILE_ATTRIBUTE_TAG_INFO_CLASS, &mut local_tag_info as *mut _ as *mut _, std::mem::size_of::<FileAttributeTagInfo>() as u32, ) };Or rename the local struct to avoid the collision entirely.
370-371: Consider RAII wrapper for handle cleanup.The closure pattern for ensuring handle cleanup works, but it's somewhat unusual in Rust. A more idiomatic approach would be to use an RAII wrapper that automatically closes the handle on drop:
struct OwnedHandle(RAW_HANDLE); impl Drop for OwnedHandle { fn drop(&mut self) { if self.0 != INVALID_HANDLE_VALUE { unsafe { CloseHandle(self.0) }; } } }This is optional since the current approach is correct; it's just a style suggestion for future maintenance.
Also applies to: 493-500
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_genericpath.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_ntpath.pyis excluded by!Lib/**Lib/test/test_shutil.pyis excluded by!Lib/**
📒 Files selected for processing (4)
.cspell.dict/cpython.txt(1 hunks)crates/vm/src/stdlib/nt.rs(1 hunks)crates/vm/src/stdlib/sysconfig.rs(1 hunks)crates/vm/src/windows.rs(3 hunks)
🧰 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 Rust 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/nt.rscrates/vm/src/windows.rscrates/vm/src/stdlib/sysconfig.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/stdlib/nt.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/sysconfig.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/sysconfig.rs
🧬 Code graph analysis (2)
crates/vm/src/stdlib/nt.rs (1)
crates/vm/src/stdlib/posix.rs (1)
umask(1328-1330)
crates/vm/src/windows.rs (1)
crates/vm/src/stdlib/winapi.rs (2)
CloseHandle(83-85)GetFileType(163-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (5)
.cspell.dict/cpython.txt (1)
49-49: LGTM!The SOABI token is correctly added to the dictionary in alphabetical order. This aligns with the new SOABI config variable introduced in
sysconfig.rs.crates/vm/src/windows.rs (4)
113-130: LGTM!The
attributes_to_modefunction correctly ports CPython's logic for converting Windows file attributes to Unix-style mode bits.
132-227: LGTM!The
attribute_data_to_statfunction is a comprehensive port of CPython's_Py_attribute_data_to_stat. The time conversion logic correctly handles the Windows-to-Unix epoch difference, and the 128-bit file ID support viast_ino_highis properly implemented.
229-267: LGTM!The
attributes_from_dirfallback correctly usesFindFirstFileWwhenCreateFileWfails due to access/sharing issues. The handle cleanup is properly handled.
534-535: LGTM!The fallback to
win32_xstat_slow_implis correctly integrated, providing robust handling when the fast path (GetFileInformationByName) cannot be used.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/vm/src/windows.rs (1)
105-111:FileAttributeTagInfonaming is slightly confusing but not incorrect.Defining a local
struct FileAttributeTagInfowhile also importing theFileAttributeTagInfoenum value fromwindows_sysis valid (type vs value namespace), but it’s easy to misread. Consider a small rename of the struct (e.g.LocalFileAttributeTagInfo) to make the distinction clearer for future readers.Also applies to: 400-408
🧹 Nitpick comments (2)
crates/vm/src/stdlib/sysconfig.rs (1)
11-21: EXT_SUFFIX stub is knowingly wrong; please follow up with a platform-aware implementation.The
.pydvalue andSOABI = Nonemake_sysconfigdiverge from CPython on all non‑Windows platforms (and even on Windows, it’s missing ABI tagging). Since the FIXME already calls this out and the PR title mentions “fake EXT_SUFFIX”, it’s fine as a temporary stub, but this will need a follow‑up that:
- Computes EXT_SUFFIX conditionally per target (Windows vs Unix), and
- Keeps EXT_SUFFIX and SOABI consistent (e.g. EXT_SUFFIX derived from SOABI).
Classifying as a reminder rather than a blocker for this PR.
crates/vm/src/windows.rs (1)
231-267:attributes_from_dirfallback is correct but could benefit from a brief comment on limitations.The FindFirstFileW-based fallback populates a minimal
BY_HANDLE_FILE_INFORMATIONand reparse tag when CreateFileW fails for access/sharing reasons, which matches CPython’s approach.A tiny nit: a short comment noting that volume serial / file index aren’t available via this path (and will keep their zeroed defaults) would clarify why inode/dev fields may differ vs the direct handle-based path.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_genericpath.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_ntpath.pyis excluded by!Lib/**Lib/test/test_shutil.pyis excluded by!Lib/**
📒 Files selected for processing (4)
.cspell.dict/cpython.txt(1 hunks)crates/vm/src/stdlib/nt.rs(1 hunks)crates/vm/src/stdlib/sysconfig.rs(1 hunks)crates/vm/src/windows.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/stdlib/nt.rs
- .cspell.dict/cpython.txt
🧰 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 Rust 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/sysconfig.rscrates/vm/src/windows.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sysconfig.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sysconfig.rs
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
crates/vm/src/stdlib/sysconfig.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (2)
crates/vm/src/windows.rs (2)
96-130: Mode mapping from Windows attributes looks correct and matches CPython semantics.The new constants and
attributes_to_mode()logic (directory vs regular file, read‑only vs writable, execute bits for directories) align with CPython’sattributes_to_modebehavior. This is a solid foundation for the rest of the stat translation.
270-501:win32_xstat_slow_impland fallback wiring inwin32_xstat_impllook faithful to CPython.
- Error handling around
CreateFileWmirrors CPython’s special cases (access denied/sharing violation → directory fallback, invalid parameter → retry with GENERIC_READ,ERROR_CANT_ACCESS_FILEwithtraverse→ unhandled reparse handling).- Non‑disk handles (char devices, pipes, unknown type) are mapped to appropriate
st_modebits and short‑circuit with a defaultedStatStruct.- Reparse tag probing with
GetFileInformationByHandleEx(FileAttributeTagInfo, ...)plus theis_unhandled_taglogic and recursive traversal for non‑link reparse points matches the CPython control flow.- Optional
FILE_BASIC_INFOandFILE_ID_INFOare used when available; failures downgrade gracefully.- Handle lifetime is correctly managed: all successful open paths are closed once, and recursive
win32_xstat_slow_impl(path, true)calls closeh_filebefore recursing, avoiding leaks/double‑closes.Overall this is a strong, careful port; I don’t see functional bugs in the flow.
It’d be worth running the existing CPython Windows stat/xstat test suite (or RustPython’s equivalent) against this implementation on real Windows targets, particularly cases involving:
- Junctions and non‑symlink reparse points,
- Unhandled tags with
traverse=True/False,- Special paths like
\\.\CONand named pipes.
Sorry, something went wrong.
youknowone
merged commit 876368e
into
RustPython:main
Dec 8, 2025
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.