feat: detect mutation to values of reactive vars (#595)

* feat: detect mutation to values of reactive vars

A common source of error is mutation of values in reactive vars.
The reactive var cannot notice a change in its value (e.g. a list)
if the list is mutated in place.

A user can be mislead that it is working correctly, when another
reactive variable triggers a rerender, teaching bad habits.

Detecting mutations in Python without using proxies can be done by
making a deepcopy of the object, and comparing the reference to the
deepcopy.

This comes at the cost of performance and memory usage, therefore we
should enabled this by default in development mode, so app builders
are aware of mistakes while developing.
In production mode we can disable the mutation checks and behave as
before.

* refactor: mutation detection is now a decorator class

* refactor: simplify initial value mutation detection

* chore: clean up mutation detection

* feat: improve mutation detection performance

By using `sys._getframe` when possible, we should make quite substantial performance gains over using inspect

* fix: None not handled properly in mutation detection

---------

Co-authored-by: Iisakki Rotko <iisakki.rotko@gmail.com>

Author: maartenbreddels

solara/_stores.py

@@ -0,0 +1,185 @@
+import copy
+import dataclasses
+import inspect
+from typing import Callable, ContextManager, Generic, Optional, Union, cast
+import warnings
+from .toestand import ValueBase, KernelStore, S, _find_outside_solara_frame
+import solara.util
+
+
+class _PublicValueNotSet:
+    pass
+
+
+@dataclasses.dataclass
+class StoreValue(Generic[S]):
+    private: S  # the internal private value, should never be mutated
+    public: Union[S, _PublicValueNotSet]  # this is the value that is exposed in .get(), it is a deep copy of private
+    get_traceback: Optional[inspect.Traceback]
+    set_value: Optional[S]  # the value that was set using .set(..), we deepcopy this to set private
+    set_traceback: Optional[inspect.Traceback]
+
+
+class MutateDetectorStore(ValueBase[S]):
+    def __init__(self, store: KernelStore[StoreValue[S]], equals=solara.util.equals_extra):
+        self._storage = store
+        self._enabled = True
+        super().__init__(equals=equals)
+
+    @property
+    def lock(self):
+        return self._storage.lock
+
+    def get(self) -> S:
+        self.check_mutations()
+        self._ensure_public_exists()
+        value = self._storage.get()
+        # value.public is of type Optional[S], so it's tempting to check for None here,
+        # but S could include None as a valid value, so best we can do is cast
+        public_value = cast(S, value.public)
+        return public_value
+
+    def peek(self) -> S:
+        """Return the value without automatically subscribing to listeners."""
+        self.check_mutations()
+        store_value = self._storage.peek()
+        self._ensure_public_exists()
+        public_value = cast(S, store_value.public)
+        return public_value
+
+    def set(self, value: S):
+        self.check_mutations()
+        self._ensure_public_exists()
+        private = copy.deepcopy(value)
+        self._check_equals(private, value)
+        frame = _find_outside_solara_frame()
+        if frame is not None:
+            frame_info = inspect.getframeinfo(frame)
+        else:
+            frame_info = None
+        store_value = StoreValue(private=private, public=_PublicValueNotSet(), get_traceback=None, set_value=value, set_traceback=frame_info)
+        self._storage.set(store_value)
+
+    def check_mutations(self):
+        self._storage._check_mutation()
+        if not self._enabled:
+            return
+        store_value = self._storage.peek()
+        if not isinstance(store_value.public, _PublicValueNotSet) and not self.equals(store_value.public, store_value.private):
+            tb = store_value.get_traceback
+            # TODO: make the error message as elaborate as below
+            msg = (
+                f"Reactive variable was read when it had the value of {store_value.private!r}, but was later mutated to {store_value.public!r}.\n"
+                "Mutation should not be done on the value of a reactive variable, as in production mode we would be unable to track changes.\n"
+            )
+            if tb:
+                if tb.code_context:
+                    code = tb.code_context[0]
+                else:
+                    code = "<No code context available>"
+                msg += f"The last value was read in the following code:\n" f"{tb.filename}:{tb.lineno}\n" f"{code}"
+            raise ValueError(msg)
+        elif store_value.set_value is not None and not self.equals(store_value.set_value, store_value.private):
+            tb = store_value.set_traceback
+            msg = f"""Reactive variable was set with a value of {store_value.private!r}, but was later mutated mutated to {store_value.set_value!r}.
+
+Mutation should not be done on the value of a reactive variable, as in production mode we would be unable to track changes.
+
+Bad:
+    mylist = reactive([]]
+    some_values = [1, 2, 3]
+    mylist.value = some_values  # you give solara a reference to your list
+    some_values.append(4)  # but later mutate it (solara cannot detect this change, so a render will not be triggered)
+    # if later on a re-render happens for a different reason, you will read of the mutated list.
+
+Good (if you want the reactive variable to be updated):
+    mylist = reactive([]]
+    some_values = [1, 2, 3]
+    mylist.value = some_values
+    mylist.value = some_values + [4]
+
+Good (if you want to keep mutating your own list):
+    mylist = reactive([]]
+    some_values = [1, 2, 3]
+    mylist.value = some_values.copy()  # this gives solara a copy of the list
+    some_values.append(4)  # you are free to mutate your own list, solara will not see this
+
+"""
+            if tb:
+                if tb.code_context:
+                    code = tb.code_context[0]
+                else:
+                    code = "<No code context available>"
+                msg += "The last time the value was set was at:\n" f"{tb.filename}:{tb.lineno}\n" f"{code}"
+            raise ValueError(msg)
+
+    def _ensure_public_exists(self):
+        store_value = self._storage.peek()
+        if isinstance(store_value.public, _PublicValueNotSet):
+            with self.lock:
+                if isinstance(store_value.public, _PublicValueNotSet):
+                    frame = _find_outside_solara_frame()
+                    if frame is not None:
+                        frame_info = inspect.getframeinfo(frame)
+                    else:
+                        frame_info = None
+                    store_value.public = copy.deepcopy(store_value.private)
+                    self._check_equals(store_value.public, store_value.private)
+                    store_value.get_traceback = frame_info
+
+    def _check_equals(self, a: S, b: S):
+        if not self._enabled:
+            return
+        if not self.equals(a, b):
+            frame = _find_outside_solara_frame()
+            if frame is not None:
+                frame_info = inspect.getframeinfo(frame)
+            else:
+                frame_info = None
+
+            warn = """The equals function for this reactive value returned False when comparing a deepcopy to itself.
+
+This reactive variable will not be able to detect mutations correctly, and is therefore disabled.
+
+To avoid this warning, and to ensure that mutation detection works correctly, please provide a better equals function to the reactive variable.
+A good choice for dataframes and numpy arrays might be solara.util.equals_pickle, which will also attempt to compare the pickled values of the objects.
+
+Example:
+df = pd.DataFrame({"a": [1, 2, 3], "b": [4, 5, 6]})
+reactive_df = solara.reactive(df, equals=solara.util.equals_pickle)
+"""
+            tb = frame_info
+            if tb:
+                if tb.code_context:
+                    code = tb.code_context[0]
+                else:
+                    code = "<No code context available>"
+                warn += "This warning was triggered from:\n" f"{tb.filename}:{tb.lineno}\n" f"{code}"
+            warnings.warn(warn)
+            self._enabled = False
+
+    def subscribe(self, listener: Callable[[S], None], scope: Optional[ContextManager] = None):
+        def listener_wrapper(new: StoreValue[S], previous: StoreValue[S]):
+            self._ensure_public_exists()
+            assert new.public is not None
+            assert previous.public is not None
+            previous_value = previous.set_value if previous.set_value is not None else previous.private
+            new_value = new.set_value
+            assert new_value is not None
+            if not self.equals(new_value, previous_value):
+                listener(new_value)
+
+        return self._storage.subscribe_change(listener_wrapper, scope=scope)
+
+    def subscribe_change(self, listener: Callable[[S, S], None], scope: Optional[ContextManager] = None):
+        def listener_wrapper(new: StoreValue[S], previous: StoreValue[S]):
+            self._ensure_public_exists()
+            assert new.public is not None
+            assert previous.public is not None
+            previous_value = previous.set_value if previous.set_value is not None else previous.private
+            new_value = new.set_value
+            assert new_value is not None
+            if not self.equals(new_value, previous_value):
+                listener(new_value, previous_value)
+
+        return self._storage.subscribe_change(listener_wrapper, scope=scope)

solara/hooks/use_reactive.py

@@ -1,4 +1,4 @@
-from typing import Callable, Optional, TypeVar, Union
+from typing import Any, Callable, Optional, TypeVar, Union
 
 import solara
 
@@ -8,6 +8,7 @@
 def use_reactive(
     value: Union[T, solara.Reactive[T]],
     on_change: Optional[Callable[[T], None]] = None,
+    equals: Callable[[Any, Any], bool] = solara.util.equals_extra,
 ) -> solara.Reactive[T]:
     """Creates a reactive variable with the a local component scope.
 
@@ -44,6 +45,12 @@ def Page():
      * on_change (Optional[Callable[[T], None]]): An optional callback function
             that will be called when the reactive variable's value changes.
 
+     * equals: A function that returns True if two values are considered equal, and False otherwise.
+            The default function is `solara.util.equals`, which performs a deep comparison of the two values
+            and is more forgiving than the default `==` operator.
+            You can provide a custom function if you need to define a different notion of equality.
+
+
     Returns:
         solara.Reactive[T]: A reactive variable with the specified initial value
             or the provided reactive variable.

solara/reactive.py

@@ -1,13 +1,14 @@
-from typing import TypeVar
+from typing import Any, Callable, TypeVar
 
 from solara.toestand import Reactive
+import solara.util
 
 __all__ = ["reactive", "Reactive"]
 
 T = TypeVar("T")
 
 
-def reactive(value: T) -> Reactive[T]:
+def reactive(value: T, equals: Callable[[Any, Any], bool] = solara.util.equals_extra) -> Reactive[T]:
     """Creates a new Reactive object with the given initial value.
 
     Reactive objects are mostly used to manage global or application-wide state in
@@ -35,6 +36,11 @@ def reactive(value: T) -> Reactive[T]:
 
     Args:
         value (T): The initial value of the reactive variable.
+        equals: A function that returns True if two values are considered equal, and False otherwise.
+            The default function is `solara.util.equals`, which performs a deep comparison of the two values
+            and is more forgiving than the default `==` operator.
+            You can provide a custom function if you need to define a different notion of equality.
+
 
     Returns:
         Reactive[T]: A new Reactive object with the specified initial value.
@@ -90,4 +96,4 @@ def Page():
     Whenever the counter value changes, `CounterDisplay` automatically updates to display the new value.
 
     """
-    return Reactive(value)
+    return Reactive(value, equals=equals)

solara/settings.py

@@ -61,9 +61,23 @@ class Config:
         env_file = ".env"
 
 
+class Storage(BaseSettings):
+    mutation_detection: Optional[bool] = None  # True/False, or None to auto determine
+    factory: str = "solara.toestand.default_storage"
+
+    def get_factory(self):
+        return solara.util.import_item(self.factory)
+
+    class Config:
+        env_prefix = "solara_storage_"
+        case_sensitive = False
+        env_file = ".env"
+
+
 assets: Assets = Assets()
 cache: Cache = Cache()
 main = MainSettings()
+storage = Storage()
 
 if main.check_hooks not in ["off", "warn", "raise"]:
     raise ValueError(f"Invalid value for check_hooks: {main.check_hooks}, expected one of ['off', 'warn', 'raise']")