◐ Shell
clean mode source ↗

various fix to support subprocess/multiprocessing by youknowone · Pull Request #6567 · RustPython/RustPython

youknowone

📜 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 feb5066 and be5631e.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_hmac.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • crates/stdlib/src/hashlib.rs
  • crates/stdlib/src/mmap.rs
  • crates/vm/src/stdlib/os.rs
  • crates/vm/src/stdlib/thread.rs
🧰 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 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/stdlib/src/mmap.rs
  • crates/vm/src/stdlib/os.rs
  • crates/stdlib/src/hashlib.rs
  • crates/vm/src/stdlib/thread.rs
🧠 Learnings (4)
📚 Learning: 2025-11-10T06:27:41.954Z 
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Applied to files:

  • crates/stdlib/src/mmap.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/os.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/os.rs
📚 Learning: 2025-12-27T14:03:49.011Z 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

  • crates/stdlib/src/hashlib.rs
🧬 Code graph analysis (2)
crates/stdlib/src/mmap.rs (1)
crates/vm/src/protocol/buffer.rs (1)
  • simple (176-184)
crates/stdlib/src/hashlib.rs (1)
crates/vm/src/class.rs (1)
  • static_type (71-79)
🔇 Additional comments (9)
crates/stdlib/src/mmap.rs (1)

654-657: Consider whether AccessMode::Copy should also produce a readonly buffer.

The current logic only sets readonly = true for AccessMode::Read. However, AccessMode::Copy creates a copy-on-write mapping where writes don't affect the underlying file. In CPython, ACCESS_COPY mmaps are still considered writable buffers since the user can modify the in-memory copy.

The implementation looks correct for matching CPython behavior, where only ACCESS_READ produces a truly readonly buffer.

crates/vm/src/stdlib/thread.rs (3)

258-265: LGTM!

The _recursion_count method correctly returns the acquisition depth when owned by the current thread, and 0 otherwise. This matches CPython's RLock._recursion_count() behavior and is useful for debugging and introspection.

218-223: LGTM!

The count is correctly incremented only when acquisition succeeds. The Relaxed ordering is appropriate here since the mutex operations provide the necessary synchronization.

244-244: LGTM!

Resetting the count to 0 during _at_fork_reinit is correct—after a fork, the child process should see a clean lock state.

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

25-29: LGTM!

The UnsupportedDigestmodError exception follows the RustPython macro pattern for custom exceptions. Deriving from PyValueError is appropriate, and the #[repr(transparent)] ensures correct memory layout. This enables Python's hmac module to catch this exception and fall back to a pure-Python HMAC implementation.

Based on learnings, the macro system (pyexception) is correctly used for implementing Python functionality in Rust.

343-361: No changes needed—exception is properly initialized.

The #[pyattr] decorator on UnsupportedDigestmodError ensures it is automatically registered as a module attribute when the _hashlib module loads. This is the standard RustPython pattern, consistent with other exception definitions throughout the codebase (e.g., in crates/stdlib/src/ssl/error.rs). The static_type() call in hmac_new() will not panic because the exception is guaranteed to be initialized before any module functions can be called.

crates/vm/src/stdlib/os.rs (3)

205-212: LGTM!

The ST_RDONLY and ST_NOSUID constants are correctly guarded with #[cfg(all(unix, not(target_os = "redox")))], following the platform-specific constant pattern used throughout the codebase. Based on retrieved learnings, this matches CPython's approach of optional declaration for platform-specific constants.

1713-1731: LGTM!

The StatvfsResultData struct correctly mirrors Python's os.statvfs_result named tuple. The #[pystruct_sequence(skip)] annotation on f_fsid ensures it's accessible as an attribute but not included in the tuple indices, matching CPython's behavior.

1740-1746: LGTM!

The slot_new implementation follows the standard pattern for struct sequences, enabling Python-side construction of statvfs_result objects.