fix: key instance states by state.value, not state.id (#624)#625
Open
AndreyNenashev wants to merge 1 commit into
Open
fix: key instance states by state.value, not state.id (#624)#625AndreyNenashev wants to merge 1 commit into
AndreyNenashev wants to merge 1 commit into
Conversation
PR fgmacedo#592 introduced ``Configuration._instance_states`` as a perf optimization but keyed it by ``state.id`` — the Python attribute name, which is not unique across sibling Compounds. When two ``State.Compound`` regions declared inner states under the same attribute name (e.g. ``g1.a`` and ``g2.a``), the second overwrote the first in the dict, causing dispatch to resolve against the wrong active state and raise ``TransitionNotAllowed``. Re-key ``_instance_states`` by ``state.value`` — the canonical identifier already used as the ``states_map`` key and guaranteed unique by construction when users provide explicit ``value=``. This restores the 3.0.0 resolution path that PR fgmacedo#592 inadvertently changed. Charts that rely on default ``value=`` and reuse Python attribute names across sibling Compounds were never supported (silent ``states_map`` overwrite at class-build time, predates fgmacedo#592). Surfacing that case as ``InvalidDefinition`` is tracked as a separate follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Andrey Nenashev <anenashev90@gmail.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Restores the 3.0.0 active-state resolution path that PR #592 inadvertently changed.
Configuration._instance_statesis re-keyed bystate.value(the canonical identifier already used bystates_map) instead ofstate.id(the Python attribute name, which collides across sibling Compounds with same-named children).Closes #624.
Change
statemachine/statemachine.py:211—_build_configurationwritesinstance_states[state.value] = ist.statemachine/configuration.py:87—Configuration.statesresolves directly:self._instance_states[v](drops the redundant_states_map[v].idhop).statemachine/configuration.py:36— type hint loosened toMapping[Any, State]to match the establisheddict[Any, State]convention used bystates_map.The
vars(self)[state.id] = istuser-facing attribute shortcut atstatemachine.py:213is untouched — orthogonal concern.Tests
New
TestSiblingCompoundIdCollisionclass intests/test_configuration.py:test_dispatch_to_g1_inner_state_both_engines— the exact Sibling Compound states with same-named inner attributes break in 3.1.0+ (regression bisected to #592) #624 repro, parametrized sync/async viasm_runner.test_dispatch_to_g2_inner_state_both_engines— same shape but in the second compound.test_instance_states_keyed_by_value_no_collision— white-box guard:len(_instance_states) == len(states_map)and key sets match.Full suite: 1472 passed, 144 skipped, 44 xfailed in ~23s. Branch coverage on
statemachine.configurationandstatemachine.statemachinepreserved.Out of scope — deeper issue surfaced during review
Charts that rely on default
value=and reuse Python attribute names across sibling Compounds were never supported, in any version:factory.py:341(cls.states_map[state.value] = state) silently overwrites. The chart constructs successfully butstates_maphas fewer entries than declared states. This predates #592 and is not a regression — it cannot be reached or repaired at the_instance_stateslayer.Tracking as separate follow-ups:
InvalidDefinitioninfactory.add_statewhenstate.valuecollides. Surfaces the constraint instead of silently overwriting. Backward-incompatible (charts that "worked by accident" will refuse to construct); should land in a minor release with notes.state.id(SCXML-style globally-unique ids) — would also make the default-value case work, but touches__repr__, diagrams, and callback name resolution. Larger design discussion.