diff --git a/packages/reflex-base/news/7015.bugfix.md b/packages/reflex-base/news/7015.bugfix.md new file mode 100644 index 00000000000..4d1ce3f5c94 --- /dev/null +++ b/packages/reflex-base/news/7015.bugfix.md @@ -0,0 +1,5 @@ +Fix hooks and imports being silently dropped from the compiled output when two +vars with the same value but different metadata were interpolated into the same +f-string. Var hashing is now derived from the same identity `Var.equals` uses, +which also fixes `Var.equals` raising `VarTypeError` for vars that carry +dependencies, and makes `NumberVar` and `BooleanVar` hashable. diff --git a/packages/reflex-base/src/reflex_base/vars/base.py b/packages/reflex-base/src/reflex_base/vars/base.py index 8803960d51c..7ce146c80c1 100644 --- a/packages/reflex-base/src/reflex_base/vars/base.py +++ b/packages/reflex-base/src/reflex_base/vars/base.py @@ -380,7 +380,13 @@ def merge(*all: VarData | None) -> VarData | None: *(var_data.imports for var_data in all_var_datas) ) - deps = [dep for var_data in all_var_datas for dep in var_data.deps] + deps = list( + { + dep._hash_key(): dep + for var_data in all_var_datas + for dep in var_data.deps + }.values() + ) positions = list( dict.fromkeys( @@ -435,16 +441,20 @@ def __bool__(self) -> bool: or self.app_wraps ) + @functools.cached_property def _identity_key(self) -> tuple: - """Return a hashable key for ``__eq__`` and ``__hash__``. + """A hashable key for ``__eq__`` and ``__hash__``. - ``components`` and ``app_wraps`` hold ``BaseComponent`` instances whose - ``__eq__`` override drops the default hash. Use component identity for - embedded components because they can contribute hooks/imports, and use - the compiler's app-wrap registry key for wrappers so fresh provider - instances with the same role still compare equal. App wraps are a set - of required roles, so a ``frozenset`` keeps identity insensitive to the - order vars happened to merge in (``a + b`` and ``b + a`` stay equal). + ``deps`` holds Vars, which a container can never compare because + ``Var.__eq__`` builds a ``BooleanVar``, so they are reduced to their + ``_hash_key()``. ``components`` and ``app_wraps`` hold ``BaseComponent`` + instances whose ``__eq__`` override drops the default hash. Use + component identity for embedded components because they can contribute + hooks/imports, and use the compiler's app-wrap registry key for + wrappers so fresh provider instances with the same role still compare + equal. App wraps are a set of required roles, so a ``frozenset`` keeps + identity insensitive to the order vars happened to merge in + (``a + b`` and ``b + a`` stay equal). Returns: A hashable tuple uniquely identifying this VarData. @@ -454,7 +464,7 @@ def _identity_key(self) -> tuple: self.field_name, self.imports, self.hooks, - self.deps, + tuple(dep._hash_key() for dep in self.deps), self.position, tuple(id(component) for component in self.components), frozenset( @@ -474,7 +484,7 @@ def __eq__(self, other: object) -> bool: """ if not isinstance(other, VarData): return NotImplemented - return self._identity_key() == other._identity_key() + return self._identity_key == other._identity_key def __hash__(self) -> int: """Hash consistent with ``__eq__``. @@ -482,7 +492,16 @@ def __hash__(self) -> int: Returns: A hash over render-time fields and hashable component metadata. """ - return hash(self._identity_key()) + return self._cached_hash + + @functools.cached_property + def _cached_hash(self) -> int: + """The hash, cached because every Var interpolation recomputes it. + + Returns: + A hash over render-time fields and hashable component metadata. + """ + return hash(self._identity_key) @classmethod def from_state(cls, state: type[BaseState] | str, field_name: str = "") -> VarData: @@ -704,13 +723,28 @@ def __post_init__(self): _var_data=VarData.merge(self._var_data, var_data_), ) + def _hash_key(self) -> tuple[Any, ...]: + """Return the canonical identity of this var. + + ``__eq__`` builds a ``BooleanVar`` rather than returning a bool, and + bool-ifying a Var raises, so a Var can never be compared by a container. + Every structural comparison goes through this key instead, which holds + no Var objects and is therefore safe to nest in tuples, dicts and sets. + Subclasses that need a different identity override this rather than + ``__hash__``, so hashing and ``equals`` can never drift apart. + + Returns: + A hashable tuple uniquely identifying this var. + """ + return (self._js_expr, self._var_type, self._get_all_var_data()) + def __hash__(self) -> int: """Define a hash function for the var. Returns: The hash of the var. """ - return hash((self._js_expr, self._var_type, self._var_data)) + return hash(self._hash_key()) def _get_all_var_data(self) -> VarData | None: """Get all VarData associated with the Var. @@ -740,11 +774,7 @@ def equals(self, other: Var) -> builtins.bool: Returns: Whether the vars are equal. """ - return ( - self._js_expr == other._js_expr - and self._var_type == other._var_type - and self._get_all_var_data() == other._get_all_var_data() - ) + return self._hash_key() == other._hash_key() @overload def _replace( @@ -1553,13 +1583,17 @@ def __post_init__(self): """Post initialization.""" object.__delattr__(self, "_js_expr") - def __hash__(self) -> int: - """Calculate the hash value of the object. + def _hash_key(self) -> tuple[Any, ...]: + """Return the canonical identity of this var. + + Identical to ``Var._hash_key``, but reads the expression straight off + ``_original``: ``__post_init__`` deletes ``_js_expr``, so the inherited + version would pay a ``__getattr__`` round trip on every hash. Returns: - int: The hash value of the object. + A hashable tuple uniquely identifying this var. """ - return hash(self._original) + return (self._original._js_expr, self._var_type, self._get_all_var_data()) def _get_all_var_data(self) -> VarData | None: """Get all the var data. @@ -2144,21 +2178,6 @@ def _cached_get_all_var_data(self: VarProtocol) -> VarData | None: self._var_data, ) - def __hash__(self: DataclassInstance) -> int: - """Calculate the hash of the object. - - Returns: - The hash of the object. - """ - return hash(( - type(self).__name__, - *[ - getattr(self, field.name) - for field in dataclasses.fields(self) - if field.name not in ["_js_expr", "_var_data", "_var_type"] - ], - )) - _PY_AND_IMPORT: ImportDict = { f"$/{constants.Dirs.STATE_PATH}": [ImportVar(tag="pyAnd")], diff --git a/packages/reflex-base/src/reflex_base/vars/color.py b/packages/reflex-base/src/reflex_base/vars/color.py index 730586005f2..94e7ac75704 100644 --- a/packages/reflex-base/src/reflex_base/vars/color.py +++ b/packages/reflex-base/src/reflex_base/vars/color.py @@ -70,19 +70,6 @@ def create( _var_value=value, ) - def __hash__(self) -> int: - """Get the hash of the var. - - Returns: - The hash of the var. - """ - return hash(( - self.__class__.__name__, - self._var_value.color, - self._var_value.alpha, - self._var_value.shade, - )) - @cached_property_no_lock def _cached_var_name(self) -> str: """The name of the var. diff --git a/packages/reflex-base/src/reflex_base/vars/number.py b/packages/reflex-base/src/reflex_base/vars/number.py index 779c06b42cf..9a1de32d006 100644 --- a/packages/reflex-base/src/reflex_base/vars/number.py +++ b/packages/reflex-base/src/reflex_base/vars/number.py @@ -59,6 +59,12 @@ def raise_unsupported_operand_types( class NumberVar(Var[NUMBER_T], python_types=(int, float, decimal.Decimal)): """Base class for immutable number vars.""" + # Load-bearing: defining __eq__ below drops the inherited hash, which would + # leave every numeric var unhashable, including LiteralNumberVar. Var.__format__ + # hashes, so removing this breaks interpolating any numeric literal into a + # string. test_numeric_literal_var_hashability_is_load_bearing pins it. + __hash__ = Var.__hash__ + def __add__(self, other: number_types) -> NumberVar: """Add two numbers. @@ -964,14 +970,6 @@ def json(self) -> str: raise PrimitiveUnserializableToJSONError(msg) return json.dumps(self._var_value) - def __hash__(self) -> int: - """Calculate the hash value of the object. - - Returns: - int: The hash value of the object. - """ - return hash((type(self).__name__, self._var_value)) - @classmethod def _get_all_var_data_without_creating_var( cls, value: float | int | decimal.Decimal @@ -1032,14 +1030,6 @@ def json(self) -> str: """ return "true" if self._var_value else "false" - def __hash__(self) -> int: - """Calculate the hash value of the object. - - Returns: - int: The hash value of the object. - """ - return hash((type(self).__name__, self._var_value)) - @classmethod def _get_all_var_data_without_creating_var(cls, value: bool) -> VarData | None: """Get all the var data without creating the var. diff --git a/packages/reflex-base/src/reflex_base/vars/object.py b/packages/reflex-base/src/reflex_base/vars/object.py index ab809eeb19f..3a8c87989ee 100644 --- a/packages/reflex-base/src/reflex_base/vars/object.py +++ b/packages/reflex-base/src/reflex_base/vars/object.py @@ -456,14 +456,6 @@ def json(self) -> str: keys_and_values.append(f"{key.json()}:{value.json()}") return "{" + ", ".join(keys_and_values) + "}" - def __hash__(self) -> int: - """Get the hash of the var. - - Returns: - The hash of the var. - """ - return hash((type(self).__name__, self._js_expr)) - @classmethod def _get_all_var_data_without_creating_var( cls, diff --git a/packages/reflex-base/src/reflex_base/vars/sequence.py b/packages/reflex-base/src/reflex_base/vars/sequence.py index 0b621e25fb8..6a19c6b7568 100644 --- a/packages/reflex-base/src/reflex_base/vars/sequence.py +++ b/packages/reflex-base/src/reflex_base/vars/sequence.py @@ -659,14 +659,6 @@ def _cached_get_all_var_data(self) -> VarData | None: self._var_data, ) - def __hash__(self) -> int: - """Get the hash of the var. - - Returns: - The hash of the var. - """ - return hash((self.__class__.__name__, self._js_expr)) - def json(self) -> str: """Get the JSON representation of the var. @@ -1501,14 +1493,6 @@ def create( _var_value=value, ) - def __hash__(self) -> int: - """Get the hash of the var. - - Returns: - The hash of the var. - """ - return hash((type(self).__name__, self._var_value)) - def json(self) -> str: """Get the JSON representation of the var. @@ -2117,19 +2101,6 @@ def create( _var_value=value, ) - def __hash__(self) -> int: - """Get the hash of the var. - - Returns: - The hash of the var. - """ - return hash(( - self.__class__.__name__, - self._var_value.start, - self._var_value.stop, - self._var_value.step, - )) - @cached_property_no_lock def _cached_var_name(self) -> str: """The name of the var. diff --git a/tests/units/test_var.py b/tests/units/test_var.py index 942d7d78d31..29418ecb612 100644 --- a/tests/units/test_var.py +++ b/tests/units/test_var.py @@ -2308,3 +2308,130 @@ async def async_wrapper(self) -> str: rx.input(placeholder=ComputedVarTypeState.sync_wrapper) rx.input(placeholder=ComputedVarTypeState.async_plain) rx.input(placeholder=ComputedVarTypeState.async_wrapper) + + +def test_var_equals_with_var_data_deps(): + """Var.equals must not bool-ify contained dep Vars. + + VarData.deps holds Vars, and Var.__eq__ builds a BooleanVar instead of + returning a bool, so comparing deps element-wise used to raise VarTypeError. + """ + dep_a = Var(_js_expr="dep", _var_type=str) + dep_b = Var(_js_expr="dep", _var_type=str) + assert dep_a is not dep_b + + var_a = Var(_js_expr="foo", _var_type=str, _var_data=VarData(deps=[dep_a])) + var_b = Var(_js_expr="foo", _var_type=str, _var_data=VarData(deps=[dep_b])) + + assert var_a.equals(var_b) + assert VarData(deps=[dep_a]) == VarData(deps=[dep_b]) + assert VarData(deps=[dep_a]) != VarData(deps=[Var(_js_expr="other", _var_type=str)]) + + +def test_var_data_merge_dedupes_deps(): + """Merging VarData collapses deps that refer to the same Var.""" + dep_a = Var(_js_expr="dep", _var_type=str) + dep_b = Var(_js_expr="dep", _var_type=str) + other = Var(_js_expr="other", _var_type=str) + + merged = VarData.merge(VarData(deps=[dep_a, other]), VarData(deps=[dep_b])) + assert merged is not None + assert [str(dep) for dep in merged.deps] == ["dep", "other"] + + +@pytest.mark.parametrize( + "value", + ["hi", 5, True, [1, 2], {"a": 1}], +) +def test_literal_var_hash_distinguishes_var_data(value): + """Literal vars with equal values but different VarData must not share a hash. + + Var.__format__ registers the var in _global_vars under hash(self), so a + collision silently drops one var's hooks/imports from the decoded output. + """ + var_a = LiteralVar.create(value)._replace( + merge_var_data=VarData(hooks="const A = 1") + ) + var_b = LiteralVar.create(value)._replace( + merge_var_data=VarData(hooks="const B = 2") + ) + + assert not var_a.equals(var_b) + assert hash(var_a) != hash(var_b) + + var_data, _ = _decode_var_immutable(f"{var_a}{var_b}") + assert var_data is not None + assert set(var_data.hooks) == {"const A = 1", "const B = 2"} + + +def test_to_operation_hash_differs_from_original(): + """A `.to()` view of a var is not equal to it, so it must not collide.""" + original = Var(_js_expr="x", _var_type=str) + converted = original.to(int) + + assert not converted.equals(original) + assert hash(converted) != hash(original) + + +@pytest.mark.parametrize( + "make_var", + [ + lambda: Var(_js_expr="x", _var_type=str), + lambda: LiteralVar.create("hi"), + lambda: LiteralVar.create(5), + lambda: LiteralVar.create([1, 2]), + lambda: LiteralVar.create({"a": 1}), + lambda: Var(_js_expr="x", _var_type=str).to(int), + lambda: LiteralVar.create("hi").upper(), + ], + ids=["plain", "string", "number", "array", "object", "to_op", "operation"], +) +def test_var_hash_consistent_with_equals(make_var): + """Structurally equal vars hash equal, and hashing never bool-ifies a Var.""" + var_a, var_b = make_var(), make_var() + assert var_a is not var_b + assert var_a.equals(var_b) + assert hash(var_a) == hash(var_b) + + +def test_var_hash_key_contains_no_vars(): + """The identity key must be Var-free so containers never call Var.__eq__.""" + + def walk(value): + assert not isinstance(value, Var), f"Var leaked into hash key: {value!r}" + if isinstance(value, (tuple, list, frozenset, set)): + for item in value: + walk(item) + elif isinstance(value, VarData): + walk(value._identity_key) + + dep = Var(_js_expr="dep", _var_type=str) + var = LiteralVar.create("hi")._replace(merge_var_data=VarData(deps=[dep])) + walk(var._hash_key()) + + +def test_number_and_boolean_vars_are_hashable(): + """Defining __eq__ must not leave NumberVar/BooleanVar unhashable.""" + from reflex_base.vars.number import BooleanVar + + assert NumberVar.__hash__ is not None + assert BooleanVar.__hash__ is not None + + +@pytest.mark.parametrize("value", [5, 5.5, True]) +def test_numeric_literal_var_hashability_is_load_bearing(value): + """Numeric literals must stay hashable so they survive interpolation. + + NumberVar defines __eq__, which drops the inherited __hash__ unless it is + restored explicitly. Var.__format__ hashes the var to register it in + _global_vars, so an unhashable numeric literal raises TypeError on any + f-string interpolation rather than failing anywhere near the cause. + """ + var = LiteralVar.create(value) + assert type(var).__hash__ is not None + + var_data, _ = _decode_var_immutable( + f"{var._replace(merge_var_data=VarData(hooks='const A = 1'))}" + ) + assert var_data is not None + assert var_data.hooks == ("const A = 1",)