fix: reject callback names colliding between nested state bodies (#674)

Author: fgmacedo

docs/releases/3.2.2.md

@@ -76,6 +76,83 @@ 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_<state>`
+declared in two bodies, and to a nested body overriding a method of the class body.
+
+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
+>>> 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 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, 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)
+...
+...                 def on_enter_end(self):
+...                     print("quest")
+...
+...             class battle(State.Compound):
+...                 fighting = State(initial=True)
+...                 won = State(final=True)
+...                 go = fighting.to(won)
+...
+...                 def on_enter_end(self):
+...                     print("battle")
+... except InvalidDefinition as e:
+...     print(e)
+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, 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).
+
+[#653](https://github.com/fgmacedo/python-statemachine/issues/653).
+
 ### Subclassing a chart with callbacks in a `State.Compound` body
 
 Subclassing a `StateChart` whose compound (or parallel) body declared any callback, as a plain

docs/states.md

@@ -162,6 +162,11 @@ True
 
 ```
 
+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
 states (all regions must reach a final state).

docs/validations.md

@@ -183,6 +183,47 @@ 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. A name reached by name (a naming convention such as
+`on_enter_<state>`, 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:
+...     class Bad(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):
+...                     print("quest")
+...
+...             class battle(State.Compound):
+...                 fighting = State(initial=True)
+...                 won = State(final=True)
+...                 go = fighting.to(won)
+...
+...                 def on_enter_end(self):
+...                     print("battle")
+... except InvalidDefinition as e:
+...     print(e)
+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, 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
 
 Entries in the `listeners` class attribute must be classes, callables, or
@@ -280,6 +321,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 in nested bodies   | Class definition| No                         |
 | Callback resolution               | Instance creation | No                       |
 | Boolean expression parsing        | Instance creation | No                       |
 

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,11 @@ def allways_true(*args, **kwargs):
     return True
 
 
+def is_unbounded_reference(func: Any) -> bool:
+    """Whether ``func`` is a function that a `CallbackSpec` holds by object."""
+    return isfunction(func) and hasattr(func, "attr_name")
+
+
 class CallbackPriority(IntEnum):
     GENERIC = 0
     INLINE = 10
@@ -103,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})"
 

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.lookup_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)

statemachine/factory.py

@@ -5,6 +5,8 @@
 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
 from .exceptions import InvalidDefinition
@@ -13,12 +15,25 @@
 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
 
 
+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.<name>``, 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"
 
@@ -59,7 +74,6 @@ def __init__(
         cls.add_inherited(bases)
         cls.add_from_attributes(attrs)
         cls._collect_class_listeners(attrs, bases)
-        cls._unpack_builders_callbacks()
         cls._update_event_references()
 
         if not cls.states:
@@ -88,6 +102,7 @@ def __init__(
 
         cls._check()
         cls._setup()
+        cls._unpack_builders_callbacks(attrs)
         cls._expand_docstring()
 
     _STATECHART_RE = re.compile(r"\{statechart:(\w+)\}")
@@ -163,14 +178,96 @@ 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]):
+        names_resolved_by_name = cls._names_resolved_by_name()
+        relocated: dict[Any, str] = {}
+        for attr_name, owners in cls._nested_callbacks().items():
+            existing = cls.__dict__.get(attr_name)
+            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)
+        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."""
+        claims: dict[str, list[tuple[State, Any]]] = {}
         for state in iterate_states(cls.states):
             if state._callbacks:
-                callbacks.update(state._callbacks)
+                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))
                 state._callbacks = None
-        for key, value in callbacks.items():
-            setattr(cls, key, value)
+        return claims
+
+    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 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]
+
+    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.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(
+        cls,
+        attr_name: str,
+        existing: Any,
+        owners: list[tuple[State, Any]],
+        attrs: dict[str, Any],
+    ):
+        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(
+                state.id, owners[1][0].id
+            )
+        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__
+            )
+        name = func.__name__ if getattr(func, "is_event", False) else attr_name
+        raise InvalidDefinition(
+            _(
+                "Callback '{}' {}. Nested state bodies share the class namespace. "
+                "Rename one of them."
+            ).format(name, conflict)
+        )
 
     def _check(cls):
         has_states = bool(cls.states)

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.