fix(py_class): propagate parent total_size through empty intermediate classes#539
Merged
junrushao merged 1 commit intoapache:mainfrom Apr 12, 2026
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request ensures that total_size is correctly propagated through empty intermediate parent classes in the TVM FFI by updating the property logic in type_info.pxi and adding regression tests. The feedback suggests optimizing the Cython implementation by explicitly typing the parent_size variable as int64_t to reduce Python overhead.
fcd3084 to
efec27e
Compare
… classes Architecture: - `TypeInfo.total_size` in `type_info.pxi` previously used `sizeof(TVMFFIObject)` as the floor when computing total object size for Python-defined types. For intermediate classes with no own fields, this discarded inherited field space entirely, producing a total_size equal to just the object header. - The fix replaces `sizeof(TVMFFIObject)` with `self.parent_type_info.total_size` as the floor, so inherited field space propagates correctly through arbitrarily deep empty parents. - Adds a guard that `parent_type_info` is present (raises `ValueError` if not) and asserts `end >= sizeof(TVMFFIObject)` as an invariant. Public Interfaces: - No public API changes. `TypeInfo.total_size` remains a `@cached_property` with the same return-type contract. Behavioral Changes: - Before: GrandParent(x,y) -> Parent(no fields) -> Child(a,b) caused Child.a and Child.b offsets to overlap with GrandParent.x and GrandParent.y, producing silent data corruption on field read/write. - After: Child field offsets start at or after Parent.total_size, which correctly reflects all inherited fields. Tests: - Executed: uv run pytest -vvs tests/python/test_dataclass_py_class.py - Result: 6 new regression tests added in TestFieldInheritance covering offset correctness, total_size propagation, non-overlapping offsets, two-level empty intermediate parents, mutation aliasing, and the @py_class decorator path. Untested Edge Cases: - Types with C++-defined metadata are unaffected (early return path), but no new test exercises a mixed C++/Python chain with empty intermediates. - Thread safety of @cached_property under free-threaded Python is not tested here (orthogonal concern).
efec27e to
d7e05d7
Compare
yaoyaoding
approved these changes
Apr 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
had no own fields (e.g.,
GrandParent(x, y) -> Parent(pass) -> Child(a, b)),TypeInfo.total_sizefell back tosizeof(TVMFFIObject)(~24 bytes) for theempty parent, ignoring inherited fields. This caused child field offsets to
overlap with grandparent field offsets, leading to silent data corruption on
read/write.
sizeof(TVMFFIObject)floor withself.parent_type_info.total_sizeinTypeInfo.total_size, ensuring inheritedfield space propagates correctly through arbitrarily deep empty parents. Added a
guard that
parent_type_infomust be present and an assertion that the base sizeis at least
sizeof(TVMFFIObject).TestFieldInheritancecovering offsetcorrectness, total_size propagation, non-overlapping child offsets, two-level
empty intermediate parents, mutation aliasing, and the
@py_classdecorator path.Architecture
python/tvm_ffi/cython/type_info.pxi—TypeInfo.total_sizeproperty body (6 lines replaced with 6 lines; decorator unchanged).
end = sizeof(TVMFFIObject)and onlyiterated over
self.fields(the type's own fields). An empty intermediate classhas no own fields, so it returned a total_size equal to just the object header,
erasing all inherited field space.
end = self.parent_type_info.total_sizeinherits the parent'sfull layout (including transitively inherited fields), then
max(end, f.offset + f.size)extends it for any own fields.
Behavioral Changes
Child.aat offset 24 overlappingGrandParent.xat offset 24 whenParenthad no own fields. Readingobj.xreturnedobj.a's value.Child.astarts at or afterParent.total_size(which equalsGrandParent.total_size), so no field overlap occurs.Test Plan
uv run pytest -vvs tests/python/test_dataclass_py_class.py::TestFieldInheritance-- all tests pass (8 existing + 6 new)