Update test_{descr,super}.py from 3.13.9 - #6314
Conversation
Sorry, something went wrong.
WalkthroughReworked super_check in crates/vm/src/builtins/super.rs to add an initial typ-based path for PyType detection, preserve existing isinstance/class routing, and produce a more descriptive error when obj is not an instance or subtype of the provided type. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
📜 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 c2ea003 and 7c86523. 📒 Files selected for processing (1)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
crates/vm/src/builtins/super.rs (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. ❤️ 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
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 6782fa2 and c2ea003.
⛔ Files ignored due to path filters (2)- Lib/test/test_descr.py is excluded by !Lib/**
- Lib/test/test_super.py is excluded by !Lib/**
- crates/vm/src/builtins/super.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/builtins/super.rs
- 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
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.
Sorry, something went wrong.
| 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.
⚠️ Potential issue | 🟡 Minor
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())
};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.
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.