[Bugfix] `.parent` allocates a fresh wrapper on every read, so `child.parent is parent` is always False #369

New issue

Closed

opened 2026-07-13 03:05:56 +00:00 by john · 0 comments

john commented

2026-07-13 03:05:56 +00:00

Owner

Found while writing #364's tests. Affects every drawable type, not just grids.

Defect

UIDrawable::get_parent (src/UIDrawable.cpp:1280) builds a brand-new Python wrapper on each access -- every arm of its switch is a bare type->tp_alloc(...) with no PythonObjectCache lookup:

case PyObjectsEnum::UIFRAME:
{
    type = &mcrfpydef::PyUIFrameType;
    auto pyObj = (PyUIFrameObject*)type->tp_alloc(type, 0);   // <-- fresh object, every time
    if (pyObj) {
        pyObj->data = std::static_pointer_cast<UIFrame>(parent_ptr);
        pyObj->weakreflist = NULL;
    }
    obj = (PyObject*)pyObj;
    break;
}

So object identity never holds:

parent = mcrfpy.Frame(pos=(0,0), size=(100,100))
kid    = mcrfpy.Frame(pos=(1,1), size=(10,10))
parent.children.append(kid)

kid.parent is parent        # False
kid.parent is kid.parent    # False  <-- two reads of the same attribute

The wrappers do share the underlying shared_ptr, so mutation through either one works. It is identity, not behavior, that is broken -- but is is exactly what a user writes to ask "am I inside this frame?", and it silently answers no.

Consequences

child.parent is some_frame is unusable; users must compare .name or another observable instead.
A Frame/Grid subclass reached through .parent comes back as the base type, losing the Python subclass identity that #266 established for Entity (UIEntity::pyobject). Any state a user put on their subclass instance is invisible through .parent.
id(x.parent) is unstable across reads, so .parent cannot be used as a dict key or in a set.

Fix

Route every arm through PythonObjectCache (keyed on serial_number) the way the entity path does, returning the existing wrapper when one is alive and only allocating on a miss. RET_PY_INSTANCE already switches on derived_type() to build the right wrapper -- the cache lookup belongs alongside it so .parent, collection indexing, and find() all return the same object for the same C++ drawable.

Worth auditing the sibling paths in the same pass: UICollection indexing/iteration and mcrfpy.find() may have the same gap, in which case scene.children[0] is scene.children[0] is also False.

Test

tests/regression/issue_364_grid_children_on_view_test.py currently asserts bubble.parent.name == "the_view" with a comment explaining why is cannot be used. When this is fixed, tighten that to bubble.parent is grid and add a subclass round-trip: class MyGrid(mcrfpy.Grid), append a child, assert child.parent is the MyGrid instance and retains its Python attributes.

Found while writing #364's tests. Affects every drawable type, not just grids. ## Defect `UIDrawable::get_parent` (`src/UIDrawable.cpp:1280`) builds a **brand-new Python wrapper** on each access -- every arm of its switch is a bare `type->tp_alloc(...)` with no `PythonObjectCache` lookup: ```cpp case PyObjectsEnum::UIFRAME: { type = &mcrfpydef::PyUIFrameType; auto pyObj = (PyUIFrameObject*)type->tp_alloc(type, 0); // <-- fresh object, every time if (pyObj) { pyObj->data = std::static_pointer_cast<UIFrame>(parent_ptr); pyObj->weakreflist = NULL; } obj = (PyObject*)pyObj; break; } ``` So object identity never holds: ```python parent = mcrfpy.Frame(pos=(0,0), size=(100,100)) kid = mcrfpy.Frame(pos=(1,1), size=(10,10)) parent.children.append(kid) kid.parent is parent # False kid.parent is kid.parent # False <-- two reads of the same attribute ``` The wrappers do share the underlying `shared_ptr`, so mutation through either one works. It is identity, not behavior, that is broken -- but `is` is exactly what a user writes to ask "am I inside this frame?", and it silently answers no. ## Consequences - `child.parent is some_frame` is unusable; users must compare `.name` or another observable instead. - A `Frame`/`Grid` **subclass** reached through `.parent` comes back as the base type, losing the Python subclass identity that #266 established for `Entity` (`UIEntity::pyobject`). Any state a user put on their subclass instance is invisible through `.parent`. - `id(x.parent)` is unstable across reads, so `.parent` cannot be used as a dict key or in a `set`. ## Fix Route every arm through `PythonObjectCache` (keyed on `serial_number`) the way the entity path does, returning the existing wrapper when one is alive and only allocating on a miss. `RET_PY_INSTANCE` already switches on `derived_type()` to build the right wrapper -- the cache lookup belongs alongside it so `.parent`, collection indexing, and `find()` all return the same object for the same C++ drawable. Worth auditing the sibling paths in the same pass: `UICollection` indexing/iteration and `mcrfpy.find()` may have the same gap, in which case `scene.children[0] is scene.children[0]` is also False. ## Test `tests/regression/issue_364_grid_children_on_view_test.py` currently asserts `bubble.parent.name == "the_view"` with a comment explaining why `is` cannot be used. When this is fixed, tighten that to `bubble.parent is grid` and add a subclass round-trip: `class MyGrid(mcrfpy.Grid)`, append a child, assert `child.parent` is the `MyGrid` instance and retains its Python attributes.