Make SystemExit.code a writable attribute set at init#8282
Conversation
📝 WalkthroughWalkthroughSystemExit now stores a writable ChangesSystemExit code lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExitAPI
participant VirtualMachine
participant PySystemExit
ExitAPI->>VirtualMachine: invoke SystemExit with exit arguments
VirtualMachine->>PySystemExit: initialize writable code
PySystemExit-->>VirtualMachine: raised SystemExit
VirtualMachine->>PySystemExit: read code
PySystemExit-->>VirtualMachine: integer, None, or other value
VirtualMachine-->>ExitAPI: exit status or printable message
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/exceptions.rs (1)
1658-1676: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
BaseException_initnote to thePyBaseException::slot_initcall below. The current placement describes the match above, not the call it refers to.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/exceptions.rs` around lines 1658 - 1676, Move the “Call BaseException_init first (handles args)” comment from above the code match in PySystemExit::slot_init to immediately above the PyBaseException::slot_init call, so it documents the call it refers to.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/vm/src/exceptions.rs`:
- Around line 1658-1676: Move the “Call BaseException_init first (handles args)”
comment from above the code match in PySystemExit::slot_init to immediately
above the PyBaseException::slot_init call, so it documents the call it refers
to.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6a9aa971-2421-42b9-9cad-54153f7ebee2
📒 Files selected for processing (1)
crates/vm/src/exceptions.rs
youknowone
left a comment
There was a problem hiding this comment.
looks great, thanks! welcome to RustPython project!
note about the change: once we couldn't set getset to exception subtypes. that restriction was resolved early this year, but this one was not updated for a while.
Summary
SystemExit.codewas a read-only getset recomputed fromargsonevery access, so assigning or deleting
exc.coderaisedAttributeError, and mutatingexc.argsafter init changedcode.It is now stored in a dedicated struct field (
#[repr(C)]with aPyBaseExceptionbase, the same approach asOSError), set at__init__time — writable, deletable, and independent fromargsafter init, as in CPython. Keeping it in a field rather than the
instance dict leaves
SystemExit().__dict__empty, matching CPython.A second commit updates
handle_exit_exceptionto read the exitstatus from the
codeattribute like CPython'shandle_system_exit;without it, reassigning
exc.codewould not affect the process exitstatus:
Test plan
sys.exit(3) / sys.exit('msg') / raise SystemExit(1, 2), and the re-raise case above.
now created via the full constructor path, not new_exception).
Assisted-by: Claude Code:claude-fable-5
Summary by CodeRabbit
Summary by CodeRabbit
SystemExitduring termination, including finalization and non-main threads.SystemExit.code(withNonemapping to0; integer values supported).SystemExitoutput is generated more reliably from thecodevalue when present.SystemExitcreated viaexit(and internal thread exit) now follows the same raising path, ensuring consistent behavior with provided exit values.