Skip to content

Fix enum and os.read related to signal #6552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 2 commits into from
Dec 28, 2025
Merged

Fix enum and os.read related to signal #6552

youknowone merged 2 commits into from
Dec 28, 2025
+60 −34

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 27, 2025 •
edited by coderabbitai bot
Loading

separate from #6535

Summary by CodeRabbit

  • Bug Fixes
    • Fixed dict descriptor duplication in type initialization to properly respect descriptors provided by base classes in the inheritance hierarchy.
    • Enhanced os.read() with improved error handling for interrupted system calls and Python-native exception conversion for better error management.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 27, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Two refinements to VM core: type initialization now guards against re-declaring __dict__ when ancestors already provide it via MRO scanning, and the os module's read function improves error handling by retrying interrupted system calls and converting all I/O errors to Python exceptions.

Changes

Cohort / File(s) Summary
Type initialization dict guard
crates/vm/src/builtins/type.rs		 Added MRO-wide guard when registering __dict__ descriptor: computes has_inherited_dict by scanning all bases for existing __dict__ attribute, preventing re-declaration if any ancestor already provides it. Alters conditional logic for descriptor insertion.
OS module read error handling
crates/vm/src/stdlib/os.rs		 Updated read() function signature from io::Result<PyBytesRef> to PyResult<PyBytesRef>. Refactored implementation to loop on reads, retry on EINTR by signaling VM, and convert all I/O errors to Python exceptions via into_pyexception().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Fix __dict__ getset type #6010: Modifies __dict__ descriptor insertion logic in type initialization by scanning bases/MRO to guard against re-declaration.
  • type.__dict__ #5957: Changes type initialization to add base_is_type check for __dict__ descriptor handling, addressing the same code area.

Poem

🐰 A rabbit hops through types with care,
dict now checks what base will share,
No duplication in the tree,
While reads retry when interrupted be,
Error handling, sleek and fair! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix enum and os.read related to signal' accurately reflects the main changes in the PR: enum-related fixes and os.read signal handling updates.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent 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 fd6c548 and 1f4a359.

⛔ Files ignored due to path filters (5)
  • Lib/test/test_descr.py is excluded by !Lib/**
  • Lib/test/test_file_eintr.py is excluded by !Lib/**
  • Lib/test/test_signal.py is excluded by !Lib/**
  • Lib/test/test_socket.py is excluded by !Lib/**
  • Lib/test/test_ssl.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/vm/src/builtins/type.rs
  • crates/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 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/vm/src/stdlib/os.rs
  • crates/vm/src/builtins/type.rs
🧠 Learnings (1)
📚 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/vm/src/stdlib/os.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/os.rs (1)
crates/common/src/crt_fd.rs (1)
  • read (372-374)
⏰ 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 (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check the WASM package and demo
🔇 Additional comments (2)
crates/vm/src/builtins/type.rs (1)

1502-1510: LGTM: Correct prevention of dict re-declaration from MRO.

The change correctly guards against re-declaring the __dict__ descriptor when any base class in the MRO already provides it. The implementation properly scans all bases in the MRO and only inserts the descriptor when neither the current type nor any ancestor defines it.

The lock acquisition pattern (reading MRO, then each base's attributes, then type's attributes) is safe since these are all read locks with no risk of deadlock.

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

277-292: LGTM: Correctly implements signal-aware read with EINTR retry.

The changes properly handle interrupted system calls by:

  1. Looping on EINTR errors and retrying the read operation
  2. Checking for pending signals via vm.check_signals()? before retry, allowing exceptions like KeyboardInterrupt to be raised
  3. Converting all I/O errors to Python exceptions using into_pyexception(vm)

This matches CPython's behavior for signal-aware I/O operations and ensures that signals are not lost when system calls are interrupted.

The signature change from io::Result<PyBytesRef> to PyResult<PyBytesRef> is necessary to propagate Python exceptions from signal handling.

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.

@youknowone youknowone marked this pull request as ready for review December 28, 2025 00:22
@youknowone youknowone merged commit 012799f into RustPython:main Dec 28, 2025
13 checks passed
@youknowone youknowone deleted the enum-os-read branch December 28, 2025 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone