diff --git a/news/7074.bugfix.md b/news/7074.bugfix.md new file mode 100644 index 00000000000..17fd03f35e5 --- /dev/null +++ b/news/7074.bugfix.md @@ -0,0 +1 @@ +Warn when a substate declares a var whose name is already a var on a parent state. Such a declaration is ignored — reads and writes resolve to the parent's var and class-level access returns the raw default instead of a reactive `Var` — and previously produced no diagnostic at all. diff --git a/reflex/state.py b/reflex/state.py index 1ea080cc863..cee25d4024a 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -344,7 +344,7 @@ def _has_data_descriptor(cls: type, name: str) -> bool: return False -def _is_user_descriptor(value: Any) -> bool: +def _is_user_descriptor(value: Any, *, include_properties: bool = False) -> bool: """Whether a class attribute is a user-defined descriptor. Excludes framework-recognized callables and var types so user-defined @@ -353,6 +353,7 @@ def _is_user_descriptor(value: Any) -> bool: Args: value: The class attribute value to check. + include_properties: Whether property-like descriptors also count. Returns: True if the value is a custom descriptor. @@ -365,14 +366,16 @@ def _is_user_descriptor(value: Any) -> bool: FunctionType, classmethod, staticmethod, - property, - functools.cached_property, EventHandler, Var, Field, ), ): return False + if not include_properties and isinstance( + value, (property, functools.cached_property) + ): + return False return not is_computed_var(value) @@ -663,6 +666,9 @@ def __init_subclass__(cls, mixin: bool = False, **kwargs): if k not in own_descriptor_names } + # Base vars silently lose to an inherited var of the same name; warn about it. + cls._check_overridden_inherited_vars() + # Get computed vars. computed_vars = cls._get_computed_vars() cls._check_overridden_computed_vars() @@ -1110,6 +1116,71 @@ def _check_overridden_computed_vars(cls) -> None: msg = f"The computed var name `{cv._js_expr}` shadows a var in {cls.__module__}.{cls.__name__}; use a different name instead" raise ComputedVarShadowsStateVarError(msg) + @classmethod + def _state_field_precedes_descriptor(cls, name: str) -> bool: + """Whether a state base declaring name outranks a same-named descriptor. + + Re-annotating is how a state field that already wins over a descriptor on a + non-state base is kept, so that redeclaration is inert rather than a mistake. + A descriptor that instead outranks the state field does not make the + redeclaration take effect, so it is not exempt. + + Args: + name: The var name to look up. + + Returns: + True if a state base declaring name precedes a non-state descriptor. + """ + state_first = False + for base in cls.__mro__[1:]: + if name not in base.__dict__: + continue + if issubclass(base, BaseState): + state_first = True + elif _is_user_descriptor(base.__dict__[name], include_properties=True): + return state_first + return False + + @classmethod + def _check_overridden_inherited_vars(cls) -> None: + """Warn about base vars that shadow a var inherited from a parent state. + + Such a redeclaration is dropped silently: the field never becomes a base var, + so reads and writes resolve to the parent's var and class-level access returns + the raw default instead of a Var. + + A redeclaration that exists to win over a descriptor reached through a + non-state base is left alone, since re-annotating is how that MRO conflict + is resolved. + """ + parent_state = cls.get_parent_state() + if parent_state is None: + return + parent_fields = parent_state.get_fields() + for name, own_field in cls.get_fields().items(): + if ( + name.startswith("_") + or not own_field.is_var + or name not in cls.inherited_vars + ): + continue + # A field redeclared on this class is a distinct object from the parent's; + # a merely inherited one is the same object. + parent_field = parent_fields.get(name) + if ( + parent_field is None + or parent_field is own_field + or cls._state_field_precedes_descriptor(name) + ): + continue + console.warn( + f"The var `{name}` in {cls.__module__}.{cls.__name__} shadows a var " + f"inherited from {parent_state.__module__}.{parent_state.__name__} and " + "is ignored: reads and writes resolve to the parent's var. Use a " + "different name instead.", + dedupe=True, + ) + @classmethod def get_skip_vars(cls) -> set[str]: """Get the vars to skip when serializing. diff --git a/tests/units/test_app.py b/tests/units/test_app.py index 932456cd6ed..9d0b8ca7e4e 100644 --- a/tests/units/test_app.py +++ b/tests/units/test_app.py @@ -1779,7 +1779,6 @@ class DynamicState(State): recalculated when the dynamic route var was dirty """ - is_hydrated: bool = False loaded: int = 0 counter: int = 0 diff --git a/tests/units/test_state.py b/tests/units/test_state.py index 37c0ed2fc4c..4801cc340df 100644 --- a/tests/units/test_state.py +++ b/tests/units/test_state.py @@ -5380,3 +5380,84 @@ def test_setattr_alias_annotated_var(mocker: MockerFixture): state.key = 1 # pyright: ignore[reportAttributeAccessIssue] assert state.key == 1 error_mock.assert_called_once() + + +def test_base_var_shadowing_inherited_var_warns(mocker: MockerFixture) -> None: + """A base var shadowing an inherited var warns instead of being dropped silently. + + Args: + mocker: Pytest mock fixture. + """ + warn_mock = mocker.patch("reflex.state.console.warn") + + class ShadowParent(BaseState): + shadowed_value: int = 1 + + class ShadowChild(ShadowParent): + shadowed_value: str = "ninety-nine" # pyright: ignore[reportIncompatibleVariableOverride, reportAssignmentType] + + assert any("shadowed_value" in call.args[0] for call in warn_mock.call_args_list), ( + "expected a warning naming the shadowed var" + ) + + +def test_base_var_shadowing_non_state_descriptor_does_not_warn( + mocker: MockerFixture, +) -> None: + """Re-annotating to win over a descriptor from a non-state base is not a shadow. + + Args: + mocker: Pytest mock fixture. + """ + from reflex_base.vars.hybrid_property import hybrid_property + + warn_mock = mocker.patch("reflex.state.console.warn") + + class SharedMixin: + @hybrid_property + def descriptor_value(self) -> int: + return 1 + + class PlainBase(SharedMixin): + pass + + class OverridingState(SharedMixin, BaseState): + descriptor_value: int = 5 # pyright: ignore[reportIncompatibleVariableOverride, reportAssignmentType] + + class DescriptorChild(PlainBase, OverridingState): + descriptor_value: int # pyright: ignore[reportGeneralTypeIssues, reportIncompatibleVariableOverride] + + assert not [ + call for call in warn_mock.call_args_list if "descriptor_value" in call.args[0] + ], "re-annotation resolving a descriptor MRO conflict must not warn" + + +def test_base_var_shadowing_warns_when_descriptor_outranks_state_field( + mocker: MockerFixture, +) -> None: + """A descriptor closer than the state field does not exempt a dropped declaration. + + Args: + mocker: Pytest mock fixture. + """ + from reflex_base.vars.hybrid_property import hybrid_property + + warn_mock = mocker.patch("reflex.state.console.warn") + + class CloserMixin: + @hybrid_property + def outranked_value(self) -> int: + return 1 + + class OutrankedParent(BaseState): + outranked_value: int = 1 # pyright: ignore[reportIncompatibleVariableOverride, reportAssignmentType] + + class OutrankedChild(CloserMixin, OutrankedParent): + outranked_value: str = "x" # pyright: ignore[reportIncompatibleVariableOverride, reportAssignmentType] + + assert "outranked_value" not in OutrankedChild.base_vars, ( + "declaration is still dropped, so it must not be treated as effective" + ) + assert any( + "outranked_value" in call.args[0] for call in warn_mock.call_args_list + ), "expected a warning when the descriptor outranks the state field"