Skip __class__ lookup in object_isinstance for standard getattro#7303
Skip __class__ lookup in object_isinstance for standard getattro#7303youknowone merged 1 commit intoRustPython:mainfrom
Conversation
In object_isinstance(), when is_subtype() returns false, the __class__ attribute lookup via get_attribute_opt is redundant for objects using standard __getattribute__, since __class__ is a data descriptor on object that always returns obj.class().
📝 WalkthroughWalkthroughThe change optimizes attribute lookup behavior in Python's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/protocol/object.rs (1)
568-598:⚠️ Potential issue | 🟠 MajorSkip
__class__lookup optimization is semantically incorrect and diverges from CPython.RustPython's optimization to skip
__class__lookup whenhas_standard_getattro()is true (lines 571 and 590) contradicts CPython's behavior. In CPython'sobject_isinstance, attribute lookup viaPyObject_GetOptionalAttris always performed, regardless of whether__getattribute__is standard or custom. This matters because a subclass can define__class__as a custom descriptor/property at the class level, and the standard__getattribute__will still invoke the descriptor protocol, returning the custom value. By skipping the lookup when getattro is standard, RustPython assumes__class__always resolves toself.class(), which is incorrect when custom__class__descriptors exist.To align with CPython, remove the
!self.has_standard_getattro()condition and always perform the__class__lookup viavm.get_attribute_opt(), as shown in the provided diff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/protocol/object.rs` around lines 568 - 598, The current optimization skips looking up __class__ when has_standard_getattro() is true, which diverges from CPython; update the logic in the object_isinstance path that currently branches on has_standard_getattro() to always call vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__)) and use its result to determine i_cls (falling back to self.class() only if lookup fails), remove the conditional gating on has_standard_getattro(), and ensure the subsequent code still converts i_cls via PyTypeRef::try_from_object and calls i_cls.abstract_issubclass(cls, vm) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/vm/src/protocol/object.rs`:
- Around line 568-598: The current optimization skips looking up __class__ when
has_standard_getattro() is true, which diverges from CPython; update the logic
in the object_isinstance path that currently branches on has_standard_getattro()
to always call vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__))
and use its result to determine i_cls (falling back to self.class() only if
lookup fails), remove the conditional gating on has_standard_getattro(), and
ensure the subsequent code still converts i_cls via PyTypeRef::try_from_object
and calls i_cls.abstract_issubclass(cls, vm) as before.
1 participant
In object_isinstance(), when is_subtype() returns false, the class attribute lookup via get_attribute_opt is redundant for objects using standard getattribute, since class is a data descriptor on object that always returns obj.class().
Summary by CodeRabbit
isinstancechecks by streamlining attribute lookup paths for standard attribute access scenarios.