Fix sys.excepthook #5830
Fix sys.excepthook #5830
Conversation
WalkthroughThe changes update exception handling behavior in both the compiler and runtime. The compiler now clears exception variables after a handler completes, matching CPython semantics. The runtime’s Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant sys.excepthook
participant VM
User->>sys.excepthook: Call with exception info
sys.excepthook->>VM: normalize_exception()
alt Normalization succeeds
sys.excepthook->>VM: Write exception to stderr
else Normalization fails
sys.excepthook->>VM: Write TypeError message to stderr (with type info)
end
sequenceDiagram
participant Compiler
participant HandlerBody
Compiler->>HandlerBody: Compile exception handler body
HandlerBody-->>Compiler: Handler completes
Compiler->>Compiler: Emit PopException
alt Exception variable bound
Compiler->>Compiler: Set variable to None
Compiler->>Compiler: Delete variable
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm/src/stdlib/sys.rs (1)
367-383: LGTM! Exception normalization error handling aligns with CPython behavior.The change correctly implements graceful handling of exception normalization failures by printing an error message to stderr instead of propagating the exception, which matches CPython's behavior. The error message format and stderr output approach are appropriate.
Minor suggestions for improvement:
- Consider using a more descriptive variable name than
excin the success case for consistency- The TODO comment on line 374 could be addressed to improve the error message accuracy
Apply this diff to improve variable naming consistency:
- match vm.normalize_exception(exc_type.clone(), exc_val.clone(), exc_tb) { - Ok(exc) => vm.write_exception(&mut crate::py_io::PyWriter(stderr, vm), &exc), + match vm.normalize_exception(exc_type.clone(), exc_val.clone(), exc_tb) { + Ok(normalized_exc) => vm.write_exception(&mut crate::py_io::PyWriter(stderr, vm), &normalized_exc),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Lib/test/test_sys.py(0 hunks)compiler/codegen/src/compile.rs(1 hunks)vm/src/stdlib/sys.rs(1 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_sys.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style; use 'cargo fmt' to format Rust code. Always run clippy to lint Rust code ('cargo clippy') before completing tasks, and fix any war...
**/*.rs: Follow the default rustfmt code style; use '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 your changes.
vm/src/stdlib/sys.rscompiler/codegen/src/compile.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (1)
compiler/codegen/src/compile.rs (1)
1320-1327: LGTM! Excellent implementation of CPython-compatible exception variable cleanup.This change correctly implements the CPython behavior of clearing exception variables after an except handler completes. The approach is well-structured:
- Proper sequencing: First sets the variable to
None, then deletes it, matching CPython's exact behavior- Conditional execution: Only performs cleanup when an exception variable was actually bound
- Correct placement: Happens after
PopExceptionbut before finally block handling- Error handling: Properly propagates errors from both
store_nameandcompile_nameoperationsThis addresses the exception variable lifecycle semantics that are important for consistent Python behavior and helps fix issues related to
sys.excepthookhandling.
7580150 to
a57328e
Compare
Summary by CodeRabbit