Fix locale.getencoding, overlapped.getresult, chdir for windows #6629
Conversation
📝 WalkthroughWalkthroughThis pull request introduces three platform-specific enhancements to stdlib modules: a locale encoding detection function with Windows/Unix/mobile fallbacks, an overlapped I/O result synchronization method with complex error handling, and per-drive environment variable propagation in the Windows Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
youknowone
force-pushed
the
fix-windows-
branch
from
0a9e7f6 to
013de57
Compare
youknowone
force-pushed
the
fix-windows-
branch
from
013de57 to
e19ba5f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/stdlib/src/locale.rs (1)
52-53: Consider reusing existingGetACPwrapper.The
GetACPfunction is already exposed incrates/vm/src/stdlib/winapi.rs(line 148-150). While the current approach of importing directly fromwindows_sysworks, consider consistency with the codebase.crates/stdlib/src/overlapped.rs (1)
374-383: Minor inefficiency inReadresult handling.The code allocates a new
Vecand creates new bytes whentransferreddiffers from buffer length. This is correct but creates an extra allocation. Consider whether the original buffer could be truncated in-place if the underlying implementation allows it.This is a minor optimization opportunity and not blocking.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/_test_multiprocessing.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/stdlib/src/locale.rscrates/stdlib/src/overlapped.rscrates/vm/src/stdlib/os.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/os.rscrates/stdlib/src/overlapped.rscrates/stdlib/src/locale.rs
🧬 Code graph analysis (1)
crates/stdlib/src/locale.rs (1)
crates/vm/src/stdlib/winapi.rs (1)
GetACP(149-151)
⏰ 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 (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (5)
crates/stdlib/src/overlapped.rs (3)
157-200:unparse_addressimplementation is functional but has a minor note.The function correctly parses both IPv4 and IPv6 addresses stored in
SOCKADDR_IN6. The_addr_lenparameter is unused but might be needed for validation in future (e.g., ensuring the address structure is fully populated).The IPv6 formatting doesn't apply zero-compression (e.g.,
::1instead of0:0:0:0:0:0:0:1), but this matches the simple formatting approach and is acceptable for debugging/display purposes.
309-326:getresulterror messages could be more informative.The validation logic is correct, but consider aligning error messages with CPython's
overlapped.c:
"operation not yet attempted"→ matches CPython"operation failed to start"→ CPython might phrase this differently
346-370: Error handling forERROR_BROKEN_PIPEis comprehensive.The logic correctly allows
ERROR_BROKEN_PIPEfor read operations while propagating it as an error for other operation types. The nested match arms forReadFromandReadFromIntochecking forNoneresults handle edge cases appropriately.crates/stdlib/src/locale.rs (1)
265-299: Implementation ofgetencodingis correct and matches CPython behavior.The platform-specific logic correctly handles:
- Windows: Returns
cp{ACP}format usingGetACP()- Unix (standard): Uses
nl_langinfo(CODESET)with UTF-8 fallback- iOS/Android/Redox: Returns hardcoded
UTF-8The
let-chainssyntax used (lines 285-288) is fully supported by the project's configured MSRV (1.89.0) with edition 2024, which meets the requirement for this feature.Likely an incorrect or invalid review comment.
crates/vm/src/stdlib/os.rs (1)
1127-1160: Code is correct—no actionable issues identified.The implementation properly mirrors CPython's
win32_wchdir()behavior:
SetEnvironmentVariableW return value: Intentionally ignored as a best-effort operation, consistent with CPython.
Drive letter assumption: The code safely assumes
cwd_wide[0]is a valid drive letter becauseenv::current_dir()callsGetCurrentDirectoryW, which returns only UNC paths (handled separately) or drive letter paths—never extended-length paths (\\?\). Extended-length prefixes are only added by APIs likecanonicalize()or explicit operations, not byGetCurrentDirectoryW.Silent
current_dir()failure: Acceptable as this is best-effort post-chdir fixup behavior matching CPython.
| OverlappedData::ReadFrom(rf) => { | ||
| // Return (resized_buffer, (host, port)) tuple | ||
| let buf = rf | ||
| .allocated_buffer | ||
| .downcast_ref::<crate::vm::builtins::PyBytes>() | ||
| .unwrap(); | ||
| let bytes = buf.as_bytes(); | ||
| let resized_buf = if transferred as usize != bytes.len() { | ||
| vm.ctx.new_bytes(bytes[..transferred as usize].to_vec()) | ||
| } else { | ||
| buf.to_owned() | ||
| }; | ||
| let addr_tuple = unparse_address(&rf.address, rf.address_length, vm); | ||
| Ok(vm | ||
| .ctx | ||
| .new_tuple(vec![resized_buf.into(), addr_tuple]) | ||
| .into()) | ||
| } |
There was a problem hiding this comment.
Potential panic from unwrap() on downcast.
Line 417 uses .unwrap() on downcast_ref::<PyBytes>(). If rf.allocated_buffer is not a PyBytes object (due to a logic error elsewhere), this will panic.
Consider using a safer pattern or adding a comment explaining why this is guaranteed to succeed.
🔎 Suggested safer alternative
- let buf = rf
- .allocated_buffer
- .downcast_ref::<crate::vm::builtins::PyBytes>()
- .unwrap();
+ let buf = rf
+ .allocated_buffer
+ .downcast_ref::<crate::vm::builtins::PyBytes>()
+ .ok_or_else(|| vm.new_type_error("expected bytes object".to_owned()))?;📝 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.
| OverlappedData::ReadFrom(rf) => { | |
| // Return (resized_buffer, (host, port)) tuple | |
| let buf = rf | |
| .allocated_buffer | |
| .downcast_ref::<crate::vm::builtins::PyBytes>() | |
| .unwrap(); | |
| let bytes = buf.as_bytes(); | |
| let resized_buf = if transferred as usize != bytes.len() { | |
| vm.ctx.new_bytes(bytes[..transferred as usize].to_vec()) | |
| } else { | |
| buf.to_owned() | |
| }; | |
| let addr_tuple = unparse_address(&rf.address, rf.address_length, vm); | |
| Ok(vm | |
| .ctx | |
| .new_tuple(vec![resized_buf.into(), addr_tuple]) | |
| .into()) | |
| } | |
| OverlappedData::ReadFrom(rf) => { | |
| // Return (resized_buffer, (host, port)) tuple | |
| let buf = rf | |
| .allocated_buffer | |
| .downcast_ref::<crate::vm::builtins::PyBytes>() | |
| .ok_or_else(|| vm.new_type_error("expected bytes object".to_owned()))?; | |
| let bytes = buf.as_bytes(); | |
| let resized_buf = if transferred as usize != bytes.len() { | |
| vm.ctx.new_bytes(bytes[..transferred as usize].to_vec()) | |
| } else { | |
| buf.to_owned() | |
| }; | |
| let addr_tuple = unparse_address(&rf.address, rf.address_length, vm); | |
| Ok(vm | |
| .ctx | |
| .new_tuple(vec![resized_buf.into(), addr_tuple]) | |
| .into()) | |
| } |
🤖 Prompt for AI Agents
In crates/stdlib/src/overlapped.rs around lines 412 to 429, the code uses
.unwrap() on rf.allocated_buffer.downcast_ref::<crate::vm::builtins::PyBytes>()
which can panic if the object is not PyBytes; replace the unwrap with a safe
check (e.g., match or if let) that handles the None case by returning a
Python-level error (TypeError or ValueError) via the vm or otherwise converting
the object to bytes safely, ensuring no panic; include a concise error message
describing the unexpected buffer type and proceed to construct resized_buf only
after confirming the downcast succeeded.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.