junrushao commented on code in PR #449:
URL: https://github.com/apache/tvm-ffi/pull/449#discussion_r2808218988
##########
python/tvm_ffi/access_path.py:
##########
@@ -105,7 +105,6 @@ def _path_equal(self, _1: AccessPath, /) -> bool: ...
def __init__(self) -> None:
"""Disallow direct construction; use `AccessPath.root()` instead."""
- super().__init__()
raise ValueError(
Review 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) → `RuntimeError`
- `AccessPath.__init__` → `ValueError`
`TypeError` is the most Pythonic choice for "this class cannot be
instantiated this way" (matches abstract classes and `__new__` guards).
Consider unifying:
```python
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)_
##########
python/tvm_ffi/cython/object.pxi:
##########
@@ -116,6 +116,12 @@ cdef class Object:
# case of error before chandle is set
self.chandle = NULL
+ def __init__(self):
Review 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_
##########
tests/python/test_object.py:
##########
@@ -136,6 +136,33 @@ def test_opaque_type_error() -> None:
)
+def test_object_direct_init_disabled() -> None:
+ """Object() and unregistered subclasses must not silently create
NULL-handle objects."""
+ # Base Object class
+ with pytest.raises(TypeError, match="Cannot directly create Object
instance"):
+ tvm_ffi.Object()
+
+ # Unregistered subclass without __init__
+ class UnregisteredObj(tvm_ffi.Object):
+ pass
+
+ with pytest.raises(TypeError, match="Cannot directly create
UnregisteredObj instance"):
+ UnregisteredObj()
+
+ # Registered class without __c_ffi_init__ should still raise
+ with pytest.raises(RuntimeError, match="__init__ method of this class is
not implemented"):
Review 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:
```python
# 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)_
##########
tests/python/test_object.py:
##########
@@ -136,6 +136,33 @@ def test_opaque_type_error() -> None:
)
+def test_object_direct_init_disabled() -> None:
Review 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:
```python
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_
##########
tests/python/test_object.py:
##########
@@ -136,6 +136,33 @@ def test_opaque_type_error() -> None:
)
+def test_object_direct_init_disabled() -> None:
+ """Object() and unregistered subclasses must not silently create
NULL-handle objects."""
+ # Base Object class
+ with pytest.raises(TypeError, match="Cannot directly create Object
instance"):
+ tvm_ffi.Object()
+
+ # Unregistered subclass without __init__
+ class UnregisteredObj(tvm_ffi.Object):
+ pass
+
+ with pytest.raises(TypeError, match="Cannot directly create
UnregisteredObj instance"):
+ UnregisteredObj()
+
+ # Registered class without __c_ffi_init__ should still raise
+ with pytest.raises(RuntimeError, match="__init__ method of this class is
not implemented"):
+ tvm_ffi.testing.TestObjectBase()
+
+ # 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
Review 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:
```python
assert pair.a == 3
assert pair.b == 4
```
_Source: Claude_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
