Reject browser storage defaults produced by a default_factory

docs/api-reference/browser_storage.md

@@ -34,6 +34,18 @@ cannot be passed as a keyword argument — the keyword arguments (`name`,
 `max_age`, `sync`, etc.) only configure the storage behavior.
 ```
 
+```md alert info
+# Declare browser storage as a default value or a storage annotation.
+
+A var is stored in the browser when its default is a storage value, such as
+`theme: str = rx.LocalStorage("light", name="theme")`, or when it is annotated
+with a storage type, such as `theme: rx.LocalStorage = "light"`. The storage key
+and options must be known when the app compiles, so a `default_factory` cannot
+provide them: `rx.field(default_factory=lambda: rx.LocalStorage(...))` fails to
+compile on a var annotated with a storage type or with a type that accepts a
+`str`, such as `str`, `str | None` or a `Literal` of strings.
+```
+
 ```md alert warning
 # **The default value of a Cookie is never set in the browser!**
 

docs/vars/base_vars.md

@@ -106,7 +106,10 @@ storage var keeps its storage name and options only with a storage value as its
 default, such as `rx.LocalStorage("dark", name="theme")`. Given a plain string,
 a var annotated `str` becomes an ordinary var, and one annotated with a storage
 type, such as `rx.LocalStorage`, stays in browser storage under the default key
-and options.
+and options. A `default_factory` cannot declare browser storage: the app fails
+to compile when a storage-typed var has a factory, or when the factory of a var
+that accepts a `str` returns a storage value. See
+[Browser Storage](/docs/api-reference/browser-storage/).
 
 An inherited var belongs to the state that declared it, so changing its field
 also changes the default for every state that inherits it. Each generated

news/+reject-factory-storage-defaults.breaking.md

@@ -0,0 +1 @@
+Browser storage vars (`rx.Cookie`, `rx.LocalStorage`, `rx.SessionStorage`) must be declared with a storage value as the default or with a storage type annotation. A `default_factory` on a storage var, or one returning a storage value, now fails at compile time instead of being silently ignored or treated as an ordinary var.

reflex/compiler/utils.py

@@ -11,9 +11,10 @@
 import tempfile
 import traceback
 from collections.abc import Iterable, Mapping, Sequence
+from dataclasses import MISSING
 from datetime import datetime
 from pathlib import Path
-from typing import TYPE_CHECKING, Any, TypedDict
+from typing import TYPE_CHECKING, Any, Literal, TypedDict, get_args, get_origin
 from urllib.parse import urlparse
 
 from reflex_base import constants
@@ -28,7 +29,8 @@
 from reflex_base.constants.state import FIELD_MARKER
 from reflex_base.registry import RegistrationContext
 from reflex_base.style import Style
-from reflex_base.utils import format, imports, memo_paths, serializers
+from reflex_base.utils import format, imports, memo_paths, serializers, types
+from reflex_base.utils.exceptions import VarTypeError
 from reflex_base.utils.imports import ImportVar, ParsedImportDict
 from reflex_base.vars.base import Field, Var, VarData
 from reflex_base.vars.function import DestructuredArg
@@ -40,7 +42,12 @@
 from reflex_components_core.el.elements.sectioning import Body
 
 from reflex.istate.delta import _resolve_delta
-from reflex.istate.storage import Cookie, LocalStorage, SessionStorage
+from reflex.istate.storage import (
+    ClientStorageBase,
+    Cookie,
+    LocalStorage,
+    SessionStorage,
+)
 from reflex.state import BaseState, cache_initial_snapshot
 from reflex.utils import path_ops
 from reflex.utils.exec import is_prod_mode
