Unraiseable traceback #6449
Unraiseable traceback #6449
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdated the sys module's unraisable-exception hook: prints traceback via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
6127c08 to
c805f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/sys.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/sys.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
⏰ 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 (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
crates/vm/src/stdlib/sys.rs (4)
18-18: LGTM!The
KwArgsimport is correctly added and used for the traceback printing functionality.
703-711: LGTM!The early return when
exc_typeis None is correct, and the assertion ensures type safety for subsequent operations.
724-730: LGTM!The qualname printing with fallback to the type name is correctly implemented.
732-748: LGTM!The exception value formatting with proper separator, error handling for
str()failures, and stderr flushing are all well implemented.
| // Print traceback (using actual exc_traceback, not current stack) | ||
| if !vm.is_none(&unraisable.exc_traceback) { | ||
| let tb_module = vm.import("traceback", 0)?; | ||
| let print_tb = tb_module.get_attr("print_tb", vm)?; | ||
| let stderr_obj = super::get_stderr(vm)?; | ||
| let kwargs: KwArgs = [("file".to_string(), stderr_obj)].into_iter().collect(); | ||
| let _ = print_tb.call( | ||
| FuncArgs::new(vec![unraisable.exc_traceback.clone()], kwargs), | ||
| vm, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Error propagation may suppress unraisable exception output.
The ? operator on vm.import() and get_attr() will cause the entire _unraisablehook to return early on failure, potentially losing the exception type/value output that follows. Consider catching these errors to ensure the hook remains resilient:
// Print traceback (using actual exc_traceback, not current stack)
if !vm.is_none(&unraisable.exc_traceback) {
- let tb_module = vm.import("traceback", 0)?;
- let print_tb = tb_module.get_attr("print_tb", vm)?;
- let stderr_obj = super::get_stderr(vm)?;
- let kwargs: KwArgs = [("file".to_string(), stderr_obj)].into_iter().collect();
- let _ = print_tb.call(
- FuncArgs::new(vec![unraisable.exc_traceback.clone()], kwargs),
- vm,
- );
+ if let Ok(tb_module) = vm.import("traceback", 0)
+ && let Ok(print_tb) = tb_module.get_attr("print_tb", vm)
+ && let Ok(stderr_obj) = super::get_stderr(vm)
+ {
+ let kwargs: KwArgs = [("file".to_string(), stderr_obj)].into_iter().collect();
+ let _ = print_tb.call(
+ FuncArgs::new(vec![unraisable.exc_traceback.clone()], kwargs),
+ vm,
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Print traceback (using actual exc_traceback, not current stack) | |
| if !vm.is_none(&unraisable.exc_traceback) { | |
| let tb_module = vm.import("traceback", 0)?; | |
| let print_tb = tb_module.get_attr("print_tb", vm)?; | |
| let stderr_obj = super::get_stderr(vm)?; | |
| let kwargs: KwArgs = [("file".to_string(), stderr_obj)].into_iter().collect(); | |
| let _ = print_tb.call( | |
| FuncArgs::new(vec![unraisable.exc_traceback.clone()], kwargs), | |
| vm, | |
| ); | |
| } | |
| // Print traceback (using actual exc_traceback, not current stack) | |
| if !vm.is_none(&unraisable.exc_traceback) { | |
| if let Ok(tb_module) = vm.import("traceback", 0) | |
| && let Ok(print_tb) = tb_module.get_attr("print_tb", vm) | |
| && let Ok(stderr_obj) = super::get_stderr(vm) | |
| { | |
| let kwargs: KwArgs = [("file".to_string(), stderr_obj)].into_iter().collect(); | |
| let _ = print_tb.call( | |
| FuncArgs::new(vec![unraisable.exc_traceback.clone()], kwargs), | |
| vm, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/sys.rs around lines 691 to 701, the use of the `?`
operator on `vm.import("traceback", 0)` and `tb_module.get_attr("print_tb", vm)`
can return early and suppress the rest of the unraisable hook output; change
these to explicit error-handling so failures do not propagate out: attempt to
import the "traceback" module with a match/if-let, and if that fails, write a
minimal note or ignore and continue without returning; if import succeeds,
attempt to get the "print_tb" attribute with a match/if-let, and if that fails
similarly continue; only call `print_tb` when both steps succeed, and ensure any
incidental errors are caught (not propagated) and optionally logged to stderr
via `super::get_stderr(vm)?` so the unraisable exception type/value remains
printed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.