fix(python): remove broken __instancecheck__/__subclasscheck__ from _ObjectSlotsMeta#498
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a fundamental flaw in the Python FFI layer's type checking, where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…ObjectSlotsMeta _ObjectSlotsMeta.__instancecheck__ and __subclasscheck__ unconditionally returned True for any CObject instance/subclass, regardless of which class was being checked. This caused incorrect behavior such as isinstance(Map(...), Array) returning True. The fix removes both methods entirely. They are unnecessary because all objects returned from C++ are always constructed as proper Object subclasses (never raw CObject instances), so standard Python MRO-based isinstance/issubclass checks work correctly. - Remove __instancecheck__ and __subclasscheck__ from _ObjectSlotsMeta - Update test_dataclass_init.py to assert correct behavior (parent is not an instance of child) - Add 22 new tests in TestIsinstanceIssubclass covering containers, inheritance, and cross-hierarchy isinstance/issubclass checks
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in _ObjectSlotsMeta where isinstance and issubclass checks were behaving incorrectly. The removal of the custom __instancecheck__ and __subclasscheck__ methods is the right approach, and the comprehensive new tests in test_object.py provide excellent coverage for this fix.
The PR also introduces a significant number of new features and refactorings to the FFI and reflection systems, including support for function object setters, a unified object creation mechanism, and a powerful new type checking and conversion system for TypeSchema. These additions appear to be of high quality and well-engineered.
I have one suggestion regarding the consistency of how Python's list type is handled in the new type conversion logic.
I am having trouble creating individual review comments. Click here to see my feedback.
python/tvm_ffi/cython/type_check.pxi (873)
For clarity and consistency, should origin == "list" also use _dispatch_list? The from_annotation(list) method in type_info.pxi maps Python's list to TypeSchema("List"), which correctly uses _dispatch_list here. Since Python's list is mutable, mapping it to tvm_ffi.List (which is also mutable) is semantically more intuitive than mapping to the immutable tvm_ffi.Array.
The comment on line 871 states that "list" is mapped to Array, which is what the current code does. However, this seems inconsistent with how from_annotation(list) is handled. If this change is made, the comment on line 871 should also be updated.
conv.dispatch = _dispatch_list if origin in ("List", "list") else _dispatch_array
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug in the Python FFI where isinstance and issubclass checks were behaving incorrectly for CObject subclasses. The fix, which removes the custom __instancecheck__ and __subclasscheck__ methods from _ObjectSlotsMeta, is correct and allows Python to use its standard MRO-based checks. The addition of 22 new regression tests in TestIsinstanceIssubclass thoroughly verifies the corrected behavior.
Beyond the bug fix, this PR introduces significant enhancements to the FFI reflection system to better support Python-defined objects. This includes:
- A mechanism to create Python-defined objects from C++ using a
__ffi_new__attribute. - Support for Python functions as field setters, adding flexibility.
- A new
CAnyclass for owned FFI value containers. - A greatly improved
TypeSchemawith robust type checking and conversion capabilities, including the ability to create schemas from Python type annotations. - A new
tvm_ffi.dataclasses.Fielddescriptor for a more declarative way of defining FFI object fields.
The changes are extensive, touching C++, Python/Cython, and Rust bindings, but they are consistent and well-implemented. The overall code quality is high, and the new features are a valuable addition to TVM's Python FFI capabilities.
Note: Security Review did not run due to the size of the PR.
2 participants
Summary
__instancecheck__and__subclasscheck__from_ObjectSlotsMetainpython/tvm_ffi/cython/object.pxi. These methods unconditionally returnedTruefor anyCObjectinstance/subclass regardless of which class was being checked, causing incorrect behavior (e.g.,isinstance(Map(...), Array)returningTrue).Objectsubclasses, so standard Python MRO-basedisinstance/issubclasschecks work correctly.test_dataclass_init.pyto assert correct behavior (parent is not an instance of child).TestIsinstanceIssubclasscovering containers, inheritance, and cross-hierarchyisinstance/issubclasschecks.Test plan
🤖 Generated with Claude Code