Warn when a base var shadows a var inherited from a parent state

[blockgroot]

Problem

Fixes #7074. When a substate declares a field whose name matches a base var it inherits, the declaration is discarded with no warning and no error. get_skip_vars() includes set(cls.inherited_vars) and base_vars filters on it, so the field stays in get_fields() but never becomes a base var: _init_var never runs, class-level access returns pydantic's raw default instead of a Var, and instance reads and writes are delegated to the parent.

The reporter's repro, unchanged:

class Parent(State):
    x: int = 1

class Child(Parent):
    x: str = "ninety-nine"   # silently ignored

child.x is 1 (an int), a write to child.x lands on parent.x, child.dirty_vars stays empty, and repr(Child.x) is a plain str — so rx.text(Child.x) compiles a literal into the page instead of a reactive binding.

Change

BaseState._check_overridden_inherited_vars() runs at class creation and emits a deduped console.warn naming the var and both states. A field redeclared on a class is a distinct Field object from the parent's, while a merely inherited one is the same object — that identity check is the discriminator, since on Python 3.14 a bare annotation carries no marker distinguishing it from an explicit assignment (__annotations__ is not even populated in cls.__dict__).

Why warn instead of raise

The issue asks for a raise, and the shadowing computed var guards do raise (ComputedVarShadowsStateVarError). I went with a warning because CLAUDE.md says "Reflex has downstream users — don't break them", and raising turns code that runs today into an import-time crash. The warning fully addresses the reported failure mode (silence), and escalating to an exception later is a small change. Happy to switch it if you'd rather have the hard error.

One deliberate exemption

tests/units/vars/test_hybrid_property.py::test_hybrid_property_shadowed_by_closer_base_stays_a_field re-annotates an inherited var on purpose, in a hierarchy where the state field already outranks a hybrid_property reached through a non-state base. A redeclaration is therefore skipped only when both a same-named non-state descriptor exists in the MRO and a state base declaring the name precedes it — the ordering in which the field wins anyway, making the re-annotation inert. With no descriptor in the MRO, an ordinary redeclaration still warns.

_is_user_descriptor gained an include_properties keyword for this (default False, so the existing call site is unchanged) because HybridProperty subclasses property, which that helper otherwise excludes.

The resulting invariant: the warning fires exactly when the shadowed name stops resolving to a Var at class level, which is the case that actually breaks a component. See the table in the review thread for the three hierarchies this was measured against.

Incidental

DynamicState in tests/units/test_app.py declared is_hydrated: bool = False, shadowing the root State var of the same name and identical default — already a no-op, and the first thing the new warning flagged. Removed.

Testing

Three tests in tests/units/test_state.py:

• test_base_var_shadowing_inherited_var_warns — fails before this change, passes after.
• test_base_var_shadowing_non_state_descriptor_does_not_warn — guards the exemption so the warning can't over-fire on the legitimate MRO pattern.
• test_base_var_shadowing_warns_when_descriptor_outranks_state_field — added after review, locks out the false negative where a descriptor precedes the state field.

uv run pytest tests/units → 8442 passed, 18 skipped. ruff check, ruff format --check and pyright reflex/state.py tests/units/test_state.py are clean. News fragment added at news/7074.bugfix.md.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge, with no distinct new actionable findings from the changes since the previous review.

Summary

• Distinguishes redeclared Pydantic fields from merely inherited fields by identity.
• Preserves the legitimate hybrid-property multiple-inheritance pattern when a state field precedes the descriptor in the MRO.
• Narrows the descriptor exemption and adds regression coverage for both descriptor orderings.
• Removes an existing inert is_hydrated redeclaration and adds a user-facing bug-fix fragment.

_{Reviews (2) · Last reviewed commit: "Narrow the descriptor exemption to where..."}

[cubic-dev-ai]

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="reflex/state.py">

<violation number="1" location="reflex/state.py:1129">
P2: When a parent state already overrides a descriptor from a non-state mixin, a child redeclaration is still a shadow, but this scan suppresses its warning. Stop at the first ancestor defining `name` so only the effective non-state descriptor exempts the redeclaration.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When a parent state already overrides a descriptor from a non-state mixin, a child redeclaration is still a shadow, but this scan suppresses its warning. Stop at the first ancestor defining name so only the effective non-state descriptor exempts the redeclaration.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At reflex/state.py, line 1129:

<comment>When a parent state already overrides a descriptor from a non-state mixin, a child redeclaration is still a shadow, but this scan suppresses its warning. Stop at the first ancestor defining `name` so only the effective non-state descriptor exempts the redeclaration.</comment>