@@ -326,17 +333,64 @@ def _compile_client_storage_field(
         if isinstance(field.default, field_type):
             cs_obj = field.default
         elif isinstance(field.type_, type) and issubclass(field.type_, field_type):
-            # A factory-backed field carries its options in what it produces.
-            produced = (
-                field.default_factory() if field.default_factory is not None else None
-            )
-            cs_obj = produced if isinstance(produced, field_type) else field.type_()
+            if field.default is MISSING:
+                _raise_factory_storage(field, field.type_)
+            cs_obj = field.type_()
         else:
             continue
         return field_type, cs_obj.options()
+    if (
+        not field._backend
+        and field.default is MISSING
+        and field.default_factory is not None
+        and _may_hold_str(field.outer_type_)
+        and isinstance(produced := field.default_factory(), ClientStorageBase)
+    ):
+        _raise_factory_storage(field, type(produced))
     return None, None
 
 
+def _may_hold_str(type_: Any) -> bool:
+    """Whether a value of the type can be a browser storage value, a str subclass.
+
+    Args:
+        type_: The field's annotated type.
+
+    Returns:
+        Whether the type accepts a str or a str subclass.
+    """
+    type_ = types.resolve_type_alias(type_)
+    if type_ is Any:
+        return True
+    if types.is_union(type_):
+        return any(_may_hold_str(arg) for arg in get_args(type_))
+    if get_origin(type_) is Literal:
+        return any(isinstance(arg, str) for arg in get_args(type_))
+    return types.safe_issubclass(str, type_) or types.safe_issubclass(type_, str)
+
+
+def _raise_factory_storage(field: Field, storage_type: type) -> None:
+    """Reject a browser storage default produced by a default_factory.
+
+    The storage name and options must be known when the app compiles, so a
+    storage var declares them with a storage value as its default.
+
+    Args:
+        field: The field with a default_factory.
+        storage_type: The browser storage type the field declares or produces.
+
+    Raises:
+        VarTypeError: Always.
+    """
+    owner = field._owner.__name__ if field._owner is not None else ""
+    msg = (
+        f"Browser storage var '{owner}.{field._name}' cannot use a default_factory."
+        " Declare the storage as the default value instead, such as"
+        f' `{field._name}: str = rx.{storage_type.__name__}("", name={field._name!r})`.'
+    )
+    raise VarTypeError(msg)
+
+
 def _compile_client_storage_recursive(
     state: type[BaseState],
 ) -> tuple[

tests/integration/tests_playwright/test_hydration_storage.py

@@ -79,26 +79,20 @@ class AssignedStorageState(rx.State):
         local: str = rx.LocalStorage("declared", name="assigned-local")
         session: str = rx.SessionStorage("declared", name="assigned-session")
         cookie: str = rx.Cookie("declared", name="assigned-cookie")
-        factory: rx.Field[str] = rx.field(
-            default_factory=lambda: rx.LocalStorage("declared", name="assigned-factory")
-        )
 
         @rx.event
         def change(self):
             """Change the values the browser stores."""
             self.local = "changed"
             self.session = "changed"
             self.cookie = "changed"
-            self.factory = "changed"
 
     fields = AssignedStorageState.__fields__
     fields["local"].set_default(rx.LocalStorage("assigned", name="assigned-local"))
     fields["session"].set_default(
         rx.SessionStorage("assigned", name="assigned-session")
     )
     fields["cookie"].set_default(rx.Cookie("assigned", name="assigned-cookie"))
-    # A storage value as the default replaces the declared factory.
-    fields["factory"].set_default(rx.LocalStorage("assigned", name="assigned-factory"))
 
     class PreferenceState(rx.ComponentState):
         pref: str = rx.LocalStorage("declared", name="assigned-pref")
@@ -233,7 +227,6 @@ def assigned():
             rx.text(AssignedStorageState.local, id="assigned-local"),
             rx.text(AssignedStorageState.session, id="assigned-session"),
             rx.text(AssignedStorageState.cookie, id="assigned-cookie"),
-            rx.text(AssignedStorageState.factory, id="assigned-factory"),
             rx.button("Change", on_click=AssignedStorageState.change, id="change"),
             PreferenceState.create(initial="assigned"),
             rx.text(rx.cond(rx.State.is_hydrated, "true", "false"), id="hydrated"),
@@ -375,7 +368,7 @@ def test_storage_annotations_hydrate_and_persist(
 
 
 # The browser storage vars of the assigned page, by element id suffix.
-VARS = ("local", "session", "cookie", "factory", "pref")
+VARS = ("local", "session", "cookie", "pref")
 
 
 def test_assigned_storage_default_persists(
@@ -400,7 +393,6 @@ def test_assigned_storage_default_persists(
         localStorage.getItem('assigned-local') === 'changed' &&
         sessionStorage.getItem('assigned-session') === 'changed' &&
         localStorage.getItem('assigned-pref') === 'changed' &&
-        localStorage.getItem('assigned-factory') === 'changed' &&
         document.cookie.split('; ').includes('assigned-cookie=changed')
     """)
 

tests/units/compiler/test_compiler_utils.py

@@ -2,6 +2,7 @@
 
 import asyncio
 from copy import deepcopy
+from typing import Annotated, Literal, TypedDict
 
 import pytest
 from reflex_base import constants
@@ -18,6 +19,7 @@
 from reflex.compiler.utils import write_file as compiler_write_file
 from reflex.constants.state import FIELD_MARKER
 from reflex.state import State, state_snapshot_hashes
+from reflex.utils.exceptions import VarTypeError
 from reflex.utils.path_ops import write_file
 from reflex.vars.base import computed_var
 
@@ -232,37 +234,152 @@ class MixedKeyState(State):
     assert len(state_snapshot_hashes(compiled)) == len(compiled) + 1
 
 
-def test_compile_client_storage_honors_default_factories(
+@pytest.mark.parametrize(
+    "annotation",
+    [rx.Cookie, rx.LocalStorage, rx.SessionStorage],
+    ids=lambda t: t.__name__,
+)
+def test_compile_client_storage_rejects_factory_on_storage_annotation(
+    annotation: type,
     forked_registration_context: RegistrationContext,
 ):
-    """Factory-backed browser storage fields compile with the options they produce.
+    """A browser storage var annotated with a storage type takes no default_factory.
+
+    Args:
+        annotation: The browser storage type the var is annotated with.
+        forked_registration_context: Keeps the test's state out of other tests.
+    """
+    # Built with type() so the annotation is the parametrized storage type.
+    StorageState = type(
+        "StorageState",
+        (State,),
+        {
+            "__module__": __name__,
+            "__qualname__": "test_rejects_factory_on_storage_annotation.<locals>.StorageState",
+            "__annotations__": {"value": annotation},
+            "value": rx.field(default_factory=lambda: annotation("new", name="key")),
+        },
+    )
+
+    with pytest.raises(VarTypeError, match=r"\.value' cannot use a default_factory"):
+        utils.compile_client_storage(StorageState)
+
+
+def test_compile_client_storage_rejects_factory_on_set_default(
+    forked_registration_context: RegistrationContext,
+):
+    """set_default with a default_factory is rejected on a browser storage var.
 
     Args:
         forked_registration_context: Keeps the test's state out of other tests.
     """
 
     class StorageState(State):
-        cookie: rx.Field[rx.Cookie] = rx.field(
-            default_factory=lambda: rx.Cookie("new", name="cookie-key", max_age=60)
+        value: rx.LocalStorage = rx.LocalStorage("declared", name="storage-key")
+
+    StorageState.__fields__["value"].set_default(
+        default_factory=lambda: rx.LocalStorage("new", name="storage-key")
+    )
+    with pytest.raises(VarTypeError, match=r"\.value' cannot use a default_factory"):
+        utils.compile_client_storage(StorageState)
+
+
+def test_compile_client_storage_rejects_factory_producing_storage(
+    forked_registration_context: RegistrationContext,
+):
+    """A plain-annotated var whose default_factory produces browser storage is rejected.
+
+    Args:
+        forked_registration_context: Keeps the test's state out of other tests.
+    """
+
+    class StorageState(State):
+        theme: rx.Field[str] = rx.field(
+            default_factory=lambda: rx.LocalStorage("light", name="theme")
         )
-        local: rx.Field[rx.LocalStorage] = rx.field(
-            default_factory=lambda: rx.LocalStorage("new", name="local-key", sync=True)
+
+    class OptionalStorageState(State):
+        theme: rx.Field[str | None] = rx.field(
+            default_factory=lambda: rx.LocalStorage("light", name="theme")
         )
-        session: rx.Field[rx.SessionStorage] = rx.field(
-            default_factory=lambda: rx.SessionStorage("new", name="session-key")
+
+    class OptionalAnnotatedStorageState(State):
+        theme: rx.Field[rx.LocalStorage | None] = rx.field(
+            default_factory=lambda: rx.LocalStorage("light", name="theme")
         )
 
-    compiled = utils.compile_client_storage(StorageState)
-    name = StorageState.get_full_name()
-    cookie = compiled[constants.COOKIES][f"{name}.cookie{FIELD_MARKER}"]
-    assert (cookie["name"], cookie["maxAge"]) == ("cookie-key", 60)
-    assert compiled[constants.LOCAL_STORAGE][f"{name}.local{FIELD_MARKER}"] == {
-        "name": "local-key",
-        "sync": True,
-    }
-    assert compiled[constants.SESSION_STORAGE][f"{name}.session{FIELD_MARKER}"] == {
-        "name": "session-key"
+    class LiteralStorageState(State):
+        theme: rx.Field[Literal["light", "dark"]] = rx.field(  # pyright: ignore[reportAssignmentType]
+            default_factory=lambda: rx.LocalStorage("light", name="theme")
+        )
+
+    class MetadataStorageState(State):
+        theme: rx.Field[Annotated[str, "meta"]] = rx.field(
+            default_factory=lambda: rx.LocalStorage("light", name="theme")
+        )
+
+    for state in (
+        StorageState,
+        OptionalStorageState,
+        OptionalAnnotatedStorageState,
+        LiteralStorageState,
+        MetadataStorageState,
+    ):
+        with pytest.raises(
+            VarTypeError, match=r"\.theme' cannot use a default_factory"
+        ):
+            utils.compile_client_storage(state)
+
+
+class Node(TypedDict):
+    """A typed dict default, which issubclass refuses to check."""
+
+    name: str
+
+
+def test_compile_client_storage_skips_plain_factories(
+    forked_registration_context: RegistrationContext,
+):
+    """A default_factory producing a plain value compiles as an ordinary var.
+
+    Args:
+        forked_registration_context: Keeps the test's state out of other tests.
+    """
+    calls = []
+
+    def make_text() -> str:
+        """Record the call and return a plain default.
+
+        Returns:
+            The default text.
+        """
+        calls.append(1)
+        return "plain"
+
+    def missing_at_compile() -> str:
+        """Fail: a backend factory is never called while compiling.
+
+        Raises:
+            RuntimeError: Always.
+        """
+        msg = "only available at runtime"
+        raise RuntimeError(msg)
+
+    class PlainState(State):
+        text: rx.Field[str] = rx.field(default_factory=make_text)
+        _token: rx.Field[str] = rx.field(default_factory=missing_at_compile)
+        items: rx.Field[list[str]] = rx.field(default_factory=list)
+        # issubclass against a TypedDict raises, so the check must not ask it.
+        node: rx.Field[Node] = rx.field(default_factory=lambda: Node(name="root"))
+        local: str = rx.LocalStorage("declared", name="local-key")
+
+    compiled = utils.compile_client_storage(PlainState)
+    name = PlainState.get_full_name()
+    assert compiled[constants.LOCAL_STORAGE] == {
+        f"{name}.local{FIELD_MARKER}": {"name": "local-key", "sync": False}
     }
+    assert compiled[constants.COOKIES] == compiled[constants.SESSION_STORAGE] == {}
+    assert calls == [1]
 
 
 def test_document_root_allows_static_id_on_head_script():