gemini-code-assist[bot] commented on code in PR #416:
URL: https://github.com/apache/tvm-ffi/pull/416#discussion_r2702461539
##########
python/tvm_ffi/dataclasses/_utils.py:
##########
@@ -122,6 +123,40 @@ def _get_all_fields(type_info: TypeInfo) ->
list[TypeField]:
return fields
+def _classify_fields_for_copy(
+ type_info: TypeInfo,
+) -> tuple[list[str], list[str], list[str]]:
+ """Classify fields for copy/replace operations.
+
+ Returns:
+ Tuple of (ffi_arg_order, init_fields, non_init_fields):
+ - ffi_arg_order: Fields passed to FFI constructor
+ - init_fields: Fields with init=True (replaceable)
+ - non_init_fields: Fields with init=False
+
+ """
+ fields = _get_all_fields(type_info)
+ ffi_arg_order: list[str] = []
+ init_fields: list[str] = []
+ non_init_fields: list[str] = []
+
+ for field in fields:
+ assert field.name is not None
+ assert field.dataclass_field is not None
+ dataclass_field = field.dataclass_field
+
+ if dataclass_field.init:
+ init_fields.append(field.name)
+ ffi_arg_order.append(field.name)
+ elif dataclass_field.default_factory is not MISSING:
+ ffi_arg_order.append(field.name)
+
+ if not dataclass_field.init:
+ non_init_fields.append(field.name)
Review Comment:
The current logic for classifying fields in `_classify_fields_for_copy` has
a bug. Fields that have `init=False` but are provided with a `default_factory`
are added to both `ffi_arg_order` and `non_init_fields`. This causes them to be
set once by the `__c_ffi_init__` constructor and then again via attribute
assignment in the generated `__copy__`, `__deepcopy__`, and `__replace__`
methods.
This double assignment is inefficient and can lead to incorrect behavior if
the field's setter has side effects.
The logic should be adjusted to ensure that `non_init_fields` only contains
fields that are not passed to the FFI constructor (i.e., fields with
`init=False` and no `default_factory`).
```suggestion
if dataclass_field.init:
init_fields.append(field.name)
ffi_arg_order.append(field.name)
else: # init=False
if dataclass_field.default_factory is not MISSING:
ffi_arg_order.append(field.name)
else:
non_init_fields.append(field.name)
```
--
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]
