Skip __class__ lookup in object_isinstance for standard getattro - #7303
Skip __class__ lookup in object_isinstance for standard getattro#7303youknowone merged 1 commit into
Conversation
Sorry, something went wrong.
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().
📝 Walkthrough
WalkthroughThe change optimizes attribute lookup behavior in Python's isinstance and issubclass operations by introducing a helper method that detects standard __getattribute__ usage, allowing short-circuit evaluation of __class__ lookups when possible, while also adding PyBaseObject to module imports. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
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 | 🟠 Major
Skip __class__ lookup optimization is semantically incorrect and diverges from CPython.
RustPython's optimization to skip __class__ lookup when has_standard_getattro() is true (lines 571 and 590) contradicts CPython's behavior. In CPython's object_isinstance, attribute lookup via PyObject_GetOptionalAttr is 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 to self.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 via vm.get_attribute_opt(), as shown in the provided diff.
🤖 Prompt for AI AgentsVerify 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.
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.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between d0b5a5a and 3b49a33.
📒 Files selected for processing (1)- crates/vm/src/protocol/object.rs
Sorry, something went wrong.
…tPython#7303) 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().
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