init debug helper - #6315
Conversation
Sorry, something went wrong.
WalkthroughAdds debug-only diagnostics to downcast/init error paths in two VM modules: captures class names, inspects error messages for duplicate class occurrences, and augments/panics with a suggestion about types that may not support init or subclassing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 080a2f6 and d5af480. 📒 Files selected for processing (2)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)crates/vm/src/types/slot.rs (1)📜 Review details866-866: TODO comment lacks specificity.
The comment mentions a potential safety concern about zelf but doesn't explain what the concern is or under what conditions it might be unsafe. This makes it difficult to evaluate or address the issue.
Please clarify the safety concern with more details. For example:
// TODO: check if this is safe. When __init__ is called on an uninitialized or // incorrectly typed object, zelf might not have the expected payload type. // Consider whether zelf should be `PyObjectRef` with explicit type checking.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between b84f7c1 and 080a2f6.
📒 Files selected for processing (2)- crates/vm/src/types/slot.rs (1 hunks)
- crates/vm/src/vm/vm_new.rs (1 hunks)
📄 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 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/types/slot.rs
- crates/vm/src/vm/vm_new.rs
crates/vm/src/stdlib/ctypes/field.rs (2)⏰ 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). (1)crates/derive-impl/src/error.rs (1)
- zelf (96-97)
- zelf (196-197)
- panic (106-112)
- GitHub Check: Run snippets and cpython tests (windows-latest)
crates/vm/src/vm/vm_new.rs (1)512-512: LGTM! Helpful debug diagnostic message.
The extended error message provides useful guidance for developers encountering downcast failures when class IDs match, pointing them toward the #[pyclass(with(Constructor))] attribute as a potential solution.
Sorry, something went wrong.
| #[cfg(debug_assertions)] | ||
| let class_name_for_debug = zelf.class().name().to_string(); | ||
|
|
||
| let zelf = match zelf.try_into_value(vm) { | ||
| Ok(zelf) => zelf, | ||
| Err(err) => { | ||
| #[cfg(debug_assertions)] | ||
| { | ||
| let msg = err.as_object().repr(vm)?; | ||
| let double_appearance = | ||
| msg.as_str().matches(&class_name_for_debug as &str).count() == 2; | ||
| if double_appearance { | ||
| panic!( | ||
| "This type `{}` doesn't seem to support `init`. Override `slot_init` instead: {}", | ||
| class_name_for_debug, msg | ||
| ); | ||
| } | ||
| } | ||
| return Err(err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Fragile error detection heuristic with potential issues.
The debug diagnostic logic has several concerns:
-
Line 846: err.as_object().repr(vm)? can fail, which would replace the original error with a repr failure error, masking the actual problem.
-
Line 848: Counting class name occurrences in the error message is fragile and depends on exact error message formatting. This heuristic could produce false positives or miss legitimate cases if error message formats change.
-
Lines 850-853: Using panic! in error handling is unconventional. In debug builds, this immediately crashes rather than allowing normal error propagation, which could complicate debugging in some scenarios.
Consider these alternatives:
- Use a more robust detection method (e.g., error type checking rather than message parsing)
- Log a warning instead of panicking to preserve normal error flow
- Handle the repr() failure gracefully to avoid masking the original error
Apply this diff to handle repr() failure safely:
#[cfg(debug_assertions)]
{
- let msg = err.as_object().repr(vm)?;
+ if let Ok(msg) = err.as_object().repr(vm) {
- let double_appearance =
- msg.as_str().matches(&class_name_for_debug as &str).count() == 2;
- if double_appearance {
- panic!(
- "This type `{}` doesn't seem to support `init`. Override `slot_init` instead: {}",
- class_name_for_debug, msg
- );
+ let double_appearance =
+ msg.as_str().matches(&class_name_for_debug as &str).count() == 2;
+ if double_appearance {
+ panic!(
+ "This type `{}` doesn't seem to support `init`. Override `slot_init` instead: {}",
+ class_name_for_debug, msg
+ );
+ }
}
}crates/vm/src/types/slot.rs around lines 838 to 858: the debug diagnostic currently calls err.as_object().repr(vm)? (which can fail and mask the original error), uses fragile string-occurrence heuristics to detect the class-name, and panics on detection; change this to first try to inspect the error in a non-fallible way (prefer checking the error type via downcast/pattern-match or a stable error variant rather than parsing repr), if you must render repr() call wrap it in a safe fallible path (e.g., match or map_err) so a repr failure is ignored/logged and the original err is preserved, replace panic! with a non-aborting diagnostic (log::warn! or vm logger) that includes any safely-obtained context, and always return Err(err) so normal error propagation is preserved.
Sorry, something went wrong.
There was a problem hiding this comment.
@coderabbitai please apply it for me
Sorry, something went wrong.
There was a problem hiding this comment.
@youknowone, I'll provide the complete fixed version of the slot_init function. Here's the updated code with the safer error handling:
#[pyslot]
#[inline]
fn slot_init(zelf: PyObjectRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> {
#[cfg(debug_assertions)]
let class_name_for_debug = zelf.class().name().to_string();
let zelf = match zelf.try_into_value(vm) {
Ok(zelf) => zelf,
Err(err) => {
#[cfg(debug_assertions)]
{
if let Ok(msg) = err.as_object().repr(vm) {
let double_appearance =
msg.as_str().matches(&class_name_for_debug as &str).count() == 2;
if double_appearance {
panic!(
"This type `{}` doesn't seem to support `init`. Override `slot_init` instead: {}",
class_name_for_debug, msg
);
}
}
}
return Err(err);
}
};
let args: Self::Args = args.bind(vm)?;
Self::init(zelf, args, vm)
}What changed:
- Wrapped err.as_object().repr(vm)? with if let Ok(msg) = err.as_object().repr(vm)
- This prevents the repr() call from failing and masking the original error
- If repr() fails, the diagnostic is simply skipped and the original error is returned
- The indentation of the subsequent lines is adjusted to be inside the if let block
This makes the debug diagnostic more robust and ensures the original error is always preserved.
Sorry, something went wrong.
#4884
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.