From c2ea4dd2e678668912e693ace8a7c00e5383e34e Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Mon, 12 Oct 2026 13:43:39 -0300 Subject: [PATCH 1/6] fix: reject callback names colliding between nested state bodies Callbacks declared inside a `State.Compound` or `State.Parallel` body are flattened into the class namespace keyed by name only, so the same name in two nested bodies, or in a nested body and the class body, kept only the last one and silently dropped the other. The metaclass now raises `InvalidDefinition` naming the callback and both owners. The same function object assigned under one name in several bodies is still accepted. Namespacing each nested body is tracked by #657. Fixes #653 Signed-off-by: Fernando Macedo --- docs/releases/3.2.2.md | 46 ++++++++ docs/states.md | 5 + docs/validations.md | 36 ++++++ statemachine/factory.py | 36 ++++++ tests/test_statechart_compound.py | 186 ++++++++++++++++++++++++++++++ 5 files changed, 309 insertions(+) diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index 1a637a01..bab2b4de 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -76,6 +76,52 @@ Reported by [@Dolecor](https://github.com/Dolecor). [#643](https://github.com/fgmacedo/python-statemachine/issues/643). +### Callback names colliding between nested state bodies + +Callbacks declared inside a `State.Compound` or `State.Parallel` body are attached to the +statechart class under their own names, with no reference to the owning state. Two sibling +regions declaring the same callback name collided, and the last body silently won: with a +`@start.to(end) def go` in each region of a parallel state, `send("go")` moved both regions but +ran only the second body. The same happened to a plain callback such as `on_enter_` +declared in two bodies, and to a nested body overriding a method of the class body. + +A name declared in two nested bodies, or in a nested body and the class body, is now rejected +at class creation: + +```py +>>> from statemachine import State, StateChart +>>> from statemachine.exceptions import InvalidDefinition + +>>> try: +... class War(StateChart): +... class war(State.Parallel): +... class quest(State.Compound): +... start = State(initial=True) +... end = State(final=True) +... +... @start.to(end) +... def go(self): +... print("quest") +... +... class battle(State.Compound): +... fighting = State(initial=True) +... won = State(final=True) +... +... @fighting.to(won) +... def go(self): +... print("battle") +... except InvalidDefinition as e: +... print(e) +Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share one namespace, so only the last one would run. Rename one of them. + +``` + +Assigning the same function object under one name in several bodies is still allowed. Giving +each nested body its own namespace, so that the same name can be reused across regions, is +tracked by [#657](https://github.com/fgmacedo/python-statemachine/issues/657). + +[#653](https://github.com/fgmacedo/python-statemachine/issues/653). + ### Events due at the same moment kept their order only by chance Both event queues are priority queues ordered by each trigger's due time, and triggers due at diff --git a/docs/states.md b/docs/states.md index dba8633c..6545fc98 100644 --- a/docs/states.md +++ b/docs/states.md @@ -162,6 +162,11 @@ True ``` +Regions run independently, but their class bodies do not form separate namespaces: every +callback declared inside a nested body is attached to the statechart class under its own name. +Declaring the same callback name in two nested bodies, or in a nested body and the class body, +raises `InvalidDefinition` (see {ref}`callback-name-collision`). + ```{seealso} See {ref}`done-state-events` for how `done.state` events work with parallel states (all regions must reach a final state). diff --git a/docs/validations.md b/docs/validations.md index 7bac5402..255f3c7c 100644 --- a/docs/validations.md +++ b/docs/validations.md @@ -183,6 +183,41 @@ The `donedata` parameter can only be used on states marked as `final=True`: ``` +(callback-name-collision)= + +### Callback names across nested state bodies + +Callbacks declared inside a `State.Compound` or `State.Parallel` body are attached to the +statechart class under their own names, so nested bodies share one namespace with each other +and with the class body. The same name declared twice would keep only the last body: + +```py +>>> try: +... class Bad(StateChart): +... class war(State.Parallel): +... class quest(State.Compound): +... start = State(initial=True) +... end = State(final=True) +... +... @start.to(end) +... def go(self): +... print("quest") +... +... class battle(State.Compound): +... fighting = State(initial=True) +... won = State(final=True) +... +... @fighting.to(won) +... def go(self): +... print("battle") +... except InvalidDefinition as e: +... print(e) +Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share one namespace, so only the last one would run. Rename one of them. + +``` + +The same function object assigned under one name in several bodies is not a collision. + ### Invalid listener entries Entries in the `listeners` class attribute must be classes, callables, or @@ -280,6 +315,7 @@ Expressions support `and`, `or`, `not`, and parentheses. See | Initial transitions have no cond | Class definition| No | | `donedata` on final states only | Class definition| No | | Invalid listener entries | Class definition| No | +| Callback names across nested bodies | Class definition| No | | Callback resolution | Instance creation | No | | Boolean expression parsing | Instance creation | No | diff --git a/statemachine/factory.py b/statemachine/factory.py index 33f66e28..a6471dd0 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -165,13 +165,49 @@ def _initials_by_document_order( # noqa: C901 def _unpack_builders_callbacks(cls): callbacks = {} + owners: dict[str, State] = {} for state in iterate_states(cls.states): if state._callbacks: + for attr_name, func in state._callbacks.items(): + cls._check_callback_collision(attr_name, func, state, callbacks, owners) + owners[attr_name] = state callbacks.update(state._callbacks) del state._callbacks for key, value in callbacks.items(): setattr(cls, key, value) + def _check_callback_collision( + cls, + attr_name: str, + func: Any, + state: State, + callbacks: dict[str, Any], + owners: dict[str, State], + ): + """Nested state bodies are flattened into the class namespace, so a name declared + in two bodies (or in a body and the class body) keeps only the last one.""" + if attr_name in callbacks: + existing = callbacks[attr_name] + declared_in = _("the '{}' and the '{}' state bodies").format( + owners[attr_name].id, state.id + ) + elif attr_name in cls.__dict__: + existing = cls.__dict__[attr_name] + declared_in = _("the '{}' state body and the '{}' class body").format( + state.id, cls.__name__ + ) + else: + return + if existing is func: + return + name = func.__name__ if getattr(func, "is_event", False) else attr_name + raise InvalidDefinition( + _( + "Callback '{}' is declared in both {}. Nested state bodies share one " + "namespace, so only the last one would run. Rename one of them." + ).format(name, declared_in) + ) + def _check(cls): has_states = bool(cls.states) cls._abstract = not has_states diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index 61dafc01..997aec0f 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -11,6 +11,7 @@ from enum import auto import pytest +from statemachine.exceptions import InvalidDefinition from statemachine.states import States from statemachine import Event @@ -495,3 +496,188 @@ def clink(self): sm = await sm_runner.start(Gate) await sm_runner.send(sm, "coin") assert log == ["clink"] + + +class TestCallbackNameCollision: + """Nested state bodies share one namespace: the same callback name in two bodies is an + error instead of a silently discarded body (#653).""" + + def test_decorator_event_in_sibling_regions(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + + @start.to(end) + def go(self): + return "quest" + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + + @fighting.to(won) + def go(self): + return "battle" + + assert str(exc_info.value) == ( + "Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. " + "Nested state bodies share one namespace, so only the last one would run. " + "Rename one of them." + ) + + def test_plain_callback_in_sibling_regions(self): + with pytest.raises(InvalidDefinition, match="Callback 'on_enter_end' is declared in both"): + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + def on_enter_end(self): + return "quest" + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + victory = fighting.to(won) + + def on_enter_end(self): + return "battle" + + def test_callback_in_parent_and_child_bodies(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class Journey(StateChart): + class moria(State.Compound): + class halls(State.Compound, initial=True): + entrance = State(initial=True) + bridge = State(final=True) + cross = entrance.to(bridge) + + def on_enter_bridge(self): + return "halls" + + daylight = State(final=True) + escape = halls.to(daylight) + + def on_enter_bridge(self): + return "moria" + + assert "'moria' and the 'halls' state bodies" in str(exc_info.value) + + def test_callback_in_state_body_and_class_body(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon) + + def on_enter_green_dragon(self): + return "shire" + + def on_enter_green_dragon(self): + return "class" + + assert str(exc_info.value) == ( + "Callback 'on_enter_green_dragon' is declared in both the 'shire' state body and " + "the 'Journey' class body. Nested state bodies share one namespace, so only the " + "last one would run. Rename one of them." + ) + + def test_decorator_event_in_state_body_and_class_body(self): + with pytest.raises(InvalidDefinition, match="Callback 'go' is declared in both"): + + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State() + + @bag_end.to(green_dragon) + def go(self): + return "shire" + + road = State(final=True) + + @shire.to(road) + def go(self): + return "class" + + async def test_same_function_in_sibling_regions_is_not_a_collision(self, sm_runner): + log = [] + + def arrive(self): + log.append("arrived") + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + on_enter_end = arrive + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + victory = fighting.to(won) + + on_enter_end = arrive + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert log == ["arrived"] + + async def test_same_function_in_state_body_and_class_body_is_not_a_collision(self, sm_runner): + log = [] + + def arrive(self): + log.append("arrived") + + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon) + + on_enter_green_dragon = arrive + + on_enter_green_dragon = arrive + + sm = await sm_runner.start(Journey) + await sm_runner.send(sm, "visit_pub") + assert log == ["arrived"] + + async def test_distinct_names_in_sibling_regions_both_run(self, sm_runner): + log = [] + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + + @start.to(end) + def go(self): + log.append("quest") + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + + @fighting.to(won) + def victory(self): + log.append("battle") + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + await sm_runner.send(sm, "victory") + assert log == ["quest", "battle"] From 4c6e341745385c683396418a0e5009b0c974290b Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Mon, 12 Oct 2026 13:53:11 -0300 Subject: [PATCH 2/6] fix: accept equal callables and name the clashing event in the collision check A bound method assigned in two nested bodies is a fresh object on each access, so identity rejected it although both bodies attach one callable. Compare by equality instead. A name that an event registered from a sibling body, or another attribute of the class, already uses was reported as a class-body declaration; name the event or the attribute instead. Signed-off-by: Fernando Macedo --- docs/releases/3.2.2.md | 4 +- docs/validations.md | 7 +-- statemachine/factory.py | 42 +++++++++++------ tests/test_statechart_compound.py | 77 +++++++++++++++++++++++++++++-- 4 files changed, 108 insertions(+), 22 deletions(-) diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index bab2b4de..14c54388 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -112,11 +112,11 @@ at class creation: ... print("battle") ... except InvalidDefinition as e: ... print(e) -Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share one namespace, so only the last one would run. Rename one of them. +Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. ``` -Assigning the same function object under one name in several bodies is still allowed. Giving +Assigning the same callable under one name in several bodies is still allowed. Giving each nested body its own namespace, so that the same name can be reused across regions, is tracked by [#657](https://github.com/fgmacedo/python-statemachine/issues/657). diff --git a/docs/validations.md b/docs/validations.md index 255f3c7c..e2197752 100644 --- a/docs/validations.md +++ b/docs/validations.md @@ -212,11 +212,12 @@ and with the class body. The same name declared twice would keep only the last b ... print("battle") ... except InvalidDefinition as e: ... print(e) -Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share one namespace, so only the last one would run. Rename one of them. +Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. ``` -The same function object assigned under one name in several bodies is not a collision. +The same callable assigned under one name in several bodies is not a collision. A name +that an event or another attribute of the class already uses is also rejected. ### Invalid listener entries @@ -315,7 +316,7 @@ Expressions support `and`, `or`, `not`, and parentheses. See | Initial transitions have no cond | Class definition| No | | `donedata` on final states only | Class definition| No | | Invalid listener entries | Class definition| No | -| Callback names across nested bodies | Class definition| No | +| Callback names in nested bodies | Class definition| No | | Callback resolution | Instance creation | No | | Boolean expression parsing | Instance creation | No | diff --git a/statemachine/factory.py b/statemachine/factory.py index a6471dd0..41841f15 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -59,7 +59,7 @@ def __init__( cls.add_inherited(bases) cls.add_from_attributes(attrs) cls._collect_class_listeners(attrs, bases) - cls._unpack_builders_callbacks() + cls._unpack_builders_callbacks(attrs) cls._update_event_references() if not cls.states: @@ -163,13 +163,19 @@ def _initials_by_document_order( # noqa: C901 if not any(t for t in parent.transitions if t.initial and t.target == state): parent.to(state, initial=True) # pragma: no cover - def _unpack_builders_callbacks(cls): - callbacks = {} + def _unpack_builders_callbacks(cls, attrs: dict[str, Any]): + callbacks: dict[str, Any] = {} owners: dict[str, State] = {} + class_body_names = set(attrs) + class_body_names.update( + value.attr_name for value in attrs.values() if getattr(value, "attr_name", None) + ) for state in iterate_states(cls.states): if state._callbacks: for attr_name, func in state._callbacks.items(): - cls._check_callback_collision(attr_name, func, state, callbacks, owners) + cls._check_callback_collision( + attr_name, func, state, callbacks, owners, class_body_names + ) owners[attr_name] = state callbacks.update(state._callbacks) del state._callbacks @@ -183,29 +189,39 @@ def _check_callback_collision( state: State, callbacks: dict[str, Any], owners: dict[str, State], + class_body_names: set[str], ): """Nested state bodies are flattened into the class namespace, so a name declared - in two bodies (or in a body and the class body) keeps only the last one.""" + in two bodies, or in a body and the class body, keeps only the last one.""" if attr_name in callbacks: existing = callbacks[attr_name] - declared_in = _("the '{}' and the '{}' state bodies").format( + conflict = _("is declared in both the '{}' and the '{}' state bodies").format( owners[attr_name].id, state.id ) elif attr_name in cls.__dict__: existing = cls.__dict__[attr_name] - declared_in = _("the '{}' state body and the '{}' class body").format( - state.id, cls.__name__ - ) + if attr_name in class_body_names: + conflict = _( + "is declared in both the '{}' state body and the '{}' class body" + ).format(state.id, cls.__name__) + elif isinstance(existing, Event): + conflict = _("in the '{}' state body clashes with the event '{}'").format( + state.id, existing.id + ) + else: + conflict = _( + "in the '{}' state body clashes with the '{}' attribute of '{}'" + ).format(state.id, attr_name, cls.__name__) else: return - if existing is func: + if existing == func: return name = func.__name__ if getattr(func, "is_event", False) else attr_name raise InvalidDefinition( _( - "Callback '{}' is declared in both {}. Nested state bodies share one " - "namespace, so only the last one would run. Rename one of them." - ).format(name, declared_in) + "Callback '{}' {}. Nested state bodies share the class namespace. " + "Rename one of them." + ).format(name, conflict) ) def _check(cls): diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index 997aec0f..c370bc70 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -525,8 +525,7 @@ def go(self): assert str(exc_info.value) == ( "Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. " - "Nested state bodies share one namespace, so only the last one would run. " - "Rename one of them." + "Nested state bodies share the class namespace. Rename one of them." ) def test_plain_callback_in_sibling_regions(self): @@ -588,8 +587,8 @@ def on_enter_green_dragon(self): assert str(exc_info.value) == ( "Callback 'on_enter_green_dragon' is declared in both the 'shire' state body and " - "the 'Journey' class body. Nested state bodies share one namespace, so only the " - "last one would run. Rename one of them." + "the 'Journey' class body. Nested state bodies share the class namespace. " + "Rename one of them." ) def test_decorator_event_in_state_body_and_class_body(self): @@ -656,6 +655,76 @@ class shire(State.Compound): await sm_runner.send(sm, "visit_pub") assert log == ["arrived"] + async def test_same_bound_method_in_sibling_regions_is_not_a_collision(self, sm_runner): + class Chronicle: + def __init__(self): + self.log = [] + + def arrive(self): + self.log.append("arrived") + + chronicle = Chronicle() + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + on_enter_end = chronicle.arrive + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + victory = fighting.to(won) + + on_enter_end = chronicle.arrive + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert chronicle.log == ["arrived"] + + def test_callback_named_as_an_event_of_a_sibling_region(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + step = start.to(end, cond="go") + + def go(self): + return True + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + assert str(exc_info.value) == ( + "Callback 'go' in the 'quest' state body clashes with the event 'go'. " + "Nested state bodies share the class namespace. Rename one of them." + ) + + def test_callback_named_as_a_class_attribute(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon, on="name") + + def name(self): + return "Frodo" + + assert str(exc_info.value) == ( + "Callback 'name' in the 'shire' state body clashes with the 'name' attribute of " + "'Journey'. Nested state bodies share the class namespace. Rename one of them." + ) + async def test_distinct_names_in_sibling_regions_both_run(self, sm_runner): log = [] From 5fd71c8a15079b0382ca1df390526b875f3cb407 Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Mon, 12 Oct 2026 14:03:44 -0300 Subject: [PATCH 3/6] fix: compare clashing callables only when they share a type `States.__eq__` iterates the other operand, so comparing a nested callback named `states` with the metaclass-provided attribute raised `TypeError` instead of `InvalidDefinition`. Signed-off-by: Fernando Macedo --- statemachine/factory.py | 2 +- tests/test_statechart_compound.py | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/statemachine/factory.py b/statemachine/factory.py index 41841f15..f9dae95b 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -214,7 +214,7 @@ def _check_callback_collision( ).format(state.id, attr_name, cls.__name__) else: return - if existing == func: + if type(existing) is type(func) and existing == func: return name = func.__name__ if getattr(func, "is_event", False) else attr_name raise InvalidDefinition( diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index c370bc70..cd71dc65 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -708,21 +708,27 @@ class battle(State.Compound): "Nested state bodies share the class namespace. Rename one of them." ) - def test_callback_named_as_a_class_attribute(self): + @pytest.mark.parametrize("attribute", ["name", "states"]) + def test_callback_named_as_a_class_attribute(self, attribute): + def callback(self): + return "Frodo" + + callback.__name__ = attribute + with pytest.raises(InvalidDefinition) as exc_info: class Journey(StateChart): class shire(State.Compound): bag_end = State(initial=True) green_dragon = State(final=True) - visit_pub = bag_end.to(green_dragon, on="name") + visit_pub = bag_end.to(green_dragon, on=attribute) - def name(self): - return "Frodo" + locals()[attribute] = callback assert str(exc_info.value) == ( - "Callback 'name' in the 'shire' state body clashes with the 'name' attribute of " - "'Journey'. Nested state bodies share the class namespace. Rename one of them." + f"Callback '{attribute}' in the 'shire' state body clashes with the " + f"'{attribute}' attribute of 'Journey'. Nested state bodies share the class " + "namespace. Rename one of them." ) async def test_distinct_names_in_sibling_regions_both_run(self, sm_runner): From 790cc178646e5ce63f3e47736d39c0949bf04ced Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Tue, 13 Oct 2026 10:18:44 -0300 Subject: [PATCH 4/6] fix: run object-referenced callbacks of every nested state body A callback attached by reference, through a decorator or by passing the function itself, keeps the function in its spec, so the owner is known. When its name is already taken, the metaclass stores it under a key unique to the owning state and the dispatcher resolves it through func.attr_name. Both sibling bodies now run instead of being rejected. Callbacks found by name (naming conventions and string references) still raise InvalidDefinition on a collision until #657 scopes names per body. Signed-off-by: Fernando Macedo --- docs/releases/3.2.2.md | 46 +++++-- docs/states.md | 9 +- docs/validations.md | 18 ++- statemachine/callbacks.py | 8 ++ statemachine/dispatcher.py | 5 +- statemachine/factory.py | 32 ++++- tests/test_statechart_compound.py | 206 +++++++++++++++++++++++++----- 7 files changed, 262 insertions(+), 62 deletions(-) diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index 14c54388..08240e91 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -85,39 +85,69 @@ regions declaring the same callback name collided, and the last body silently wo ran only the second body. The same happened to a plain callback such as `on_enter_` declared in two bodies, and to a nested body overriding a method of the class body. -A name declared in two nested bodies, or in a nested body and the class body, is now rejected -at class creation: +A callback attached by reference, through a decorator or by passing the function itself +(`cond=ready`), now runs in every body that declares it: ```py >>> from statemachine import State, StateChart >>> from statemachine.exceptions import InvalidDefinition +>>> class War(StateChart): +... class war(State.Parallel): +... class quest(State.Compound): +... start = State(initial=True) +... end = State(final=True) +... +... @start.to(end) +... def go(self): +... print("quest") +... +... class battle(State.Compound): +... fighting = State(initial=True) +... won = State(final=True) +... +... @fighting.to(won) +... def go(self): +... print("battle") + +>>> sm = War() +>>> _ = sm.send("go") +quest +battle + +``` + +A callback found by its name, through a naming convention or a string reference, has only its +name to tell the bodies apart. The same such name declared in two nested bodies, or in a nested +body and the class body, is now rejected at class creation: + +```py >>> try: ... class War(StateChart): ... class war(State.Parallel): ... class quest(State.Compound): ... start = State(initial=True) ... end = State(final=True) +... go = start.to(end) ... -... @start.to(end) -... def go(self): +... def on_enter_end(self): ... print("quest") ... ... class battle(State.Compound): ... fighting = State(initial=True) ... won = State(final=True) +... go = fighting.to(won) ... -... @fighting.to(won) -... def go(self): +... def on_enter_end(self): ... print("battle") ... except InvalidDefinition as e: ... print(e) -Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. +Callback 'on_enter_end' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. ``` Assigning the same callable under one name in several bodies is still allowed. Giving -each nested body its own namespace, so that the same name can be reused across regions, is +each nested body its own namespace for names, so that they can be reused across regions, is tracked by [#657](https://github.com/fgmacedo/python-statemachine/issues/657). [#653](https://github.com/fgmacedo/python-statemachine/issues/653). diff --git a/docs/states.md b/docs/states.md index 6545fc98..2e33ed99 100644 --- a/docs/states.md +++ b/docs/states.md @@ -162,10 +162,11 @@ True ``` -Regions run independently, but their class bodies do not form separate namespaces: every -callback declared inside a nested body is attached to the statechart class under its own name. -Declaring the same callback name in two nested bodies, or in a nested body and the class body, -raises `InvalidDefinition` (see {ref}`callback-name-collision`). +Regions run independently, but their class bodies do not form separate namespaces for +callbacks found by name. Declaring the same naming-convention callback or string-referenced +name in two nested bodies, or in a nested body and the class body, raises `InvalidDefinition` +(see {ref}`callback-name-collision`). Callbacks attached through a decorator run in their own +body whatever their name. ```{seealso} See {ref}`done-state-events` for how `done.state` events work with parallel diff --git a/docs/validations.md b/docs/validations.md index e2197752..5f9a1810 100644 --- a/docs/validations.md +++ b/docs/validations.md @@ -189,7 +189,9 @@ The `donedata` parameter can only be used on states marked as `final=True`: Callbacks declared inside a `State.Compound` or `State.Parallel` body are attached to the statechart class under their own names, so nested bodies share one namespace with each other -and with the class body. The same name declared twice would keep only the last body: +and with the class body. A callback found by its name (a naming convention such as +`on_enter_`, or a string reference such as `cond="ready"`) declared twice would keep +only the last body: ```py >>> try: @@ -198,26 +200,28 @@ and with the class body. The same name declared twice would keep only the last b ... class quest(State.Compound): ... start = State(initial=True) ... end = State(final=True) +... go = start.to(end) ... -... @start.to(end) -... def go(self): +... def on_enter_end(self): ... print("quest") ... ... class battle(State.Compound): ... fighting = State(initial=True) ... won = State(final=True) +... go = fighting.to(won) ... -... @fighting.to(won) -... def go(self): +... def on_enter_end(self): ... print("battle") ... except InvalidDefinition as e: ... print(e) -Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. +Callback 'on_enter_end' is declared in both the 'quest' and the 'battle' state bodies. Nested state bodies share the class namespace. Rename one of them. ``` The same callable assigned under one name in several bodies is not a collision. A name -that an event or another attribute of the class already uses is also rejected. +that an event or another attribute of the class already uses is also rejected. Callbacks +attached by reference, through a decorator or by passing the function itself, never collide: +each one runs in the body that declares it. ### Invalid listener entries diff --git a/statemachine/callbacks.py b/statemachine/callbacks.py index e7c6da1a..f362771d 100644 --- a/statemachine/callbacks.py +++ b/statemachine/callbacks.py @@ -8,6 +8,8 @@ from enum import auto from functools import partial from inspect import isawaitable +from inspect import isfunction +from typing import Any from .exceptions import AttrNotFound from .i18n import _ @@ -18,6 +20,12 @@ def allways_true(*args, **kwargs): return True +def is_unbounded_reference(func: Any) -> bool: + """Whether ``func`` is a function a `CallbackSpec` holds by object, to be bound to the + machine through the attribute named by ``func.attr_name``.""" + return isfunction(func) and hasattr(func, "attr_name") + + class CallbackPriority(IntEnum): GENERIC = 0 INLINE = 10 diff --git a/statemachine/dispatcher.py b/statemachine/dispatcher.py index 8f9bc369..a7ce7a2e 100644 --- a/statemachine/dispatcher.py +++ b/statemachine/dispatcher.py @@ -161,14 +161,15 @@ def _search_callable(self, spec): # if the attr is an unbounded method, we'll try to find the bounded method # on the self if not spec.is_bounded: + attr_name = spec.func.attr_name for listener in self.items: - func = getattr(listener.obj, spec.attr_name, None) + func = getattr(listener.obj, attr_name, None) # ``getattr`` may return a non-method that happens to share the name # (e.g. a model attribute named like a compiled guard); it is not the # unbounded method we are rebinding, so skip it instead of accessing # ``__func__`` (which would raise on a plain value). if getattr(func, "__func__", None) is spec.func: - yield listener.build_key(spec.attr_name), partial(callable_method, func) + yield listener.build_key(attr_name), partial(callable_method, func) return yield f"{spec.attr_name}-{id(spec.func)}@None", partial(callable_method, spec.func) diff --git a/statemachine/factory.py b/statemachine/factory.py index f9dae95b..d11f89bd 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -5,6 +5,7 @@ from .callbacks import CallbackGroup from .callbacks import CallbackPriority from .callbacks import CallbackSpecList +from .callbacks import is_unbounded_reference from .event import Event from .event import _expand_event_id from .exceptions import InvalidDefinition @@ -170,18 +171,37 @@ def _unpack_builders_callbacks(cls, attrs: dict[str, Any]): class_body_names.update( value.attr_name for value in attrs.values() if getattr(value, "attr_name", None) ) + declared: list[tuple[State, str, Any]] = [] for state in iterate_states(cls.states): if state._callbacks: - for attr_name, func in state._callbacks.items(): - cls._check_callback_collision( - attr_name, func, state, callbacks, owners, class_body_names - ) - owners[attr_name] = state - callbacks.update(state._callbacks) + declared.extend((state, name, func) for name, func in state._callbacks.items()) del state._callbacks + + for state, attr_name, func in declared: + if is_unbounded_reference(func): + continue + cls._check_callback_collision( + attr_name, func, state, callbacks, owners, class_body_names + ) + owners[attr_name] = state + callbacks[attr_name] = func for key, value in callbacks.items(): setattr(cls, key, value) + for state, attr_name, func in declared: + if is_unbounded_reference(func): + cls._store_unbounded_reference(attr_name, func, state) + + def _store_unbounded_reference(cls, attr_name: str, func: Any, state: State): + """Callbacks are resolved through ``func.attr_name``, so a name already taken moves + the function to a key unique to its owning state instead of replacing the other.""" + existing = cls.__dict__.get(attr_name) + if existing is None: + setattr(cls, attr_name, func) + elif existing is not func: + func.attr_name = f"{state.id}:{attr_name}" + setattr(cls, func.attr_name, func) + def _check_callback_collision( cls, attr_name: str, diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index cd71dc65..f9cf8718 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -499,34 +499,138 @@ def clink(self): class TestCallbackNameCollision: - """Nested state bodies share one namespace: the same callback name in two bodies is an - error instead of a silently discarded body (#653).""" + """A callback referenced by object runs in every nested body that declares it; a name + resolved by name in two bodies is an error instead of a silently discarded body (#653).""" - def test_decorator_event_in_sibling_regions(self): - with pytest.raises(InvalidDefinition) as exc_info: + async def test_decorator_event_in_sibling_regions(self, sm_runner): + log = [] - class War(StateChart): - class war(State.Parallel): - class quest(State.Compound): - start = State(initial=True) - end = State(final=True) + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) - @start.to(end) - def go(self): - return "quest" + @start.to(end) + def go(self): + log.append("quest") - class battle(State.Compound): - fighting = State(initial=True) - won = State(final=True) + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) - @fighting.to(won) - def go(self): - return "battle" + @fighting.to(won) + def go(self): + log.append("battle") - assert str(exc_info.value) == ( - "Callback 'go' is declared in both the 'quest' and the 'battle' state bodies. " - "Nested state bodies share the class namespace. Rename one of them." - ) + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert sorted(log) == ["battle", "quest"] + assert {"end", "won"} <= set(sm.configuration_values) + + async def test_decorated_action_in_sibling_regions(self, sm_runner): + log = [] + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + @start.exit + def leave(self): + log.append("quest") + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + @fighting.exit + def leave(self): + log.append("battle") + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert sorted(log) == ["battle", "quest"] + + async def test_guard_object_in_sibling_regions(self, sm_runner): + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + + def ready(self): + return True + + go = start.to(end, cond=ready) + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + + def ready(self): + return False + + go = fighting.to(won, cond=ready) + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert "end" in sm.configuration_values + assert "fighting" in sm.configuration_values + + async def test_decorated_callback_keeps_class_body_name(self, sm_runner): + log = [] + + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon) + + @bag_end.exit + def ready(self): + log.append("shire") + + road = State(final=True) + leave = shire.to(road, cond="ready") + + def ready(self): + log.append("class") + return True + + sm = await sm_runner.start(Journey) + await sm_runner.send(sm, "visit_pub") + await sm_runner.send(sm, "leave") + assert log == ["shire", "class"] + assert "road" in sm.configuration_values + + async def test_decorated_callback_named_as_a_sibling_event(self, sm_runner): + log = [] + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + step = start.to(end) + + @start.exit + def go(self): + log.append("quest") + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert "won" in sm.configuration_values + await sm_runner.send(sm, "step") + assert log == ["quest"] def test_plain_callback_in_sibling_regions(self): with pytest.raises(InvalidDefinition, match="Callback 'on_enter_end' is declared in both"): @@ -591,23 +695,29 @@ def on_enter_green_dragon(self): "Rename one of them." ) - def test_decorator_event_in_state_body_and_class_body(self): - with pytest.raises(InvalidDefinition, match="Callback 'go' is declared in both"): + async def test_decorator_event_in_state_body_and_class_body(self, sm_runner): + log = [] - class Journey(StateChart): - class shire(State.Compound): - bag_end = State(initial=True) - green_dragon = State() + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State() - @bag_end.to(green_dragon) - def go(self): - return "shire" + @bag_end.to(green_dragon) + def go(self): + log.append("shire") - road = State(final=True) + road = State(final=True) - @shire.to(road) - def go(self): - return "class" + @shire.to(road) + def go(self): + log.append("class") + + sm = await sm_runner.start(Journey) + await sm_runner.send(sm, "go") + assert log == ["shire"] + await sm_runner.send(sm, "go") + assert log == ["shire", "class"] async def test_same_function_in_sibling_regions_is_not_a_collision(self, sm_runner): log = [] @@ -655,6 +765,32 @@ class shire(State.Compound): await sm_runner.send(sm, "visit_pub") assert log == ["arrived"] + async def test_same_decorated_function_in_sibling_regions(self, sm_runner): + log = [] + + def arrive(self): + log.append("arrived") + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + on_arrive = end.enter(arrive) + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + on_arrive = won.enter(arrive) + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert log == ["arrived", "arrived"] + async def test_same_bound_method_in_sibling_regions_is_not_a_collision(self, sm_runner): class Chronicle: def __init__(self): From f122c6b1852a7607dfa159d44572e98852fc12fc Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Tue, 13 Oct 2026 10:35:28 -0300 Subject: [PATCH 5/6] fix: store a nested callback apart only when no lookup by name can reach it Holding a function by object does not make its name unreachable: a string reference, a naming convention or a self.() call still resolves through the class attribute, and silently picked another body's function. A shared name now moves to per-state keys only when every claimant is held by object, the class attribute it would shadow is an event or another such function, and no name-resolved spec of the machine mentions it. Otherwise it raises, also against inherited attributes. The relocated key lives on CallbackSpec.lookup_name instead of the shared func.attr_name, which any later spec over the same function overwrote. The check runs after _setup so convention specs are known. Signed-off-by: Fernando Macedo --- docs/releases/3.2.2.md | 10 +- docs/states.md | 9 +- docs/validations.md | 11 ++- statemachine/callbacks.py | 7 +- statemachine/dispatcher.py | 2 +- statemachine/factory.py | 144 ++++++++++++++++------------ tests/test_statechart_compound.py | 152 ++++++++++++++++++++++++++---- 7 files changed, 239 insertions(+), 96 deletions(-) diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index 08240e91..c8f6795f 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -85,8 +85,8 @@ regions declaring the same callback name collided, and the last body silently wo ran only the second body. The same happened to a plain callback such as `on_enter_` declared in two bodies, and to a nested body overriding a method of the class body. -A callback attached by reference, through a decorator or by passing the function itself -(`cond=ready`), now runs in every body that declares it: +A callback attached through a decorator, or by passing the function itself (`cond=ready`), now +runs in every body that declares it: ```py >>> from statemachine import State, StateChart @@ -117,9 +117,9 @@ battle ``` -A callback found by its name, through a naming convention or a string reference, has only its -name to tell the bodies apart. The same such name declared in two nested bodies, or in a nested -body and the class body, is now rejected at class creation: +A name reached by name, through a naming convention, a string reference or a plain method, +cannot tell the bodies apart. Such a name declared in two nested bodies, or in a nested body and +the class body or a base class, is now rejected at class creation: ```py >>> try: diff --git a/docs/states.md b/docs/states.md index 2e33ed99..0a01bc5e 100644 --- a/docs/states.md +++ b/docs/states.md @@ -162,11 +162,10 @@ True ``` -Regions run independently, but their class bodies do not form separate namespaces for -callbacks found by name. Declaring the same naming-convention callback or string-referenced -name in two nested bodies, or in a nested body and the class body, raises `InvalidDefinition` -(see {ref}`callback-name-collision`). Callbacks attached through a decorator run in their own -body whatever their name. +Regions run independently, but their class bodies share one namespace. Callbacks attached +through a decorator, or by passing the function itself, may reuse a name across bodies and each +runs in its own body. Any other reuse of a name, such as a naming-convention callback, a string +reference or a plain method, raises `InvalidDefinition` (see {ref}`callback-name-collision`). ```{seealso} See {ref}`done-state-events` for how `done.state` events work with parallel diff --git a/docs/validations.md b/docs/validations.md index 5f9a1810..18ca4060 100644 --- a/docs/validations.md +++ b/docs/validations.md @@ -189,9 +189,9 @@ The `donedata` parameter can only be used on states marked as `final=True`: Callbacks declared inside a `State.Compound` or `State.Parallel` body are attached to the statechart class under their own names, so nested bodies share one namespace with each other -and with the class body. A callback found by its name (a naming convention such as -`on_enter_`, or a string reference such as `cond="ready"`) declared twice would keep -only the last body: +and with the class body. A name reached by name (a naming convention such as +`on_enter_`, a string reference such as `cond="ready"`, or a plain method called as +`self.ready()`) declared twice would keep only the last body: ```py >>> try: @@ -220,8 +220,9 @@ Callback 'on_enter_end' is declared in both the 'quest' and the 'battle' state b The same callable assigned under one name in several bodies is not a collision. A name that an event or another attribute of the class already uses is also rejected. Callbacks -attached by reference, through a decorator or by passing the function itself, never collide: -each one runs in the body that declares it. +attached through a decorator, or by passing the function itself, may share a name: each runs in +the body that declares it, as long as no naming convention, string reference or plain method +uses that name. ### Invalid listener entries diff --git a/statemachine/callbacks.py b/statemachine/callbacks.py index f362771d..b564f217 100644 --- a/statemachine/callbacks.py +++ b/statemachine/callbacks.py @@ -21,8 +21,7 @@ def allways_true(*args, **kwargs): def is_unbounded_reference(func: Any) -> bool: - """Whether ``func`` is a function a `CallbackSpec` holds by object, to be bound to the - machine through the attribute named by ``func.attr_name``.""" + """Whether ``func`` is a function that a `CallbackSpec` holds by object.""" return isfunction(func) and hasattr(func, "attr_name") @@ -111,6 +110,10 @@ def __init__( and self.reference == SpecReference.NAME ) + self.lookup_name: str = self.attr_name + """Attribute of the machine holding an unbounded callable. Differs from `attr_name` + when nested state bodies share the name and each function is stored apart.""" + def __repr__(self): return f"{type(self).__name__}({self.func!r}, is_convention={self.is_convention!r})" diff --git a/statemachine/dispatcher.py b/statemachine/dispatcher.py index a7ce7a2e..2bf4073c 100644 --- a/statemachine/dispatcher.py +++ b/statemachine/dispatcher.py @@ -161,7 +161,7 @@ def _search_callable(self, spec): # if the attr is an unbounded method, we'll try to find the bounded method # on the self if not spec.is_bounded: - attr_name = spec.func.attr_name + attr_name = spec.lookup_name for listener in self.items: func = getattr(listener.obj, attr_name, None) # ``getattr`` may return a non-method that happens to share the name diff --git a/statemachine/factory.py b/statemachine/factory.py index d11f89bd..b1da74a5 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -5,6 +5,7 @@ from .callbacks import CallbackGroup from .callbacks import CallbackPriority from .callbacks import CallbackSpecList +from .callbacks import SpecReference from .callbacks import is_unbounded_reference from .event import Event from .event import _expand_event_id @@ -19,6 +20,20 @@ from .transition import Transition from .transition_list import TransitionList +_IDENTIFIER_RE = re.compile(r"[A-Za-z_]\w*") + + +def _same_callable(func: Any, other: Any) -> bool: + return type(func) is type(other) and func == other + + +def _can_store_apart(existing: Any, owners: list[tuple[State, Any]]) -> bool: + """Only callbacks held by object can move to a key of their own: a name is still + reachable through ``self.``, so it must not lose a plain method or attribute.""" + return all(is_unbounded_reference(func) for _, func in owners) and ( + existing is None or isinstance(existing, Event) or is_unbounded_reference(existing) + ) + class StateMachineMetaclass(type): "Metaclass for constructing StateMachine classes" @@ -60,7 +75,6 @@ def __init__( cls.add_inherited(bases) cls.add_from_attributes(attrs) cls._collect_class_listeners(attrs, bases) - cls._unpack_builders_callbacks(attrs) cls._update_event_references() if not cls.states: @@ -89,6 +103,7 @@ def __init__( cls._check() cls._setup() + cls._unpack_builders_callbacks(attrs) cls._expand_docstring() _STATECHART_RE = re.compile(r"\{statechart:(\w+)\}") @@ -165,77 +180,84 @@ def _initials_by_document_order( # noqa: C901 parent.to(state, initial=True) # pragma: no cover def _unpack_builders_callbacks(cls, attrs: dict[str, Any]): - callbacks: dict[str, Any] = {} - owners: dict[str, State] = {} - class_body_names = set(attrs) - class_body_names.update( - value.attr_name for value in attrs.values() if getattr(value, "attr_name", None) - ) - declared: list[tuple[State, str, Any]] = [] + names_resolved_by_name = cls._names_resolved_by_name() + relocated: dict[Any, str] = {} + for attr_name, owners in cls._nested_callbacks().items(): + existing = getattr(cls, attr_name, None) + owners = [(s, f) for s, f in owners if not _same_callable(f, existing)] + if not owners: + continue + if existing is None and len(owners) == 1: + setattr(cls, attr_name, owners[0][1]) + continue + if attr_name in names_resolved_by_name or not _can_store_apart(existing, owners): + cls._raise_callback_collision(attr_name, existing, owners, attrs) + for state, func in owners: + relocated[func] = f"{state.id}:{attr_name}" + setattr(cls, relocated[func], func) + cls._update_lookup_names(relocated) + + def _nested_callbacks(cls) -> dict[str, list[tuple[State, Any]]]: + """Distinct callables declared under each name, with the state body declaring them.""" + claims: dict[str, list[tuple[State, Any]]] = {} for state in iterate_states(cls.states): if state._callbacks: - declared.extend((state, name, func) for name, func in state._callbacks.items()) + for attr_name, func in state._callbacks.items(): + owners = claims.setdefault(attr_name, []) + if not any(_same_callable(func, other) for _, other in owners): + owners.append((state, func)) del state._callbacks + return claims - for state, attr_name, func in declared: - if is_unbounded_reference(func): - continue - cls._check_callback_collision( - attr_name, func, state, callbacks, owners, class_body_names - ) - owners[attr_name] = state - callbacks[attr_name] = func - for key, value in callbacks.items(): - setattr(cls, key, value) - - for state, attr_name, func in declared: - if is_unbounded_reference(func): - cls._store_unbounded_reference(attr_name, func, state) - - def _store_unbounded_reference(cls, attr_name: str, func: Any, state: State): - """Callbacks are resolved through ``func.attr_name``, so a name already taken moves - the function to a key unique to its owning state instead of replacing the other.""" - existing = cls.__dict__.get(attr_name) - if existing is None: - setattr(cls, attr_name, func) - elif existing is not func: - func.attr_name = f"{state.id}:{attr_name}" - setattr(cls, func.attr_name, func) + def _update_lookup_names(cls, relocated: dict[Any, str]): + if not relocated: + return + for specs in cls._all_specs(): + for spec in specs: + if spec.reference is SpecReference.CALLABLE and spec.func in relocated: + spec.lookup_name = relocated[spec.func] - def _check_callback_collision( + def _all_specs(cls): + yield cls._specs + for visited in iterate_states_and_transitions(cls.states): + yield visited._specs + + def _names_resolved_by_name(cls) -> set[str]: + names: set[str] = set() + for specs in cls._all_specs(): + for spec in specs: + if spec.reference is SpecReference.NAME: + names.update(_IDENTIFIER_RE.findall(spec.attr_name)) + return names + + def _raise_callback_collision( cls, attr_name: str, - func: Any, - state: State, - callbacks: dict[str, Any], - owners: dict[str, State], - class_body_names: set[str], + existing: Any, + owners: list[tuple[State, Any]], + attrs: dict[str, Any], ): - """Nested state bodies are flattened into the class namespace, so a name declared - in two bodies, or in a body and the class body, keeps only the last one.""" - if attr_name in callbacks: - existing = callbacks[attr_name] + class_body_names = set(attrs) + class_body_names.update( + value.attr_name for value in attrs.values() if getattr(value, "attr_name", None) + ) + state, func = owners[0] + if existing is None: conflict = _("is declared in both the '{}' and the '{}' state bodies").format( - owners[attr_name].id, state.id + state.id, owners[1][0].id + ) + elif attr_name in class_body_names: + conflict = _("is declared in both the '{}' state body and the '{}' class body").format( + state.id, cls.__name__ + ) + elif isinstance(existing, Event): + conflict = _("in the '{}' state body clashes with the event '{}'").format( + state.id, existing.id ) - elif attr_name in cls.__dict__: - existing = cls.__dict__[attr_name] - if attr_name in class_body_names: - conflict = _( - "is declared in both the '{}' state body and the '{}' class body" - ).format(state.id, cls.__name__) - elif isinstance(existing, Event): - conflict = _("in the '{}' state body clashes with the event '{}'").format( - state.id, existing.id - ) - else: - conflict = _( - "in the '{}' state body clashes with the '{}' attribute of '{}'" - ).format(state.id, attr_name, cls.__name__) else: - return - if type(existing) is type(func) and existing == func: - return + conflict = _("in the '{}' state body clashes with the '{}' attribute of '{}'").format( + state.id, attr_name, cls.__name__ + ) name = func.__name__ if getattr(func, "is_event", False) else attr_name raise InvalidDefinition( _( diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index f9cf8718..0ff372d4 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -581,31 +581,149 @@ def ready(self): assert "end" in sm.configuration_values assert "fighting" in sm.configuration_values - async def test_decorated_callback_keeps_class_body_name(self, sm_runner): - log = [] + def test_decorated_callback_named_as_a_class_body_method(self): + with pytest.raises(InvalidDefinition) as exc_info: - class Journey(StateChart): - class shire(State.Compound): - bag_end = State(initial=True) - green_dragon = State(final=True) - visit_pub = bag_end.to(green_dragon) + class Journey(StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon) + + @bag_end.exit + def ready(self): + return "shire" + + road = State(final=True) + leave = shire.to(road) - @bag_end.exit def ready(self): - log.append("shire") + return "class" - road = State(final=True) - leave = shire.to(road, cond="ready") + assert str(exc_info.value) == ( + "Callback 'ready' is declared in both the 'shire' state body and the 'Journey' " + "class body. Nested state bodies share the class namespace. Rename one of them." + ) + def test_decorated_callback_named_as_an_inherited_method(self): + class Guards: def ready(self): - log.append("class") return True - sm = await sm_runner.start(Journey) - await sm_runner.send(sm, "visit_pub") - await sm_runner.send(sm, "leave") - assert log == ["shire", "class"] - assert "road" in sm.configuration_values + with pytest.raises(InvalidDefinition, match="clashes with the 'ready' attribute"): + + class Journey(Guards, StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State() + road = State(final=True) + visit_pub = bag_end.to(green_dragon, cond="ready") + leave = green_dragon.to(road) + + @green_dragon.enter + def ready(self): + return False + + def test_decorated_callback_beside_a_plain_helper_of_the_same_name(self): + with pytest.raises(InvalidDefinition, match="Callback 'limit' is declared in both"): + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + + def limit(self): + return 10 + + go = start.to(end, cond=limit) + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + victory = fighting.to(won) + + def limit(self): + return 0 + + def test_decorated_callback_whose_name_is_used_by_a_string_reference(self): + with pytest.raises(InvalidDefinition, match="Callback 'ok' is declared in both"): + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end, cond="ok") + + @start.enter + def ok(self): + return True + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + victory = fighting.to(won) + + @fighting.enter + def ok(self): + return False + + def test_decorated_callbacks_named_by_a_naming_convention(self): + with pytest.raises(InvalidDefinition, match="Callback 'on_enter_end' is declared in both"): + + class War(StateChart): + class war(State.Parallel): + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + @won.enter + def on_enter_end(self): + return "battle" + + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + @end.enter + def on_enter_end(self): + return "quest" + + async def test_stored_apart_callback_survives_reuse_by_another_machine(self, sm_runner): + log = [] + + def arrive(self): + log.append("quest") + + class War(StateChart): + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end) + + on_arrive = end.enter(arrive) + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + @won.enter + def arrive(self): + log.append("battle") + + class Errand(StateChart): + home = State(initial=True) + market = State(final=True) + go = home.to(market, on=arrive) + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert sorted(log) == ["battle", "quest"] async def test_decorated_callback_named_as_a_sibling_event(self, sm_runner): log = [] From aec7d91f054ec08cb9c0ace3fe0a6d4c3e038443 Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Tue, 13 Oct 2026 10:58:27 -0300 Subject: [PATCH 6/6] fix: keep inherited overrides and base specs intact when storing callbacks apart - A nested body overriding a method inherited from a base class works again, as on 3.2.1; only the class's own attributes count as a collision. - Rewrite lookup_name only on specs of the states this class declares: since #673 a subclass shares the base's states, and relocating there broke the base. - Collect names from guard expressions through the AST, so string literals no longer block storing a callback apart. - Report a plain callback named like a class-body event as an event clash. Signed-off-by: Fernando Macedo --- docs/releases/3.2.2.md | 5 +- docs/validations.md | 10 +-- statemachine/factory.py | 29 ++++---- statemachine/spec_parser.py | 9 +++ tests/test_statechart_compound.py | 111 ++++++++++++++++++++++++++---- 5 files changed, 132 insertions(+), 32 deletions(-) diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index fa83d9b4..c468d339 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -119,7 +119,7 @@ battle A name reached by name, through a naming convention, a string reference or a plain method, cannot tell the bodies apart. Such a name declared in two nested bodies, or in a nested body and -the class body or a base class, is now rejected at class creation: +the class body, is now rejected at class creation: ```py >>> try: @@ -146,7 +146,8 @@ Callback 'on_enter_end' is declared in both the 'quest' and the 'battle' state b ``` -Assigning the same callable under one name in several bodies is still allowed. Giving +Assigning the same callable under one name in several bodies is still allowed, and so is +overriding a method inherited from a base class. Giving each nested body its own namespace for names, so that they can be reused across regions, is tracked by [#657](https://github.com/fgmacedo/python-statemachine/issues/657). diff --git a/docs/validations.md b/docs/validations.md index 18ca4060..cc1010dd 100644 --- a/docs/validations.md +++ b/docs/validations.md @@ -218,11 +218,11 @@ Callback 'on_enter_end' is declared in both the 'quest' and the 'battle' state b ``` -The same callable assigned under one name in several bodies is not a collision. A name -that an event or another attribute of the class already uses is also rejected. Callbacks -attached through a decorator, or by passing the function itself, may share a name: each runs in -the body that declares it, as long as no naming convention, string reference or plain method -uses that name. +The same callable assigned under one name in several bodies is not a collision, and a nested +body may override a method inherited from a base class. A plain method named like an event or +another attribute of the class is also rejected. Callbacks attached through a decorator, or by +passing the function itself, may share a name, even with an event: each runs in the body that +declares it, as long as no naming convention, string reference or plain method uses that name. ### Invalid listener entries diff --git a/statemachine/factory.py b/statemachine/factory.py index ccdb1fc4..606553ff 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -15,13 +15,12 @@ from .graph import iterate_states_and_transitions from .graph import states_without_path_to_final_states from .i18n import _ +from .spec_parser import names_in_boolean_expr from .state import State from .states import States from .transition import Transition from .transition_list import TransitionList -_IDENTIFIER_RE = re.compile(r"[A-Za-z_]\w*") - def _same_callable(func: Any, other: Any) -> bool: return type(func) is type(other) and func == other @@ -183,7 +182,7 @@ def _unpack_builders_callbacks(cls, attrs: dict[str, Any]): names_resolved_by_name = cls._names_resolved_by_name() relocated: dict[Any, str] = {} for attr_name, owners in cls._nested_callbacks().items(): - existing = getattr(cls, attr_name, None) + existing = cls.__dict__.get(attr_name) owners = [(s, f) for s, f in owners if not _same_callable(f, existing)] if not owners: continue @@ -195,7 +194,8 @@ def _unpack_builders_callbacks(cls, attrs: dict[str, Any]): for state, func in owners: relocated[func] = f"{state.id}:{attr_name}" setattr(cls, relocated[func], func) - cls._update_lookup_names(relocated) + own_states = [value for value in attrs.values() if isinstance(value, State)] + cls._update_lookup_names(relocated, own_states) def _nested_callbacks(cls) -> dict[str, list[tuple[State, Any]]]: """Distinct callables declared under each name, with the state body declaring them.""" @@ -209,11 +209,12 @@ def _nested_callbacks(cls) -> dict[str, list[tuple[State, Any]]]: state._callbacks = None return claims - def _update_lookup_names(cls, relocated: dict[Any, str]): + def _update_lookup_names(cls, relocated: dict[Any, str], own_states: list[State]): + """Inherited states share their specs with the base class, which must keep its own.""" if not relocated: return - for specs in cls._all_specs(): - for spec in specs: + for visited in iterate_states_and_transitions(own_states): + for spec in visited._specs: if spec.reference is SpecReference.CALLABLE and spec.func in relocated: spec.lookup_name = relocated[spec.func] @@ -226,8 +227,10 @@ def _names_resolved_by_name(cls) -> set[str]: names: set[str] = set() for specs in cls._all_specs(): for spec in specs: - if spec.reference is SpecReference.NAME: - names.update(_IDENTIFIER_RE.findall(spec.attr_name)) + if spec.may_contain_boolean_expression: + names.update(names_in_boolean_expr(spec.attr_name)) + elif spec.reference is SpecReference.NAME: + names.add(spec.attr_name) return names def _raise_callback_collision( @@ -246,14 +249,14 @@ def _raise_callback_collision( conflict = _("is declared in both the '{}' and the '{}' state bodies").format( state.id, owners[1][0].id ) - elif attr_name in class_body_names: - conflict = _("is declared in both the '{}' state body and the '{}' class body").format( - state.id, cls.__name__ - ) elif isinstance(existing, Event): conflict = _("in the '{}' state body clashes with the event '{}'").format( state.id, existing.id ) + elif attr_name in class_body_names: + conflict = _("is declared in both the '{}' state body and the '{}' class body").format( + state.id, cls.__name__ + ) else: conflict = _("in the '{}' state body clashes with the '{}' attribute of '{}'").format( state.id, attr_name, cls.__name__ diff --git a/statemachine/spec_parser.py b/statemachine/spec_parser.py index 1046a423..ee526faf 100644 --- a/statemachine/spec_parser.py +++ b/statemachine/spec_parser.py @@ -339,6 +339,15 @@ def parse_boolean_expr(expr, variable_hook, operator_mapping): return build_expression(tree.body, variable_hook, operator_mapping) +def names_in_boolean_expr(expr: str) -> "set[str]": + """Variable names that :func:`parse_boolean_expr` would resolve in ``expr``.""" + try: + tree = ast.parse(replace_operators(expr), mode="eval") + except SyntaxError: + return set() + return {node.id for node in ast.walk(tree) if isinstance(node, ast.Name)} + + def parse_expr(expr: str, variable_hook: Callable) -> Callable: """Parse a value expression into a callable using the restricted AST allowlist. diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index d3b9e7bf..f99e7de6 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -598,24 +598,111 @@ def ready(self): "class body. Nested state bodies share the class namespace. Rename one of them." ) - def test_decorated_callback_named_as_an_inherited_method(self): + @pytest.mark.parametrize("decorated", [False, True]) + async def test_nested_callback_overrides_an_inherited_method(self, sm_runner, decorated): class Guards: def ready(self): - return True + return "base" - with pytest.raises(InvalidDefinition, match="clashes with the 'ready' attribute"): + class Journey(Guards, StateChart): + class shire(State.Compound): + bag_end = State(initial=True) + green_dragon = State(final=True) + visit_pub = bag_end.to(green_dragon) - class Journey(Guards, StateChart): - class shire(State.Compound): - bag_end = State(initial=True) - green_dragon = State() - road = State(final=True) - visit_pub = bag_end.to(green_dragon, cond="ready") - leave = green_dragon.to(road) + if decorated: - @green_dragon.enter + @bag_end.exit def ready(self): - return False + return "nested" + + else: + + def ready(self): + return "nested" + + sm = await sm_runner.start(Journey) + assert sm.ready() == "nested" + + async def test_subclass_leaves_base_callbacks_untouched(self, sm_runner): + log = [] + + def arrive(self): + log.append(type(self).__name__) + + class Model: + pass + + Model.arrive = arrive + + class Base(StateChart): + validate_trap_states = False + validate_disconnected_states = False + idle = State(initial=True, enter=arrive) + + class Sub(Base): # noqa: F841 + class extra(State.Compound): + start = State(initial=True) + on_arrive = arrive + + class other(State.Compound): + begin = State(initial=True) + + @begin.enter + def arrive(self): + log.append("other") + + await sm_runner.start(Base, model=Model()) + assert log == ["Model"] + + async def test_string_literal_in_a_guard_does_not_block_storing_apart(self, sm_runner): + log = [] + + class War(StateChart): + mode = "audit" + + class war(State.Parallel): + class quest(State.Compound): + start = State(initial=True) + end = State(final=True) + go = start.to(end, cond="mode == 'audit'") + + @end.enter + def audit(self): + log.append("quest") + + class battle(State.Compound): + fighting = State(initial=True) + won = State(final=True) + go = fighting.to(won) + + @won.enter + def audit(self): + log.append("battle") + + sm = await sm_runner.start(War) + await sm_runner.send(sm, "go") + assert sorted(log) == ["battle", "quest"] + + def test_plain_callback_named_as_a_class_body_event(self): + with pytest.raises(InvalidDefinition) as exc_info: + + class Gate(StateChart): + class gate(State.Compound): + closed = State(initial=True) + opened = State(final=True) + push = closed.to(opened) + + def kick(self): + return True + + broken = State(final=True) + kick = gate.to(broken) + + assert str(exc_info.value) == ( + "Callback 'kick' in the 'gate' state body clashes with the event 'kick'. " + "Nested state bodies share the class namespace. Rename one of them." + ) def test_decorated_callback_beside_a_plain_helper_of_the_same_name(self): with pytest.raises(InvalidDefinition, match="Callback 'limit' is declared in both"):