<file context>
@@ -1110,6 +1116,63 @@ def _check_overridden_computed_vars(cls) -> None:
+        Returns:
+            True if a non-state base in the MRO defines name as a user descriptor.
+        """
+        return any(
+            not issubclass(base, BaseState)
+            and _is_user_descriptor(base.__dict__[name], include_properties=True)
</file context>

Suggested change

	return any(
	not issubclass(base, BaseState)
	and _is_user_descriptor(base.__dict__[name], include_properties=True)
	for base in cls.__mro__
	if name in base.__dict__
	)
	for base in cls.__mro__[1:]:
	if name not in base.__dict__:
	continue
	return (
	not issubclass(base, BaseState)
	and _is_user_descriptor(
	base.__dict__[name], include_properties=True
	)
	)
	return False

[blockgroot]

Thanks both — Greptile's P1 was valid and is fixed in b4c2d768.

The exemption was too broad: it accepted a same-named descriptor anywhere in the MRO without checking whether that descriptor was actually the reason for the redeclaration. In Child(DescriptorMixin, ParentState) the declaration is still discarded (value absent from base_vars, Child.value is the raw 'x' rather than a Var) yet the warning was suppressed. It now requires both that a same-named non-state descriptor exists and that a state base declaring the name precedes it in the MRO — the ordering in which the field already wins, making the re-annotation inert.

@cubic-dev-ai — same root cause, but I didn't apply the suggested patch, because stopping at the first ancestor defining the name inverts two of the three cases:

case	declaration dropped?	Cls.value is a Var?	should warn	suggested rule	this PR
Child(Parent), plain redeclare	yes	no	yes	warns ✓	warns ✓
Child(DescriptorMixin, ParentState)	yes	no	yes	silent ✗	warns ✓
Child(Plain, Overriding) over Shared (the existing test_hybrid_property_shadowed_by_closer_base_stays_a_field)	yes	yes	no	warns ✗	silent ✓

The third column is the invariant that fell out of this: the warning fires exactly when the shadowed name stops resolving to a Var at class level, which is the case that actually breaks a component. Row 3 still resolves through the parent, so that redeclaration is redundant rather than harmful — and it's an existing test, so warning there would be a false positive.

Regression test added for row 2; uv run pytest tests/units → 8442 passed.

[cubic-dev-ai]

Thanks both — Greptile's P1 was valid and is fixed in b4c2d768.

The exemption was too broad: it accepted a same-named descriptor anywhere in the MRO without checking whether that descriptor was actually the reason for the redeclaration. In Child(DescriptorMixin, ParentState) the declaration is still discarded (value absent from base_vars, Child.value is the raw 'x' rather than a Var) yet the warning was suppressed. It now requires both that a same-named non-state descriptor exists and that a state base declaring the name precedes it in the MRO — the ordering in which the field already wins, making the re-annotation inert.

@cubic-dev-ai — same root cause, but I didn't apply the suggested patch, because stopping at the first ancestor defining the name inverts two of the three cases:
...

@blockgroot You're right, and that's a fair correction to my earlier suggestion. The "stop at the first ancestor that defines the name" rule I proposed was too coarse — it keys off MRO position alone, not on whether the redeclaration is actually doing anything. Your table shows exactly where it breaks: it can't distinguish "the descriptor swallowed the field" (row 2, should warn) from "the field still wins and the re-annotation is inert" (row 3, shouldn't).

I checked the implemented logic against your three rows and it lands where you say:

• Row 1 Child(Parent): only state bases carry the name, never a descriptor, so _state_field_precedes_descriptor returns False → warns ✓
• Row 2 Child(DescriptorMixin, ParentState): the descriptor is hit first with state_first=False, returns False → not exempt → warns ✓
• Row 3 Child(Plain, Overriding) over Shared: OverridingState sets state_first=True before SharedMixin's descriptor is reached → returns True → silent ✓

And the invariant you landed on is the meaningful one: warn iff the shadowed name stops resolving to a Var at class level. That's the case that actually changes compiled output, and it's exactly what the base_vars/identity-check machinery is measuring. The descriptor-ordering rule is a decent proxy for it, and _state_field_precedes_descriptor reads clearly enough.

Only thing I'd flag as a residual edge, not a blocker: the exemption keys entirely on ordering in the MRO, so a same-named descriptor that appears before the state field but doesn't actually swallow the field (e.g. a non-data descriptor) would still get a warning even though Cls.value might resolve fine. That's the one spot where "ordering" and "resolves to a Var" could drift apart. If you want to be strict about it, the identity/is_var check is the more direct test — but given the existing hybrid-property test is the only in-tree consumer, the current scoping is reasonable as-is.

_{Tip: get faster answers by chatting with cubic’s review copilot in the review UI}

[masenf]

something about this phrasing reads as confusing to me, because calling the function with include_properties=True is like saying "include properties in the list of explicitly excluded descriptors that should NOT be considered user descriptors".

i think it's more clear to name this parameter as exclude_properties: bool = True. that way the new call site reads as _is_user_descriptor(base.__dict__[name], exclude_properties=False), which is interpreted as "do not exclude properties when determining if a value is a user descriptor". it seems like the more natural expression of what is being requested.

also generally speaking, i prefer to add optional kwargs with behavior-preserving defaults in the affirmative unless there's a clear reason to deviate.