Add missing _winapi functions#7185
Merged
youknowone merged 7 commits intoRustPython:mainfrom Feb 22, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds extensive Windows WinAPI stdlib bindings and constants, changes Windows handle integer alias to signed, makes Settings.utf8_mode signed with new PYTHONUTF8 parsing and default resolution, adjusts module path for winapi, and a small test file encoding change. Changes
Sequence Diagram(s)sequenceDiagram
participant PyCode as Python code
participant VM as Rust VM stdlib
participant WinAPI as Windows API (kernel)
participant Registry as Windows Registry
PyCode->>VM: call winapi.ReadFile/WriteFile/MapViewOfFile/CopyFile2/etc.
VM->>WinAPI: perform FFI call with WinHandle / args
WinAPI-->>VM: return status / bytes / pointer
VM-->>PyCode: convert results to Python objects (handles, bytes, errors)
VM->>Registry: _mimetypes_read_windows_registry enumerates keys
Registry-->>VM: return key/value entries
VM-->>PyCode: invoke Python callback with batched MIME entries
sequenceDiagram
participant Env as OS environment
participant Startup as VM startup/settings
participant Flags as sys.flags
Env->>Startup: provide PYTHONUTF8 (e.g., "1"/"0"/other)
Startup->>Startup: parse PYTHONUTF8 and -X utf8 -> Settings.utf8_mode (i8)
Startup->>Startup: if Settings.utf8_mode < 0 -> set default utf8_mode = 1
Startup->>Flags: FlagsData.from_settings reads utf8_mode -> set sys.flags.utf8_mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin winapi |
youknowone
force-pushed
the
winapi
branch
3 times, most recently
from
3e83c21 to
6387c4f
Compare
youknowone
changed the title
youknowone
force-pushed
the
winapi
branch
3 times, most recently
from
5404443 to
83bbc0a
Compare
Add 13 functions: ReadFile, SetNamedPipeHandleState, CreateFileMapping, OpenFileMapping, MapViewOfFile, UnmapViewOfFile, VirtualQuerySize, CopyFile2, ResetEvent, CreateMutex, OpenEventW, LoadLibrary, _mimetypes_read_windows_registry. Add constants: INVALID_HANDLE_VALUE, FILE_MAP_READ/WRITE/COPY/EXECUTE. Change WinHandle integer type from usize to isize so negative values like INVALID_HANDLE_VALUE (-1) can be passed from Python.
- Rename winapi.rs to _winapi.rs with #[path] attribute - Rename CreateMutex to CreateMutexW - Add missing constants: ERROR_ACCESS_DENIED, ERROR_PRIVILEGE_NOT_HELD, PROCESS_ALL_ACCESS, 10 STARTF_ constants, LOCALE_NAME_SYSTEM_DEFAULT, LOCALE_NAME_USER_DEFAULT, COPY_FILE_DIRECTORY - Fix OpenMutexW return type and ReleaseMutex param type to use WinHandle
Use FromArgs structs so overlapped parameter can be passed as a keyword argument (overlapped=True), matching the CPython API.
Remove 19 constants (WAIT_ABANDONED, CREATE_ALWAYS, CREATE_NEW, OPEN_ALWAYS, TRUNCATE_EXISTING, FILE_ATTRIBUTE_NORMAL, 8 FILE_FLAG_*, 3 FILE_SHARE_*, NMPWAIT_NOWAIT, NMPWAIT_USE_DEFAULT_WAIT) and LoadLibrary function that are not present in CPython's _winapi module.
youknowone
force-pushed
the
winapi
branch
2 times, most recently
from
390d51e to
d1643ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/vm/src/stdlib/_winapi.rs (2)
282-284:⚠️ Potential issue | 🟠 Major
hStdInput/hStdOutput/hStdErrorextracted asusizebut handles are nowisize.With
HandleIntchanged toisize, Python-visible handles can be negative (e.g.,INVALID_HANDLE_VALUE = -1). Thesi_attr!macro extracts these asOption<usize>, so passing a negative handle (like-1or-11) from Python will fail the conversion. Consider changing the type parameter toisize.Proposed fix
- si_attr!(hStdInput, usize); - si_attr!(hStdOutput, usize); - si_attr!(hStdError, usize); + si_attr!(hStdInput, isize); + si_attr!(hStdOutput, isize); + si_attr!(hStdError, isize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 282 - 284, The si_attr! invocations for hStdInput, hStdOutput, and hStdError extract handles as usize but HandleInt is now isize, so negative Python handle values fail conversion; change the type parameter in those macro calls from usize to isize (update si_attr!(hStdInput, isize), si_attr!(hStdOutput, isize), si_attr!(hStdError, isize)) so the macro yields Option<isize> and preserves negative INVALID_HANDLE_VALUE semantics when passing handles from Python.
1180-1188:⚠️ Potential issue | 🔴 CriticalBug:
CreateEventWreturnsNULLon failure, notINVALID_HANDLE_VALUE.The Win32
CreateEventWAPI returnsNULLon failure (per MSDN). The check at line 1180 againstINVALID_HANDLE_VALUEwill never match a real failure. Consequently, a failed call falls through to theis_null()check at line 1184, which returnsOk(None)— silently swallowing the error instead of raising anOSError.Fix required
- if handle == windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE { - return Err(vm.new_last_os_error()); - } - - if handle.is_null() { - return Ok(None); + if handle.is_null() { + return Err(vm.new_last_os_error()); } Ok(Some(WinHandle(handle)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 1180 - 1188, The code checks for INVALID_HANDLE_VALUE but CreateEventW returns NULL on failure, so change the failure handling in the function around the WIN handle checks: treat a null handle (handle.is_null()) as the error case and return Err(vm.new_last_os_error()), and only return Ok(Some(WinHandle(handle))) for non-null handles; remove or ignore the INVALID_HANDLE_VALUE check (leave WinHandle(handle), vm.new_last_os_error(), and the is_null() check as the referenced symbols to update).
🧹 Nitpick comments (1)
crates/vm/src/stdlib/_winapi.rs (1)
1940-1948: Non-FACILITY_WIN32HRESULTs passed through as raw error codes.When
CopyFile2returns a non-FACILITY_WIN32HRESULT (e.g.,E_OUTOFMEMORY = 0x8007000Eworks, butE_UNEXPECTED = 0x8000FFFFwould pass the raw HRESULT), the resultingio::Errormay produce a confusing message. CPython has similar behavior, so this is acceptable — just noting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 1940 - 1948, The current HRESULT-to-io::Error mapping uses a wrong/magic check ((hr as u32 >> 16) == 0x8007) which incorrectly handles facility bits and can pass raw HRESULTs like E_UNEXPECTED; update the check around the hr variable in this CopyFile2 error branch to test the FACILITY field properly (e.g. ((hr as u32 >> 16) & 0x1FFF) == 7 or use a FACILITY_WIN32 constant) and only then extract the low 16 bits as the Win32 error code, otherwise fall back to using the full HRESULT as before; keep the rest of the Err(std::io::Error::from_raw_os_error(...).to_pyexception(vm)) path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/vm/setting.rs`:
- Line 127: Settings.utf8_mode can be left as -1 in embedded/WASM paths and
currently gets cast to 255_u8 when constructing sys.flags; update
FlagsData::from_settings to clamp/normalize the Settings.utf8_mode value into
the valid unsigned range before casting so -1 becomes a valid sentinel (e.g., 0
or max allowed) instead of 255. Locate the utf8_mode field on Settings and the
FlagsData::from_settings constructor and add a small normalization step (e.g.,
map negative values to a safe default or clamp between 0 and 255) prior to
casting to u8 so sys.flags never exposes 255 from -1.
---
Outside diff comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 282-284: The si_attr! invocations for hStdInput, hStdOutput, and
hStdError extract handles as usize but HandleInt is now isize, so negative
Python handle values fail conversion; change the type parameter in those macro
calls from usize to isize (update si_attr!(hStdInput, isize),
si_attr!(hStdOutput, isize), si_attr!(hStdError, isize)) so the macro yields
Option<isize> and preserves negative INVALID_HANDLE_VALUE semantics when passing
handles from Python.
- Around line 1180-1188: The code checks for INVALID_HANDLE_VALUE but
CreateEventW returns NULL on failure, so change the failure handling in the
function around the WIN handle checks: treat a null handle (handle.is_null()) as
the error case and return Err(vm.new_last_os_error()), and only return
Ok(Some(WinHandle(handle))) for non-null handles; remove or ignore the
INVALID_HANDLE_VALUE check (leave WinHandle(handle), vm.new_last_os_error(), and
the is_null() check as the referenced symbols to update).
---
Duplicate comments:
In `@crates/vm/src/stdlib/sys.rs`:
- Line 1591: The current code blindly casts settings.utf8_mode (an i8) to u8
causing -1 => 255; update the struct initialization that sets utf8_mode (where
you have "utf8_mode: settings.utf8_mode as u8") to explicitly handle the -1
sentinel from settings.utf8_mode: check if settings.utf8_mode < 0 and map that
case to the intended safe value (e.g. 0 or another defined sentinel) or use
Option/u8 semantics, otherwise cast with settings.utf8_mode as u8; keep the
change local to the utf8_mode assignment so callers like sys.rs use the guarded
conversion.
---
Nitpick comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 1940-1948: The current HRESULT-to-io::Error mapping uses a
wrong/magic check ((hr as u32 >> 16) == 0x8007) which incorrectly handles
facility bits and can pass raw HRESULTs like E_UNEXPECTED; update the check
around the hr variable in this CopyFile2 error branch to test the FACILITY field
properly (e.g. ((hr as u32 >> 16) & 0x1FFF) == 7 or use a FACILITY_WIN32
constant) and only then extract the low 16 bits as the Win32 error code,
otherwise fall back to using the full HRESULT as before; keep the rest of the
Err(std::io::Error::from_raw_os_error(...).to_pyexception(vm)) path.
Default utf8_mode was incorrectly set to 1, causing text-mode subprocess to always decode as UTF-8 instead of locale encoding. Changed default to 0 to match CPython 3.13 behavior on Windows. Added PYTHONUTF8 environment variable handling with -X utf8 override.
Use std::io::Error::from_raw_os_error instead of vm.new_os_error so that winerror attribute is set and errno-to-exception mapping works (e.g. ERROR_ACCESS_DENIED → PermissionError).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/vm/src/stdlib/_winapi.rs (1)
1157-1185:CreateEventWreturn typeOption<WinHandle>is neverNone.The function always returns
Ok(Some(WinHandle(handle)))on the happy path andErr(...)on failure;Noneis never produced. The return type could be simplified toPyResult<WinHandle>to match the other similar functions (OpenMutexW,CreateMutexW,OpenEventW) and avoid misleading callers.♻️ Proposed simplification
fn CreateEventW( security_attributes: isize, manual_reset: bool, initial_state: bool, name: Option<PyStrRef>, vm: &VirtualMachine, -) -> PyResult<Option<WinHandle>> { +) -> PyResult<WinHandle> { ... - Ok(Some(WinHandle(handle))) + Ok(WinHandle(handle)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 1157 - 1185, Change CreateEventW's signature and return handling to return PyResult<WinHandle> instead of PyResult<Option<WinHandle>>: update the function signature fn CreateEventW(...) -> PyResult<WinHandle>, and on success return Ok(WinHandle(handle)) (leave error path returning Err(vm.new_last_os_error()) unchanged). Ensure any callers expecting Option<WinHandle> are updated to the plain WinHandle result (consistent with OpenMutexW/CreateMutexW/OpenEventW patterns) and adjust any matching code that handled None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 2047-2051: The batch flush loop can return early when
on_type_read.call(...) raises, leaking the opened registry handle hkcr; fix by
ensuring hkcr is closed before propagating any error from the callback: inside
the entries.drain(..) loop (the block that iterates when entries.len() >= 64)
replace the direct use of the ? on on_type_read.call(...) with explicit error
handling — call on_type_read.call(...), and if it returns Err, call
RegCloseKey(hkcr) and then return the error (or convert/propagate it) so hkcr is
always closed; keep the existing post-loop flush logic as-is.
- Around line 23-28: The import list incorrectly imports STILL_ACTIVE from
Win32::Foundation; move the STILL_ACTIVE symbol into the
Win32::System::Threading import group (i.e., remove STILL_ACTIVE from the
Win32::Foundation import and add it under Win32::System::Threading) so the crate
compiles with windows-sys 0.61.2 (Win32_System_Threading feature is already
enabled).
In `@src/settings.rs`:
- Around line 263-276: The current PYTHONUTF8 handling treats an empty string as
"1"; change the logic in the get_env("PYTHONUTF8") branch so empty strings are
treated as unset: call get_env("PYTHONUTF8"), convert with val.to_str(), skip
processing if the resulting &str is empty, and only match non-empty values "1"
=> settings.utf8_mode = 1 or "0" => settings.utf8_mode = 0 (otherwise log the
existing error and exit); reference get_env, settings.utf8_mode and the
PYTHONUTF8 handling in settings.rs to locate and update the code.
---
Duplicate comments:
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 1591-1595: The sys.flags construction currently casts a sentinel
-1 to u8 producing 255; update the utf8_mode field assignment so that when
settings.utf8_mode < 0 it uses 1, otherwise it uses settings.utf8_mode as u8
(i.e., replace the direct cast with a conditional clamp). Locate the utf8_mode
assignment in crates/vm/src/stdlib/sys.rs (references: settings.utf8_mode and
parse_opts()) and implement the conditional branch to return 1 for negative
values to prevent exposing 255.
---
Nitpick comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 1157-1185: Change CreateEventW's signature and return handling to
return PyResult<WinHandle> instead of PyResult<Option<WinHandle>>: update the
function signature fn CreateEventW(...) -> PyResult<WinHandle>, and on success
return Ok(WinHandle(handle)) (leave error path returning
Err(vm.new_last_os_error()) unchanged). Ensure any callers expecting
Option<WinHandle> are updated to the plain WinHandle result (consistent with
OpenMutexW/CreateMutexW/OpenEventW patterns) and adjust any matching code that
handled None.
Comment on lines
+23
to
+28
| DUPLICATE_CLOSE_SOURCE, DUPLICATE_SAME_ACCESS, ERROR_ACCESS_DENIED, | ||
| ERROR_ALREADY_EXISTS, ERROR_BROKEN_PIPE, ERROR_IO_PENDING, ERROR_MORE_DATA, | ||
| ERROR_NETNAME_DELETED, ERROR_NO_DATA, ERROR_NO_SYSTEM_RESOURCES, | ||
| ERROR_OPERATION_ABORTED, ERROR_PIPE_BUSY, ERROR_PIPE_CONNECTED, | ||
| ERROR_PRIVILEGE_NOT_HELD, ERROR_SEM_TIMEOUT, GENERIC_READ, GENERIC_WRITE, STILL_ACTIVE, | ||
| WAIT_ABANDONED_0, WAIT_OBJECT_0, WAIT_TIMEOUT, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the Cargo.toml for the vm crate
fd Cargo.toml -x grep -l "vm" {} \;Repository: RustPython/RustPython
Length of output: 305
🏁 Script executed:
# Look at the vm crate's Cargo.toml to find windows-sys version
cat crates/vm/Cargo.toml | grep -A 5 -B 5 "windows-sys"Repository: RustPython/RustPython
Length of output: 304
🏁 Script executed:
# Examine the actual imports in the _winapi.rs file
head -50 crates/vm/src/stdlib/_winapi.rsRepository: RustPython/RustPython
Length of output: 2558
🏁 Script executed:
# Find the workspace Cargo.toml to check windows-sys version
cat Cargo.toml | grep -A 5 "windows-sys"Repository: RustPython/RustPython
Length of output: 145
🏁 Script executed:
# Also check if there's a workspace member dependency file
grep -r "windows-sys" Cargo.toml --include="Cargo.toml" | head -5Repository: RustPython/RustPython
Length of output: 87
🌐 Web query:
windows-sys 0.61.2 STILL_ACTIVE Win32::Foundation
💡 Result:
In windows-sys 0.61.2, STILL_ACTIVE is not in Win32::Foundation; it’s associated with process/thread exit codes and is exposed under Win32::System::Threading.
Use:
use windows_sys::Win32::System::Threading::STILL_ACTIVE;Cargo features:
[dependencies.windows-sys]
version = "0.61.2"
features = ["Win32_System_Threading"]STILL_ACTIVE is the value returned by GetExitCodeProcess (and GetExitCodeThread) when the process/thread hasn’t terminated yet, and its value is 259. [1]
Sources: windows-sys crate docs (feature-gating / module usage) [2], Microsoft docs for GetExitCodeProcess / STILL_ACTIVE (259) [1].
[1] https://learn.microsoft.com/tr-tr/windows/win32/api/processthreadsapi/nf-processthreadsapi-getexitcodeprocess
[2] https://docs.rs/crate/windows-sys/latest
🏁 Script executed:
# Check if there are any local windows-sys files in the project
find . -path "*/target" -prune -o -path "*/.cargo" -prune -o -name "*.rs" -type f -exec grep -l "STILL_ACTIVE" {} \; 2>/dev/null | head -10Repository: RustPython/RustPython
Length of output: 98
🏁 Script executed:
# Search for STILL_ACTIVE references in the codebase
rg "STILL_ACTIVE" --type rs -nRepository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Verify the feature flags in vm crate Cargo.toml
cat crates/vm/Cargo.toml | grep -A 20 "windows-sys"Repository: RustPython/RustPython
Length of output: 627
🏁 Script executed:
# Check if Win32_System_Threading is enabled
rg "Win32_System_Threading" --type tomlRepository: RustPython/RustPython
Length of output: 226
🏁 Script executed:
# Final check: verify the exact line where STILL_ACTIVE is imported
sed -n '20,30p' crates/vm/src/stdlib/_winapi.rsRepository: RustPython/RustPython
Length of output: 646
Move STILL_ACTIVE import from Win32::Foundation to Win32::System::Threading.
In windows-sys 0.61.2, STILL_ACTIVE is located under Win32::System::Threading, not Win32::Foundation. The current import will fail to compile on Windows. Since Win32_System_Threading is already enabled in the feature flags, move the import to the correct module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/_winapi.rs` around lines 23 - 28, The import list
incorrectly imports STILL_ACTIVE from Win32::Foundation; move the STILL_ACTIVE
symbol into the Win32::System::Threading import group (i.e., remove STILL_ACTIVE
from the Win32::Foundation import and add it under Win32::System::Threading) so
the crate compiles with windows-sys 0.61.2 (Win32_System_Threading feature is
already enabled).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 1964-1993: The scopeguard::defer! that runs unsafe {
RegCloseKey(hkcr) } already ensures hkcr is closed on all return paths, so
remove the redundant explicit unsafe { RegCloseKey(hkcr) } inside the
RegEnumKeyExW error branch; keep the error handling logic (return
Err(vm.new_os_error(err as i32))) intact so the defer will run once and avoid
double-closing the hkcr handle.
---
Duplicate comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 23-28: The import list still pulls STILL_ACTIVE from
Win32::Foundation but in windows-sys 0.61.2 STILL_ACTIVE is defined under
Win32::System::Threading; update the imports so that STILL_ACTIVE is
re-exported/used from Win32::System::Threading (not Win32::Foundation) in the
_winapi.rs import block, ensuring the feature flag Win32_System_Threading
remains used and that any references to STILL_ACTIVE continue to compile.
Use explicit encoding='latin-1' so the test works regardless of the system locale (e.g. C/POSIX locale uses ASCII by default).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores