Repository: Textualize/textual
Pull request: mutate via data bind
Fixes https://github.com/Textualize/textual/issues/4825

- Ensures that watchers are called on data bound children, when calling `mutate_reactive`.
- Also enforces a new instance when data binding. Previously parent and child would end up sharing an instance, which would sure be a common source of errors.

Problem statement:
mutate_reactive doesn't work with data binding and recompose=False
Note that in the following example recompose is set to False on both reactive members.
Adding an item to the list in on_mount is working as expected, and `watch_msg` is called. But changing an element in `on_button_pressed`, doesn't call `watch_msg`. Apparently only changing the size of the list, is considered a change.

```python
from textual.app import App, ComposeResult
from textual.widgets import Static, Button
from textual.reactive import reactive
from textual.events import Mount

class TestWidget(Static):
    msg: reactive[list[str]] = reactive(list, recompose=False)
    count = reactive(0)

    def compute_count(self):
        return len(self.msg)

    def watch_msg(self, old, new):
        if new and len(new) > 0:
            self.mount(Static(self.msg[0]["text"]))

class TestApp(App):
    msg: reactive[list[str]] = reactive(list, recompose=False)

    def on_mount(self, event: Mount) -> None:
        self.msg.append( { "text": "Hello" } )
        self.mutate_reactive(TestApp.msg)

    def compose(self) -> ComposeResult:
        yield TestWidget().data_bind(TestApp.msg)
        yield Button("Click me", id="button")

    def on_button_pressed(self, event: Button.Pressed) -> None:
        self.msg[0] = {"text": "World!"}
        self.mutate_reactive(TestApp.msg)

if __name__ == "__main__":
    app = TestApp()
    app.run()```

Hints from issue discussion:

Commit message: better test

Patch to review:
diff --git a/src/textual/dom.py b/src/textual/dom.py
index 55ce52afea..d3b4b83e9e 100644
--- a/src/textual/dom.py
+++ b/src/textual/dom.py
@@ -8,6 +8,7 @@
 
 import re
 import threading
+from copy import copy
 from functools import lru_cache, partial
 from inspect import getfile
 from typing import (
@@ -41,7 +42,7 @@
 from .css.styles import RenderStyles, Styles
 from .css.tokenize import IDENTIFIER
 from .message_pump import MessagePump
-from .reactive import Reactive, ReactiveError, _watch
+from .reactive import Reactive, ReactiveError, _Mutated, _watch
 from .timer import Timer
 from .walk import walk_breadth_first, walk_depth_first
 from .worker_manager import WorkerManager
@@ -334,7 +335,10 @@ def setter(value: object) -> None:
                     """Set bound data."""
                     _rich_traceback_omit = True
                     Reactive._initialize_object(self)
-                    setattr(self, variable_name, value)
+                    # Copy the bound value
+                    # Sharing the same instance with mutable objects can lead to confusing behavior
+                    # Wrap the value in `_Mutated` so the setter knows to invoke watchers etc
+                    setattr(self, variable_name, _Mutated(copy(value)))
 
                 return setter
 
diff --git a/src/textual/reactive.py b/src/textual/reactive.py
index 17b109d77e..64b6e007ce 100644
--- a/src/textual/reactive.py
+++ b/src/textual/reactive.py
@@ -42,6 +42,13 @@
 ReactableType = TypeVar("ReactableType", bound="DOMNode")
 
 
+class _Mutated:
+    """A wrapper to indicate a value was mutated."""
+
+    def __init__(self, value: Any) -> None:
+        self.value = value
+
+
 class ReactiveError(Exception):
     """Base class for reactive errors."""
 
@@ -273,6 +280,10 @@ def _set(self, obj: Reactable, value: ReactiveType, always: bool = False) -> Non
                 f"Node is missing data; Check you are calling super().__init__(...) in the {obj.__class__.__name__}() constructor, before setting reactives."
             )
 
+        if isinstance(value, _Mutated):
+            value = value.value
+            always = True
+
         self._initialize_reactive(obj, self.name)
 
         if hasattr(obj, self.compute_name):
diff --git a/tests/test_reactive.py b/tests/test_reactive.py
index ad2da5724d..49ab1fd0f6 100644
--- a/tests/test_reactive.py
+++ b/tests/test_reactive.py
@@ -783,3 +783,46 @@ def compose(self) -> ComposeResult:
         widget.mutate_reactive(TestWidget.names)
         # Watcher should be invoked
         assert watched_names == [[], ["Paul"], ["Jessica"]]
+
+
+async def test_mutate_reactive_data_bind() -> None:
+    """https://github.com/Textualize/textual/issues/4825"""
+
+    # Record mutations to TestWidget.messages
+    widget_messages: list[list[str]] = []
+
+    class TestWidget(Widget):
+        messages: reactive[list[str]] = reactive(list, init=False)
+
+        def watch_messages(self, names: list[str]) -> None:
+            widget_messages.append(names.copy())
+
+    class TestApp(App):
+        messages: reactive[list[str]] = reactive(list, init=False)
+
+        def compose(self) -> ComposeResult:
+            yield TestWidget().data_bind(TestApp.messages)
+
+    app = TestApp()
+    async with app.run_test():
+        test_widget = app.query_one(TestWidget)
+        assert widget_messages == [[]]
+        assert test_widget.messages == []
+
+        # Previously setting a mutable object would lead to shared references
+        assert app.messages is not test_widget.messages
+
+        # Mutate app
+        app.messages.append("foo")
+        # Mutations aren't detected
+        assert widget_messages == [[]]
+        assert app.messages == ["foo"]
+        assert test_widget.messages == []
+        # Explicitly mutate app reactive
+        app.mutate_reactive(TestApp.messages)
+        # Mutating app, will also invoke watchers on any data binds
+        assert widget_messages == [[], ["foo"]]
+        assert app.messages == ["foo"]
+        assert test_widget.messages == ["foo"]
+
+        assert app.messages is not test_widget.messages


Code review comments:
diff_hunk: @@ -334,7 +335,10 @@ def setter(value: object) -> None:
                     """Set bound data."""
                     _rich_traceback_omit = True
                     Reactive._initialize_object(self)
-                    setattr(self, variable_name, value)
+                    # Copy the bound value
path: src/textual/dom.py
text: @user1:
I feel like this should be documented since copying can be expensive or potentially crash the program.

I'm guessing shallow copying means it also wouldn't fix the confusion for e.g. a list of mutable objects. For example, if one of the objects in the list is mutated and I call `mutate_reactive`, the prior confusing scenario still arises.

@author:
I'm reconsidering the copy.

It's not clear what the dev would expect when modifying a child attribute that has been bound. I'm not sure if they would expect the changes to be reflected on the parent. But I don't know if they would expect the inverse either.

Going to remove the copy, until I have a better idea what it would fix (if anything)

@user1:
I agree. I actually think dealing with the mutation via offering APIs like this is likely to be filled with more footguns, "things we need to document", and generally worse than just saying "only use immutable values for reactives, and watchers only fire when you assign".