gemini-code-assist[bot] commented on code in PR #384:
URL: https://github.com/apache/tvm-ffi/pull/384#discussion_r2663593511
##########
python/tvm_ffi/dataclasses/c_class.py:
##########
@@ -140,20 +177,36 @@ def decorator(super_type_cls: Type[_InputClsType]) ->
Type[_InputClsType]: # no
return decorator
-def _inspect_c_class_fields(type_cls: type, type_info: TypeInfo) ->
list[TypeField]:
+def _inspect_c_class_fields(
+ type_cls: type, type_info: TypeInfo
+) -> tuple[list[TypeField], int | None]:
if sys.version_info >= (3, 9):
type_hints_resolved = get_type_hints(type_cls, include_extras=True)
else:
type_hints_resolved = get_type_hints(type_cls)
- type_hints_py = {
- name: type_hints_resolved[name]
- for name in getattr(type_cls, "__annotations__", {}).keys()
- if get_origin(type_hints_resolved[name])
- not in [ # ignore non-field annotations
- ClassVar,
- InitVar,
- ]
- }
+
+ # Filter out ClassVar, InitVar, and detect KW_ONLY
+ annotations = getattr(type_cls, "__annotations__", {})
+ type_hints_py: dict[str, type] = {}
+ kw_only_start_idx: int | None = None
+ field_count = 0
+
+ for name in annotations.keys():
+ resolved_type = type_hints_resolved.get(name)
+ if resolved_type is None:
+ continue
+ origin = get_origin(resolved_type)
+ # Skip ClassVar and InitVar
+ if origin in [ClassVar, InitVar]:
+ continue
+ # Detect KW_ONLY sentinel
+ if resolved_type is _KW_ONLY_TYPE or isinstance(resolved_type,
_KW_ONLY_TYPE):
Review Comment:
The check for the `KW_ONLY` sentinel can be simplified. The class
`_KW_ONLY_TYPE` is an internal detail, and users are expected to use the
`KW_ONLY` instance, as shown in the documentation. Supporting annotations with
the internal class `_KW_ONLY_TYPE` directly is likely unnecessary and could
lead to confusion. A simpler check, `isinstance(resolved_type, _KW_ONLY_TYPE)`,
would correctly handle cases where the annotation is the `KW_ONLY` instance and
aligns better with how the standard library `dataclasses` handles its `KW_ONLY`
sentinel.
```suggestion
if isinstance(resolved_type, _KW_ONLY_TYPE):
```
##########
python/tvm_ffi/dataclasses/field.py:
##########
@@ -118,14 +150,28 @@ class PyBase:
obj = PyBase(v_i64=4)
obj.v_i32 # -> 16
+ Use ``kw_only=True`` to make a field keyword-only:
+
+ .. code-block:: python
+
+ @c_class("testing.TestCxxClassBase")
+ class PyBase:
+ v_i64: int
+ v_i32: int = field(kw_only=True)
+
+
+ obj = PyBase(4, v_i32=8) # v_i32 must be keyword
+
"""
if default is not MISSING and default_factory is not MISSING:
raise ValueError("Cannot specify both `default` and `default_factory`")
if not isinstance(init, bool):
raise TypeError("`init` must be a bool")
+ if kw_only is not MISSING and not isinstance(kw_only, bool):
+ raise TypeError("`kw_only` must be a bool or MISSING")
Review Comment:
The error message for the `kw_only` type check could be more precise for the
user. The `MISSING` value is an internal sentinel for a non-provided argument
and not a value a user can pass. The message could be simplified to state that
`kw_only` must be a boolean, which is the expected type from a user's
perspective.
```suggestion
raise TypeError("`kw_only` must be a bool")
```
--
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]
