Skip to content

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

Merged
fgmacedo merged 7 commits into
mainfrom
fgmacedo/fix-653-sibling-callback-collision
Oct 13, 2026
Merged

fgmacedo merged 7 commits into
mainfrom
fgmacedo/fix-653-sibling-callback-collision

Conversation

@fgmacedo

@fgmacedo fgmacedo commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Fixes #653

Why

Callbacks declared inside State.Compound / State.Parallel bodies are flattened into the class namespace keyed by name only, so the same name in two sibling regions kept only the last body. With the issue's example, send("go") moved both regions and ran 1 of the 2 go bodies (log ['r2'], measured on origin/main). The same loss applied to plain callbacks such as on_enter_<state> in two bodies and to a nested body shadowing a method of the class body (log ['nested']).

What changes

  • A callback name declared in two nested bodies raises InvalidDefinition at class creation, naming the callback and both state bodies. The decorator form reports the name the user wrote (go, not the mangled _go_).
  • A nested callback that shadows a method of the class body raises too (today it silently replaced it).
  • A nested callback whose name an event, or another attribute of the class, already uses raises with a message naming the event or the attribute. Before, this clobbered the event trigger silently, or crashed with TypeError for states.
  • The same callable assigned under one name in several bodies stays accepted: plain function, bound method of one instance, functools.partial.
  • Subclassing is unaffected: the check only sees callbacks pending on a state, and a subclass that overrides a nested callback in its own body keeps working. The subclass crash on _callbacks is Subclassing a StateChart with callbacks in a State.Compound body raises AttributeError #646, handled by its own PR; this check does not misfire with either del or = None there.
  • Namespacing each nested body so the same name can be reused across regions stays with Read every statechart class body through one reader with an owning state #657.

Evidence

Issue reproduction, before and after:

$ uv run python before_after.py   # origin/main factory.py
log: ['r2']
$ uv run python before_after.py   # this branch
InvalidDefinition: Callback 'go' is declared in both the 'r1' and the 'r2' state bodies. Nested state bodies share the class namespace. Rename one of them.
$ timeout 120 uv run pytest tests/test_statechart_compound.py -k TestCallbackNameCollision -p no:cacheprovider --no-cov -q
16 passed, 51 deselected in 0.09s
$ timeout 120 uv run pytest -n 4 -p no:cacheprovider -q
2358 passed, 1 skipped, 44 xfailed, 9 warnings in 24.53s
$ timeout 120 uv run pytest tests/test_statechart_compound.py --cov=statemachine.factory --cov-report=term-missing --cov-branch -p no:cacheprovider -q
statemachine/factory.py  ...  (no missing line or branch inside the new code, 166-224)
$ uv run mypy --namespace-packages --explicit-package-bases statemachine/ tests/
Success: no issues found in 262 source files
$ uv run pyright statemachine/
0 errors, 0 warnings, 0 informations

The seven raising tests fail on origin/main (7 failed, 8 passed when the test class runs against a git archive origin/main export).

Three adversarial review rounds by a fresh reviewer each. Round 1 found a bound-method false positive (is comparison) and misleading "class body" wording for events and metaclass attributes, both fixed in the second commit. Round 2 found a TypeError from States.__eq__ when the clashing attribute is cls.states, fixed in the third commit. Round 3 verified the guard only.

Not verified

  • A subclass that adds a new nested body reusing a callback name from a base body is not detected (the base state has no pending callbacks on the second metaclass run). Left as "a subclass overrides on purpose".
  • Two distinct callables of the same type whose __eq__ returns True across instances pass the check and the last one wins. Not reachable with functions, methods or partials.
  • Helper classes or Enums defined inside a nested body are classified as callbacks by pre-existing code, so two bodies defining class Kind(Enum) raise with "Callback 'Kind'"; the collision is real, only the noun is off.
  • Not run against the parallel PR for Subclassing a StateChart with callbacks in a State.Compound body raises AttributeError #646 itself; the state._callbacks = None variant was simulated locally by patching the line.

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 <fgmacedo@gmail.com>
…ion 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 <fgmacedo@gmail.com>
`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 <fgmacedo@gmail.com>
@fgmacedo fgmacedo added this to the 3.2.2 milestone Oct 12, 2026
@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (dbf8eb0) to head (aec7d91).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #674   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines         5714      5787   +73     
  Branches       900       918   +18     
=========================================
+ Hits          5714      5787   +73     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 <fgmacedo@gmail.com>
…ach it

Holding a function by object does not make its name unreachable: a string
reference, a naming convention or a self.<name>() 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 <fgmacedo@gmail.com>
…ing-callback-collision

Signed-off-by: Fernando Macedo <fgmacedo@gmail.com>

# Conflicts:
#	docs/releases/3.2.2.md
#	statemachine/factory.py
#	tests/test_statechart_compound.py
…backs 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 <fgmacedo@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

@fgmacedo
fgmacedo merged commit b52d534 into main Oct 13, 2026
13 checks passed
@fgmacedo
fgmacedo deleted the fgmacedo/fix-653-sibling-callback-collision branch October 13, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Callback names collide between sibling nested state bodies

1 participant