GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Update `test_{descr,super}.py` from 3.13.9 by ShaharNaveh · Pull Request #6314 · RustPython/RustPython 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
youknowone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.