fix: prevent silent NULL-handle creation in Object.__init__#449
fix: prevent silent NULL-handle creation in Object.__init__#449junrushao wants to merge 1 commit intoapache:mainfrom
Conversation
Previously, calling `Object()` or constructing an unregistered subclass without `__init__` would silently succeed and produce an object with a NULL handle. Add `Object.__init__` that raises TypeError to catch this misuse early. Remove the now-redundant `super().__init__()` call in AccessPath.__init__. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @junrushao, 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 enhances the robustness of object instantiation by explicitly preventing the creation of 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
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
|
![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
The pull request effectively addresses the issue of silent NULL-handle creation by introducing a TypeError in the base Object.__init__. This ensures that direct instantiation of Object or unregistered subclasses is caught early with a descriptive error message. The removal of the redundant super().__init__() call in AccessPath is a correct cleanup to avoid conflicting exceptions, and the new test cases provide comprehensive coverage for the various instantiation scenarios (base class, unregistered subclasses, and registered classes with or without constructors). The implementation is clean and follows the existing patterns in the codebase.
junrushao
left a comment
junrushao
left a comment
There was a problem hiding this comment.
Synthesized Code Review Report
Scope: PR #449 (all commits since diverging from main) | Files changed: 3 | Reviewers: Claude Code, Codex
Summary: 2 high, 3 medium, 5 low/nit findings. Core mechanism is sound and well-motivated — __cinit__ still initializes chandle = NULL, __init__ now raises early, and all legitimate construction paths go through __new__ + handle setup.
General Findings (not mapped to specific diff lines)
[high | API Design | Consensus] Breaking change: Any code calling Object() or super().__init__() on unregistered subclasses will now fail with TypeError. This is intentional but should be documented in release notes.
[low | Maintainability | Claude] registry.py:387 — __init__invalid message says "The __init__ method of this class is not implemented." without naming the class. The new Object.__init__ demonstrates how to include type(self).__name__; consider aligning for consistency in a follow-up.
[low | Test Coverage | Codex] Missing test for a registered subclass that naively overrides __init__ — would help document the expected behavior.
[nit | Test | Claude] # ty: ignore[too-many-positional-arguments] appears in multiple test locations; may indicate a stub issue for TestIntPair.__init__.
Questions for Author
- Pickle path:
__reduce__returns(_new_object, (cls,), self.__getstate__())which usescls.__new__(cls), bypassing__init__. Have you verified pickle round-trips still work? - Cython MRO: If a Python subclass doesn't define
__init__, the Cython baseObject.__init__fires. Registry replaces__init__viasetattron the Python type's__dict__. Confirmed for all registered types?
Generated by multi-model code review (Claude Code + Codex)
| def __init__(self) -> None: | ||
| """Disallow direct construction; use `AccessPath.root()` instead.""" | ||
| super().__init__() | ||
| raise ValueError( |
There was a problem hiding this comment.
[high | API Consistency | Consensus] AccessPath.__init__ raises ValueError while the base Object.__init__ now raises TypeError. Three different exception types are used for "cannot construct":
Object.__init__→TypeError__init__invalid(registry) →RuntimeErrorAccessPath.__init__→ValueError
TypeError is the most Pythonic choice for "this class cannot be instantiated this way" (matches abstract classes and __new__ guards). Consider unifying:
def __init__(self) -> None:
"""Disallow direct construction; use `AccessPath.root()` instead."""
raise TypeError(
"AccessPath cannot be initialized directly. "
"Use AccessPath.root() to create a path to the root object"
)Source: Consensus (Claude + Codex)
| # case of error before chandle is set | ||
| self.chandle = NULL | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
[low | Maintainability | Codex] The error message hardcodes "FFI constructors" and "@register_object" as the solutions. If new mechanisms for object creation are added, this message won't mention them. Minor suggestion — consider making it slightly more generic, e.g. adding "factory methods" as an option.
Source: Codex
| UnregisteredObj() | ||
|
|
||
| # Registered class without __c_ffi_init__ should still raise | ||
| with pytest.raises(RuntimeError, match="__init__ method of this class is not implemented"): |
There was a problem hiding this comment.
[medium | Correctness | Claude] The match string "__init__ method of this class is not implemented" is a fragile substring — the actual message in registry.py:387 is "The __init__ method of this class is not implemented." (starts with "The"). While re.search makes this work, a reader may think the full message starts with "__init__".
[medium | Test Clarity | Codex] It's also unclear from the test whether the RuntimeError comes from the missing __c_ffi_init__ path or from the new __init__ guard. A brief comment clarifying the error path would help.
Suggested fix:
# Registered class without __c_ffi_init__: the registry replaces __init__
# with __init__invalid, which raises RuntimeError (not the base TypeError)
with pytest.raises(RuntimeError, match="The __init__ method of this class is not implemented"):
tvm_ffi.testing.TestObjectBase()Source: Claude (match string) + Codex (clarity)
| ) | ||
|
|
||
|
|
||
| def test_object_direct_init_disabled() -> None: |
There was a problem hiding this comment.
[medium | Test Coverage | Claude] Missing test for AccessPath.__init__ after removing super().__init__(). The removal changes exception ordering — previously the base Object.__init__ (no-op) ran first, then ValueError fired. Now only ValueError fires directly. If someone accidentally removes the raise ValueError, the base TypeError would kick in with different semantics.
Suggested addition:
def test_access_path_direct_init_raises():
"""AccessPath() must raise ValueError directing users to root()."""
from tvm_ffi.access_path import AccessPath
with pytest.raises(ValueError, match="AccessPath can.t be initialized directly"):
AccessPath()Source: Claude
|
|
||
| # Registered class with __c_ffi_init__ should work fine | ||
| pair = tvm_ffi.testing.TestIntPair(3, 4) # ty: ignore[too-many-positional-arguments] | ||
| assert pair.a == 3 and pair.b == 4 |
There was a problem hiding this comment.
[nit | Readability | Claude] If pair.a == 3 passes but pair.b == 4 fails, pytest will only report the combined expression failed without showing which half. Consider splitting:
assert pair.a == 3
assert pair.b == 4Source: Claude
| # case of error before chandle is set | ||
| self.chandle = NULL | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
there might be a cases that depends on class construction and handle move, so we need to cross check downstream for this change
|
blocked by cuteDSL usage of this private API |
2 participants
blocked by cuteDSL usage of this private API
Previously, calling
Object()or constructing an unregistered subclass without__init__would silently succeed and produce an object with a NULL handle. AddObject.__init__that raises TypeError to catch this misuse early. Remove the now-redundantsuper().__init__()call in AccessPath.init.