Update test_{descr,super}.py from 3.13.9
#6314
Conversation
WalkthroughReworked Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ 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). (11)
🔇 Additional comments (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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_super.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/builtins/super.rs(1 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/builtins/super.rs
⏰ 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). (11)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
crates/vm/src/builtins/super.rs (1)
269-274: LGTM! Enhanced error message provides better debugging information.The error message now clearly distinguishes between type and instance arguments and includes their names, making it much easier to diagnose
super()misuse.
crates/vm/src/builtins/super.rs
Outdated
| let is_type = match obj.clone().downcast::<PyType>() { | ||
| Ok(cls) if cls.fast_issubclass(&ty) => return Ok(cls), | ||
| Ok(_) => true, | ||
| Err(_) => false, | ||
| }; | ||
|
|
||
| if obj.fast_isinstance(&ty) { | ||
| return Ok(obj.class().to_owned()); | ||
| } | ||
|
|
||
| let class_attr = obj.get_attr("__class__", vm)?; | ||
| if let Ok(cls) = class_attr.downcast::<PyType>() | ||
| && !cls.is(&ty) | ||
| && cls.fast_issubclass(&ty) | ||
| { | ||
| return Ok(cls); | ||
| } | ||
| Err(vm.new_type_error("super(type, obj): obj must be an instance or subtype of type")) | ||
|
|
||
| let (type_or_instance, obj_str) = if is_type { | ||
| // SAFETY: This is will always be successful as we already checked at the begining of | ||
| // the function. | ||
| let typ = unsafe { obj.downcast::<PyType>().unwrap_unchecked() }; | ||
| ("type", typ.name().to_owned()) | ||
| } else { | ||
| ("instance of", obj.class().name().to_owned()) | ||
| }; |
There was a problem hiding this comment.
Fix typo and avoid unsafe downcast by storing the type.
Line 261 has a typo: "This is will always be successful" should be "This will always be successful".
More importantly, the unsafe downcast at line 263 can be avoided. The current code discards the downcasted PyType in the Ok(_) branch at line 244, then later performs an unsafe downcast to retrieve it. Instead, store the downcasted type when is_type is true and reuse it, eliminating the need for unsafe code.
Apply this diff to fix the typo and avoid unsafe:
- let is_type = match obj.clone().downcast::<PyType>() {
+ let obj_type = match obj.clone().downcast::<PyType>() {
Ok(cls) if cls.fast_issubclass(&ty) => return Ok(cls),
- Ok(_) => true,
- Err(_) => false,
+ Ok(cls) => Some(cls),
+ Err(_) => None,
};
+ let is_type = obj_type.is_some();
if obj.fast_isinstance(&ty) {
return Ok(obj.class().to_owned());
}
let class_attr = obj.get_attr("__class__", vm)?;
if let Ok(cls) = class_attr.downcast::<PyType>()
&& !cls.is(&ty)
&& cls.fast_issubclass(&ty)
{
return Ok(cls);
}
- let (type_or_instance, obj_str) = if is_type {
- // SAFETY: This is will always be successful as we already checked at the begining of
- // the function.
- let typ = unsafe { obj.downcast::<PyType>().unwrap_unchecked() };
- ("type", typ.name().to_owned())
+ let (type_or_instance, obj_str) = if let Some(typ) = obj_type {
+ ("type", typ.name().to_owned())
} else {
("instance of", obj.class().name().to_owned())
};🤖 Prompt for AI Agents
crates/vm/src/builtins/super.rs around lines 242-267: the code currently throws
away the successful downcasted PyType in the match and later does an unsafe
downcast and has a typo in the comment; change the match to capture and store
the downcasted PyType (e.g. Option<PyTypeRef> or similar) instead of a boolean
flag, use that stored PyType when building type_or_instance (avoiding unsafe
downcast), update the comment to "This will always be successful", and remove
the unsafe unwrap_unchecked usage so the stored safe reference is reused.
Summary by CodeRabbit
super()when given mismatched type/instance combinations to avoid spurious checks and preserve existing subclass/instance routing.super(type, obj)calls, providing clearer descriptions of whetherobjis a type or instance and naming the offending types for easier troubleshooting.✏️ Tip: You can customize this high-level summary in your review settings.