Update isintance #5868
Update isintance #5868
Conversation
WalkthroughA public accessor method Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PyObject
participant PyUnion
Caller->>PyObject: is_subclass(cls)
alt cls is PyUnion
PyObject->>PyUnion: args()
PyObject->>PyObject: is_subclass(member) for each member in args
else cls is tuple
PyObject->>PyObject: is_subclass(member) for each member in tuple
else
PyObject->>PyObject: __subclasscheck__ or default check
end
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vm/src/builtins/union.rs(1 hunks)vm/src/protocol/object.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
vm/src/protocol/object.rs (3)
vm/src/protocol/sequence.rs (1)
tuple(279-289)vm/src/builtins/union.rs (1)
__subclasscheck__(122-136)vm/src/builtins/type.rs (2)
__subclasscheck__(1247-1251)typ(1053-1061)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (5)
vm/src/builtins/union.rs (1)
43-47: LGTM! Well-implemented accessor method.The new
args()method provides clean, direct access to the union's arguments and properly aligns with CPython's_Py_union_argsapproach. The implementation follows Rust conventions with appropriate inlining and documentation.vm/src/protocol/object.rs (4)
500-506: Variable rename improves clarity.The rename from
selftoderivedmakes the subclass checking logic more explicit and readable.
532-539: Variable naming improvements enhance readability.The standardization of variable names (
meth→checker,ret→res) improves code consistency and clarity.
605-615: Consistent union handling implementation.The refactoring to use direct downcasting and the new
args()method is well-implemented and consistent with theis_subclasschanges. However, the same.expect()concern applies here.The logic correctly handles union types, but consider the same defensive error handling suggestion as in
is_subclass:- let union = cls - .try_to_ref::<crate::builtins::PyUnion>(vm) - .expect("checked by is"); + let union = cls + .try_to_ref::<crate::builtins::PyUnion>(vm) + .map_err(|_| vm.new_type_error("Invalid union type"))?;
510-519: Union downcasting.expect()is safe hereThe preceding
cls.class().is(vm.ctx.types.union_type)check guarantees that the object is indeed aPyUnion, so thedowncast_ref(...).expect(...)can never fail in normal operation. This is an internal invariant check rather than user-facing error handling, matching the existing pattern (e.g. theexpectcalls invm/src/frame.rsfor tuple downcasts). No change required.
| if let Some(tuple) = cls.downcast_ref::<PyTuple>() { | ||
| for item in tuple { | ||
| if vm.with_recursion("in __subclasscheck__", || derived.is_subclass(item, vm))? { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent tuple handling between methods.
The is_subclass method uses downcast_ref::<PyTuple>() while is_instance (line 618) uses try_to_ref::<PyTuple>(vm). This inconsistency could lead to different error handling behavior.
Standardize tuple handling across both methods. Consider using the same approach:
- if let Some(tuple) = cls.downcast_ref::<PyTuple>() {
+ if let Ok(tuple) = cls.try_to_ref::<PyTuple>(vm) {Or alternatively, update is_instance to match this pattern for consistency.
📝 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.
| if let Some(tuple) = cls.downcast_ref::<PyTuple>() { | |
| for item in tuple { | |
| if vm.with_recursion("in __subclasscheck__", || derived.is_subclass(item, vm))? { | |
| return Ok(true); | |
| } | |
| } | |
| return Ok(false); | |
| } | |
| if let Ok(tuple) = cls.try_to_ref::<PyTuple>(vm) { | |
| for item in tuple { | |
| if vm.with_recursion("in __subclasscheck__", || derived.is_subclass(item, vm))? { | |
| return Ok(true); | |
| } | |
| } | |
| return Ok(false); | |
| } |
🤖 Prompt for AI Agents
In vm/src/protocol/object.rs around lines 522 to 529, the method uses
downcast_ref::<PyTuple>() to handle tuples, while is_instance at line 618 uses
try_to_ref::<PyTuple>(vm), causing inconsistent tuple handling and error
behavior. To fix this, standardize tuple handling by either changing is_subclass
to use try_to_ref with the vm parameter like is_instance, or update is_instance
to use downcast_ref without vm, ensuring both methods handle tuples the same way
and maintain consistent error handling.
Summary by CodeRabbit
Bug Fixes
New Features