Fix windows socket by youknowone · Pull Request #6408 · RustPython/RustPython · GitHub

Summary by CodeRabbit

Release Notes

API Changes
- Socket methods fileno() and detach() now return integer values instead of raw socket objects. Code using these methods may require updates.
Refactoring
- Internal path handling implementation updated with improved buffer handling mechanisms.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Two public method signatures updated for improved type handling: Socket methods detach() and fileno() now return i64 instead of RawSocket, and the path splitting utility _path_splitroot now returns Wtf8Buf pairs instead of String pairs with a PyResult wrapper.

Changes

Cohort / File(s)	Summary
Socket API Return Type Updates crates/stdlib/src/socket.rs	Updated public method signatures: detach() and fileno() now return i64 instead of RawSocket. Implementation adjusted to convert socket file descriptors and invalid socket constants to i64. Call sites in close() and repr_str adapted to handle the new return types.
Path Splitting Buffer Refactor crates/vm/src/stdlib/nt.rs	Refactored _path_splitroot signature to return (Wtf8Buf, Wtf8Buf) instead of PyResult<(String, String)>. Parameter vm renamed to _vm (unused). Implementation now uses Wtf8Buf::from_wide for buffer handling instead of String decoding via UTF-16 conversion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify type consistency across socket method call sites, particularly in close() and repr_str() where type conversions occur
Confirm Wtf8Buf conversions in _path_splitroot preserve correct buffer semantics and handle edge cases (empty paths, invalid inputs)
Ensure no downstream callers of these methods are affected by the signature changes

Suggested reviewers

ShaharNaveh

Poem

🐰 A socket's file now dances with i64,
And paths split wide through buffers galore,
Where Wtf8 buffers take String's old place,
The refactor hops with encoding grace! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix windows socket' is vague and overly generic, failing to convey the specific nature of the changes made to socket return types and related Windows path handling.	Provide a more descriptive title that specifies the main changes, such as 'Change socket fileno/detach return types to i64' or 'Update socket API to use i64 instead of RawSocket'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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 and usage tips.

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/stdlib/src/socket.rs (1)
1462-1468: Verify Windows SOCKET handle range assumptions.

The code assumes that socket handles fit within i64::MAX on Windows. While this is true in practice (Windows SOCKET handles are kernel handles that won't exceed this range), the cast from usize (which SOCKET is on 64-bit Windows) to i64 could theoretically overflow for values above i64::MAX.

Consider adding a debug assertion or comment documenting this assumption.
 #[pymethod]
 fn close(&self) -> io::Result<()> {
     let sock = self.detach();
+    // Windows SOCKET handles are kernel handles that always fit in i64
     if sock != INVALID_SOCKET as i64 {
         close_inner(sock as RawSocket)?;
     }
     Ok(())
 }

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ff1fba and d5fb695.

⛔ Files ignored due to path filters (3)

Lib/test/test_selectors.py is excluded by !Lib/**
Lib/test/test_socketserver.py is excluded by !Lib/**
Lib/test/test_zipimport.py is excluded by !Lib/**

📒 Files selected for processing (2)

crates/stdlib/src/socket.rs (2 hunks)
crates/vm/src/stdlib/nt.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 running cargo fmt to 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.rs
crates/stdlib/src/socket.rs

🧬 Code graph analysis (1) crates/vm/src/stdlib/nt.rs (1)

crates/wtf8/src/lib.rs (1)

from_wide (336-352)

⏰ 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 rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Check Rust code with clippy
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

🔇 Additional comments (3)

crates/stdlib/src/socket.rs (2)

1068-1068: LGTM!

The repr_str formatting correctly uses zelf.fileno() directly now that fileno() returns i64 instead of RawSocket.

1478-1483: The fileno() method correctly returns i64 for consistency with detach().

The implementation properly handles closed sockets by returning INVALID_SOCKET as i64 (-1) and correctly casts valid socket file descriptors to i64. The existing caller in __repr__ (line 1068) is compatible with the i64 return type.

crates/vm/src/stdlib/nt.rs (1)

1125-1131: The function design is correct; no caller updates are needed.

This is a #[pyfunction] exposed to Python, not an internal Rust API. The #[pyfunction] macro automatically converts types implementing ToPyObject (including Wtf8Buf and tuples thereof) to Python objects. The return type (Wtf8Buf, Wtf8Buf) correctly produces a Python tuple of two strings without requiring manual conversion by callers—there are no Rust callers to update.

The _vm parameter is unused but follows the established pattern in the codebase (e.g., re.rs, gc.rs, typing.rs, os.rs, imp.rs) where unused parameters are retained for API consistency.

Likely an incorrect or invalid review comment.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n --type rust '\.detach\s*\(' -B 3 -A 3