From 355f5b42cae0834bc9da668fd8a14aa6158886e1 Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Tue, 29 Sep 2026 13:52:49 +0200 Subject: [PATCH 1/2] Fuzz keyed children whose wrapper container goes away The previous fuzz extension moves a keyed child out of a container that is replaced by one of another type. Review found the same problem when the container is removed instead, e.g. VBox(children=[VBox(children=[child]) if wrapped else child]): the removal of the stale wrapper removes the child with it, although the child is still in the tree. The random trees now have an Unwrapper component that puts a child with an explicit key inside 0, 1 or 2 wrapper containers, depending on its state, with a child that is sometimes the same object (use_memo) and sometimes new. The moved keyed children (of Mover and Unwrapper) now have an effect, and the effects that run at close() are compared too, so a kept child whose effect was cleaned up (or never is) shows up. On master, 2 of the 12 seeds fail (73 of 300): the default renderer raises AttributeError ('NoneType' object has no attribute 'comm_id'), and the fast renderer leaves elements behind. Fixed in the next commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/fuzz_test.py | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/reacton/fuzz_test.py b/reacton/fuzz_test.py index 8e6312b..fe3f336 100644 --- a/reacton/fuzz_test.py +++ b/reacton/fuzz_test.py @@ -42,6 +42,17 @@ def Leaf(id): return reacton.Fragment(children=[w.Button(description=f"f{id}.{i}") for i in range(value % 3)]) return w.Button(description=f"leaf {id} {value}") + @react.component + def EffectLeaf(id): + # a leaf with an effect: however its containers change, the effect must run once + # while it stays in the tree, and be cleaned up once when it leaves + def effect(): + log.append(("keyed effect", id)) + return lambda: log.append(("keyed cleanup", id)) + + react.use_effect(effect, []) + return Leaf(id) + @react.component def Wrapper(id): return Leaf(id * 7 + 1) @@ -75,7 +86,7 @@ def Mover(id): # that moves between two sibling containers whose types flip state, set_state = react.use_state(0) registry[id] = set_state - child = react.use_memo(lambda: Leaf(id * 3 + 2).key(f"moved {id}"), []) + child = react.use_memo(lambda: EffectLeaf(id * 3 + 2).key(f"moved {id}"), []) kind = h(id, state) % 8 First = w.HBox if kind & 2 else w.VBox Second = w.HBox if kind & 4 else w.VBox @@ -83,6 +94,21 @@ def Mover(id): return w.VBox(children=[First(children=[child]).key("first"), Second(children=[]).key("second")]) return w.VBox(children=[First(children=[]).key("first"), Second(children=[child]).key("second")]) + @react.component + def Unwrapper(id): + # a child with an explicit key inside 0, 1 or 2 wrapper containers: when a wrapper + # goes away (or comes back), the child stays in the tree at another level + state, set_state = react.use_state(0) + registry[id] = set_state + memo_child = react.use_memo(lambda: EffectLeaf(id * 3 + 2).key(f"unwrapped {id}"), []) + kind = h(id, state) % 12 + child = memo_child if kind & 1 else EffectLeaf(id * 3 + 2).key(f"unwrapped {id}") + Wrapper = w.HBox if kind & 2 else w.VBox + element = child + for i in range((kind >> 2) % 3): + element = Wrapper(children=[element]) + return w.VBox(children=[element]) + @react.component def Node(id, depth): state, set_state = react.use_state(0) @@ -114,6 +140,8 @@ def cleanup(): child = Catcher(child_id) elif r < 0.8: child = Mover(child_id) + elif r < 0.9: + child = Unwrapper(child_id) else: child = Leaf(child_id) if rnd.random() < 0.3: @@ -147,7 +175,7 @@ def _run_random_updates(fast: bool, seed: int, steps: int, batches: Optional[Lis record = batches is None batches = [] if batches is None else batches choices = random.Random(seed) - results = [] + results: List[Any] = [] with unittest.mock.patch.dict(os.environ, {"REACTON_FAST": "1" if fast else "0"}): widget, rc = react.render_fixed(Node(1, 0), handle_error=False) assert isinstance(rc, core._RenderContextFast) == fast @@ -162,7 +190,10 @@ def _run_random_updates(fast: bool, seed: int, steps: int, batches: Optional[Lis if id in registry: registry[id](value) results.append((_widget_signature(rc.last_root_widget), sorted(registry), list(log))) + log.clear() rc.close() + # every effect that ran is cleaned up once, in the same order + results.append(("close", list(log))) return results, batches @@ -179,4 +210,4 @@ def test_renderers_agree_on_random_updates(seed): finally: core.logger.setLevel(level) for step, (a, b) in enumerate(zip(expected, got)): - assert a == b, f"step {step}, batch {batches[step]}" + assert a == b, f"step {step}, batch {batches[step]}" if step < len(batches) else "close" From 77f53cbac5844af3ea382a711b20ac2e85b985e4 Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Tue, 29 Sep 2026 13:52:57 +0200 Subject: [PATCH 2/2] Keep keyed children when their wrapper container goes away When a container is no longer in the new tree, the stale-key sweep removes it with all its children, by key. An explicit key is the same in any container of a component, so a keyed child that is still in the tree at another level, e.g. in VBox(children=[VBox(children=[child]) if wrapped else child]), was removed too: the default renderer raised AttributeError ('NoneType' object has no attribute 'comm_id'), the fast renderer showed a closed widget or left elements behind. This is the same problem that the previous fix solved for a container replaced by one of another type. The stale-key sweeps (both renderers, and the fast renderer's root sweep) now remove through the same path, renamed from _remove_replaced to _remove_outgoing: children with an explicit key that the new tree still uses stay, and reconciliation updates them where the new tree has them. The fast renderer cleans up the effects of stale subtrees early (before new effects run); that pass now also leaves the kept children alone, so their effects keep running. Shared elements keep the old behavior, and close() still removes everything. The fuzz test: on master 73 of 300 seeds fail; with this fix 1000 of 1000 pass. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 59 ++++++++++++++++++++++++++------------------ reacton/core_test.py | 59 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 24 deletions(-) diff --git a/reacton/core.py b/reacton/core.py index 76adc3b..d4952ce 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1306,29 +1306,30 @@ def __call__(self): class _RenderContext: context: Optional[ComponentContext] = None - # the element that an element of another type replaces, and its context, while - # reconciliation removes it (see _remove_replaced) - _replaced: Optional[Tuple[ComponentContext, Element]] = None + # the element that reconciliation removes because the new tree replaced it by an element + # of another type, or no longer has it, and its context (see _remove_outgoing) + _outgoing: Optional[Tuple[ComponentContext, Element]] = None - def _remove_replaced(self, el_prev: Element, key: str, parent_key: str): - """Remove the element at key, which an element of another type replaces.""" + def _remove_outgoing(self, el: Element, key: str, parent_key: str): + """Remove the element at key, which the new tree replaced or no longer has.""" assert self.context is not None - replaced = self._replaced - self._replaced = (self.context, el_prev) + outgoing = self._outgoing + self._outgoing = (self.context, el) try: - self._remove_element(el_prev, key, parent_key=parent_key) + self._remove_element(el, key, parent_key=parent_key) finally: - self._replaced = replaced + self._outgoing = outgoing def _keep_keyed_child(self, context: ComponentContext, el: Element, key: str) -> bool: # An explicit key is the same in any container of a component, so a child with an - # explicit key that the new tree still uses (it moved out of the replaced element, or - # stays under the new one) is not removed with the replaced element: reconciliation - # updates it where the new tree has it. Shared elements keep the old behavior: their - # bookkeeping (_shared_elements, _shared_widgets) needs the removal. - replaced = self._replaced + # explicit key that the new tree still uses (it moved out of the outgoing element, for + # instance a wrapper that went away, or stays under the element that replaces it) is + # not removed with the outgoing element: reconciliation updates it where the new tree + # has it. Shared elements keep the old behavior: their bookkeeping (_shared_elements, + # _shared_widgets) needs the removal. + outgoing = self._outgoing return ( - replaced is not None and context is replaced[0] and el is not replaced[1] and el._key is not None and not el.is_shared and key in context.used_keys + outgoing is not None and context is outgoing[0] and el is not outgoing[1] and el._key is not None and not el.is_shared and key in context.used_keys ) def __init__(self, element: Element, container: widgets.Widget = None, children_trait="children", handle_error: bool = True, initial_state=None): @@ -2118,7 +2119,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): try: if isinstance(el.component, ComponentFunction): if el_prev and isinstance(el_prev.component, ComponentWidget): - self._remove_replaced(el_prev, key, parent_key=parent_key) + self._remove_outgoing(el_prev, key, parent_key=parent_key) new_parent_key = join_key(parent_key, key) try: # TODO: test suite passes when this block if commented out @@ -2164,7 +2165,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): if removed: for key_remove in removed: el_remove = elements[key_remove] - self._remove_element(el_remove, key_remove, parent_key) + self._remove_outgoing(el_remove, key_remove, parent_key) for effect_index, effect in enumerate(child_context.effects): if effect.next: # if we have a next, it means that effect itself is executed @@ -2295,7 +2296,7 @@ def reconsolidate_children(): else: assert el_prev is not None, "widget_previous is not None, but el_prev is" logger.debug("Replacing widget: %r → %r %r", el_prev, el, key) - self._remove_replaced(el_prev, key, parent_key=parent_key) + self._remove_outgoing(el_prev, key, parent_key=parent_key) kwargs = reconsolidate_children() widget = None if not context.exceptions_children: @@ -2372,7 +2373,7 @@ def reconsolidate_children(): if extra: for key in list(extra): if key in self.context.elements: - self._remove_element(self.context.elements[key], key, parent_key=parent_key) + self._remove_outgoing(self.context.elements[key], key, parent_key=parent_key) # keeping this for debugging # logger.debug("Current:") @@ -2796,7 +2797,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): if el_prev and isinstance(el_prev.component, ComponentWidget): # a widget element was replaced by a component element at this key - self._remove_replaced(el_prev, key, parent_key=parent_key) + self._remove_outgoing(el_prev, key, parent_key=parent_key) new_parent_key = join_key(parent_key, key) try: if el.is_shared and (el.args or el.kwargs): @@ -2832,7 +2833,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): logger.info("elements to be removed: %r", stale_keys) for stale_key in stale_keys: if stale_key in child_context.elements: - self._remove_element(child_context.elements[stale_key], stale_key, new_parent_key) + self._remove_outgoing(child_context.elements[stale_key], stale_key, new_parent_key) self._process_effects(child_context, context) @@ -2904,7 +2905,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): else: assert el_prev is not None, "widget_previous is not None, but el_prev is" # a different widget type at the same key: replace - self._remove_replaced(el_prev, key, parent_key=parent_key) + self._remove_outgoing(el_prev, key, parent_key=parent_key) kwargs = self._visit_children_values(el.kwargs, key, parent_key, self._reconsolidate) widget = None if not context.exceptions_children: @@ -3013,7 +3014,15 @@ def _cleanup_stale_effects_for_context(self, context: "ComponentContext", parent if stale_key not in context.elements or stale_key in context.fast_stale_effects_cleaned: continue self.context = context - self._cleanup_stale_effects(context.elements[stale_key], stale_key, parent_key) + el = context.elements[stale_key] + # keyed children that the new tree still uses stay, with their effects + # (see _keep_keyed_child) + outgoing = self._outgoing + self._outgoing = (context, el) + try: + self._cleanup_stale_effects(el, stale_key, parent_key) + finally: + self._outgoing = outgoing finally: self.context = context_prev @@ -3026,6 +3035,8 @@ def _cleanup_stale_effects(self, el: Element, default_key: str, parent_key: str) assert context is not None if key in context.fast_stale_effects_cleaned: return + if self._keep_keyed_child(context, el, key): + return if el.is_shared and (el in self._shared_elements_next or el not in self._shared_elements): return context.fast_stale_effects_cleaned.add(key) @@ -3188,7 +3199,7 @@ def _remove_stale_root_elements(self, parent_key): stale_keys = sorted(set(self.context_root.elements) - self.context_root.used_keys) for stale_key in stale_keys: if stale_key in self.context_root.elements: - self._remove_element(self.context_root.elements[stale_key], stale_key, parent_key) + self._remove_outgoing(self.context_root.elements[stale_key], stale_key, parent_key) def _render_context_class(): diff --git a/reacton/core_test.py b/reacton/core_test.py index fa2955c..48e3c30 100644 --- a/reacton/core_test.py +++ b/reacton/core_test.py @@ -4245,3 +4245,62 @@ def Test(): assert rc.find(widgets.Button).widget.comm is not None assert len(rc._shared_widgets) == 1 rc.close() + + +@pytest.mark.parametrize("same_element", [True, False]) +def test_keyed_child_out_of_removed_wrapper(same_element): + # the wrapper around a child with an explicit key goes away (and comes back): its + # removal must not remove (or close) the child, which is still in the tree + set_wrapped = lambda x: None # noqa + + @react.component + def Child(): + return w.Button(description="child") + + child_once = Child().key("x") + + @react.component + def Test(): + nonlocal set_wrapped + wrapped, set_wrapped = react.use_state(True) + child = child_once if same_element else Child().key("x") + return w.VBox(children=[w.VBox(children=[child]) if wrapped else child]) + + root, rc = react.render_fixed(Test(), handle_error=False) + for wrapped in [False, True, False]: + set_wrapped(wrapped) + button = rc.find(widgets.Button).widget + assert button.comm is not None + assert (root.children[0] is button) != wrapped + rc.close() + + +def test_keyed_child_out_of_removed_wrapper_keeps_its_effects(): + # the kept child stays mounted: its effect must not be cleaned up when the wrapper goes + # away (the fast renderer cleans up stale subtrees early), only when the child leaves + log = [] + set_wrapped = lambda x: None # noqa + + @react.component + def Child(): + def effect(): + log.append("run") + return lambda: log.append("cleanup") + + react.use_effect(effect, []) + return w.Button(description="child") + + @react.component + def Test(): + nonlocal set_wrapped + wrapped, set_wrapped = react.use_state(True) + child = Child().key("x") + return w.VBox(children=[w.Label(value="sibling"), w.VBox(children=[child]) if wrapped else child]) + + box, rc = react.render(Test(), handle_error=False) + assert log == ["run"] + set_wrapped(False) + set_wrapped(True) + assert log == ["run"] + rc.close() + assert log == ["run", "cleanup"]