From cef0b50221ccd30aa7163574775100f33cef400c Mon Sep 17 00:00:00 2001 From: chuenchen309 <48723787+chuenchen309@users.noreply.github.com> Date: Sun, 19 Jul 2026 11:39:41 +0800 Subject: [PATCH 1/4] Treat ^ and v as operators even without surrounding whitespace parse_boolean_expr has a fast-path that returns the whole string as a single variable name when it contains no operator. It only checked for "!", not the other two classical operators "^" (and) and "v" (or), so an unspaced guard like cond="a^b" skipped operator replacement and was looked up as a variable named "a^b" -- silently evaluating wrong (or raising InvalidDefinition on a StateChart). "!a" already worked unspaced, so this was an inconsistency. Gate the fast-path on the module's existing operator `pattern` (which already matches !, ^ and the word-bounded v) instead of a hand-rolled "!" check. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: chuenchen309 <48723787+chuenchen309@users.noreply.github.com> --- statemachine/spec_parser.py | 6 ++++-- tests/test_spec_parser.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/statemachine/spec_parser.py b/statemachine/spec_parser.py index 306a49c3..007d4d86 100644 --- a/statemachine/spec_parser.py +++ b/statemachine/spec_parser.py @@ -315,8 +315,10 @@ def parse_boolean_expr(expr, variable_hook, operator_mapping): if expr.strip() == "": raise SyntaxError("Empty expression") - # Optimization trying to avoid parsing the expression if not needed - if "!" not in expr and " " not in expr and "In(" not in expr: + # Optimization trying to avoid parsing the expression if not needed. Skip it + # when any classical operator is present, not just "!" -- an unspaced "^"/"v" + # (e.g. "a^b") would otherwise be swallowed into a single variable name. + if not pattern.search(expr) and " " not in expr and "In(" not in expr: return variable_hook(expr) expr = replace_operators(expr) tree = ast.parse(expr, mode="eval") diff --git a/tests/test_spec_parser.py b/tests/test_spec_parser.py index 51115bd7..cce7b87e 100644 --- a/tests/test_spec_parser.py +++ b/tests/test_spec_parser.py @@ -210,6 +210,18 @@ def test_classical_operators_name(): ) # name reflects expression structure +def test_classical_operators_without_spaces(): + # "^" and "v" are operators even without surrounding whitespace, like "!", + # so they must not be swallowed into a single variable name. + for unspaced, spaced in [ + ("frodo_has_ring^sam_is_loyal", "frodo_has_ring ^ sam_is_loyal"), + ("(frodo_has_ring)v(sauron_alive)", "(frodo_has_ring) v (sauron_alive)"), + ]: + got = parse_boolean_expr(unspaced, variable_hook, operator_mapping)() + want = parse_boolean_expr(spaced, variable_hook, operator_mapping)() + assert got is want, unspaced + + def test_empty_expression(): expr = "" with pytest.raises(SyntaxError): From a0a19a5bf335c9e54f7ea0cd01f513ba135ac2c3 Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Sat, 1 Aug 2026 08:23:44 -0300 Subject: [PATCH 2/4] fix: restrict cond fast-path to a lone identifier The fast-path in `parse_boolean_expr` returned the whole expression as a single variable name whenever it found no `!`, no space and no `In(`. Any other operator written without surrounding whitespace was swallowed into a variable name, so `cond="a^b"` or `cond="items>0"` were resolved as variables literally named `a^b` / `items>0`. Gating on the module's `pattern` (as originally proposed) fixes `^`/`v` but regresses two working cases: unspaced `!=` (skipped by the `\!(?!=)` lookahead) stops parsing as a comparison, and a guard literally named `v` starts being replaced by ` or `. The invariant the fast-path wants is "the whole string is a single variable name", which `str.isidentifier()` checks directly. This keeps `!=` and a bare `v` working, makes the `" "` / `"In("` checks redundant, and also fixes the sibling bug where unspaced comparisons like `x==1` were swallowed too. Expressions with a structure that is invalid in a boolean context (e.g. `cond="user.age"`) now reach the AST allowlist and raise `ValueError`, so `Listeners.build` catches it alongside `SyntaxError` to keep reporting the friendly `InvalidDefinition`. Signed-off-by: Fernando Macedo --- docs/releases/3.2.1.md | 49 ++++++++++++++++++++++++++++++++ statemachine/dispatcher.py | 4 ++- statemachine/spec_parser.py | 8 +++--- tests/test_conditions_algebra.py | 11 +++++++ tests/test_spec_parser.py | 21 ++++++++++++++ 5 files changed, 88 insertions(+), 5 deletions(-) diff --git a/docs/releases/3.2.1.md b/docs/releases/3.2.1.md index a05e422e..cacd0476 100644 --- a/docs/releases/3.2.1.md +++ b/docs/releases/3.2.1.md @@ -100,3 +100,52 @@ instead of at class-definition time. ``` [#632](https://github.com/fgmacedo/python-statemachine/issues/632). + +### Operators without surrounding whitespace in `cond` expressions + +Boolean expressions used in `cond` / `unless` had a fast-path that returned the +whole string as a single variable name when it looked operator-free. The check +was too naive: it only looked for `!`, a literal space and `In(`, so any other +operator written without surrounding whitespace was swallowed into a variable +name. `cond="is_paid^is_shipped"` and `cond="items>0"` were resolved as +variables literally named `is_paid^is_shipped` and `items>0`, which raised +`InvalidDefinition: Did not found name ...` at class-definition time (or +silently evaluated to the wrong value when such an attribute happened to exist). + +The fast-path now triggers only for a lone Python identifier, so every other +expression goes through the parser and whitespace is never required: + +```py +>>> from statemachine import State, StateMachine + +>>> class Order(StateMachine): +... waiting = State(initial=True) +... completed = State(final=True) +... +... complete = waiting.to(completed, cond="is_paid^items>0") +... +... is_paid: bool = False +... items: int = 2 + +>>> sm = Order() +>>> sm.send("complete") +Traceback (most recent call last): + ... +statemachine.exceptions.TransitionNotAllowed: Can't complete when in Waiting. + +>>> sm.is_paid = True +>>> sm.send("complete") +>>> sm.completed.is_active +True + +``` + +Guards that are a single name (including a bare `v`) keep taking the fast-path, +and `!=` keeps parsing as a comparison rather than a negation. + +As a side effect, a `cond` with a structure that is not valid in a boolean +expression, such as `cond="user.age"`, now raises +`InvalidDefinition: Failed to parse boolean expression 'user.age'` instead of +reporting the whole string as a name that was not found. + +[#639](https://github.com/fgmacedo/python-statemachine/pull/639). diff --git a/statemachine/dispatcher.py b/statemachine/dispatcher.py index 3324f38a..c879775b 100644 --- a/statemachine/dispatcher.py +++ b/statemachine/dispatcher.py @@ -118,7 +118,9 @@ def build(self, spec: "CallbackSpec"): try: expression = parse_boolean_expr(spec.func, take_callback_partial, operator_mapping) - except SyntaxError as err: + except (SyntaxError, ValueError) as err: + # ``ValueError`` comes from the AST allowlist rejecting a node kind that + # cannot appear in a boolean expression (e.g. ``"user.age"``). raise InvalidDefinition( _("Failed to parse boolean expression '{}'").format(spec.func) ) from err diff --git a/statemachine/spec_parser.py b/statemachine/spec_parser.py index 007d4d86..c4b0b035 100644 --- a/statemachine/spec_parser.py +++ b/statemachine/spec_parser.py @@ -315,10 +315,10 @@ def parse_boolean_expr(expr, variable_hook, operator_mapping): if expr.strip() == "": raise SyntaxError("Empty expression") - # Optimization trying to avoid parsing the expression if not needed. Skip it - # when any classical operator is present, not just "!" -- an unspaced "^"/"v" - # (e.g. "a^b") would otherwise be swallowed into a single variable name. - if not pattern.search(expr) and " " not in expr and "In(" not in expr: + # Optimization: a lone identifier can only be a variable name, so there is + # nothing to parse. Anything else (operators, comparisons, spaces, calls) + # goes through the parser. + if expr.isidentifier(): return variable_hook(expr) expr = replace_operators(expr) tree = ast.parse(expr, mode="eval") diff --git a/tests/test_conditions_algebra.py b/tests/test_conditions_algebra.py index db5e3c7a..286a1f60 100644 --- a/tests/test_conditions_algebra.py +++ b/tests/test_conditions_algebra.py @@ -66,3 +66,14 @@ class AnyConditionSM(StateChart): with pytest.raises(InvalidDefinition, match="Did not found name 'xxx'"): AnyConditionSM() + + +def test_should_raise_invalid_definition_if_cond_has_unsupported_structure(): + class AnyConditionSM(StateChart): + start = State(initial=True) + end = State(final=True) + + submit = start.to(end, cond="user.age") + + with pytest.raises(InvalidDefinition, match="Failed to parse boolean expression 'user.age'"): + AnyConditionSM() diff --git a/tests/test_spec_parser.py b/tests/test_spec_parser.py index cce7b87e..902c7e91 100644 --- a/tests/test_spec_parser.py +++ b/tests/test_spec_parser.py @@ -222,6 +222,27 @@ def test_classical_operators_without_spaces(): assert got is want, unspaced +def test_unspaced_not_equal_is_a_comparison(): + # "!" is an operator, but "!=" is not a negation: it must keep parsing + # as a comparison even without surrounding whitespace. + got = parse_boolean_expr("frodo_age!=51", variable_hook, operator_mapping)() + want = parse_boolean_expr("frodo_age != 51", variable_hook, operator_mapping)() + assert got is want is True + + +def test_unspaced_comparison(): + # A comparison is not a variable name, even without surrounding whitespace. + got = parse_boolean_expr("frodo_age>=50", variable_hook, operator_mapping)() + want = parse_boolean_expr("frodo_age >= 50", variable_hook, operator_mapping)() + assert got is want is True + + +def test_bare_v_is_a_variable_name(): + # "v" is only an operator between operands; a lone "v" is a plain variable. + expr = parse_boolean_expr("v", variable_hook, operator_mapping) + assert expr.__name__ == "v" + + def test_empty_expression(): expr = "" with pytest.raises(SyntaxError): From e0ee324de22f439b092e825962524eb135017779 Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Sat, 1 Aug 2026 08:43:24 -0300 Subject: [PATCH 3/4] refactor: raise a dedicated error for unsupported guard expressions Catching a bare `ValueError` around `parse_boolean_expr` was too broad: the `variable_hook` runs user code while resolving names (`search_name` reads attributes from the model, which may execute a property), so an unrelated `ValueError` raised there was reported as `InvalidDefinition: Failed to parse boolean expression ''`, hiding the real cause. The parser now raises `UnsupportedExpression` (a `ValueError` subclass, so `io.evaluators` and user code catching `ValueError` keep working) when a node kind is rejected by the allowlist, and `Listeners.build` catches only that. Errors from name resolution propagate untouched. Also parametrizes the unspaced-operator tests, adding a falsy case, and drops an inaccurate claim from the release notes: a name that is not a valid identifier can never be silently resolved, since `search_name` only matches names present in `dir(obj)`. Signed-off-by: Fernando Macedo --- docs/releases/3.2.1.md | 5 ++-- statemachine/dispatcher.py | 8 +++--- statemachine/spec_parser.py | 17 +++++++++--- tests/test_conditions_algebra.py | 18 +++++++++++++ tests/test_spec_parser.py | 46 +++++++++++++++----------------- 5 files changed, 60 insertions(+), 34 deletions(-) diff --git a/docs/releases/3.2.1.md b/docs/releases/3.2.1.md index cacd0476..ea3983f3 100644 --- a/docs/releases/3.2.1.md +++ b/docs/releases/3.2.1.md @@ -108,9 +108,8 @@ whole string as a single variable name when it looked operator-free. The check was too naive: it only looked for `!`, a literal space and `In(`, so any other operator written without surrounding whitespace was swallowed into a variable name. `cond="is_paid^is_shipped"` and `cond="items>0"` were resolved as -variables literally named `is_paid^is_shipped` and `items>0`, which raised -`InvalidDefinition: Did not found name ...` at class-definition time (or -silently evaluated to the wrong value when such an attribute happened to exist). +variables literally named `is_paid^is_shipped` and `items>0`, failing with +`InvalidDefinition: Did not found name ...` when the machine was built. The fast-path now triggers only for a lone Python identifier, so every other expression goes through the parser and whitespace is never required: diff --git a/statemachine/dispatcher.py b/statemachine/dispatcher.py index c879775b..8f9bc369 100644 --- a/statemachine/dispatcher.py +++ b/statemachine/dispatcher.py @@ -14,6 +14,7 @@ from .exceptions import InvalidDefinition from .i18n import _ from .signature import SignatureAdapter +from .spec_parser import UnsupportedExpression from .spec_parser import custom_and from .spec_parser import operator_mapping from .spec_parser import parse_boolean_expr @@ -118,9 +119,10 @@ def build(self, spec: "CallbackSpec"): try: expression = parse_boolean_expr(spec.func, take_callback_partial, operator_mapping) - except (SyntaxError, ValueError) as err: - # ``ValueError`` comes from the AST allowlist rejecting a node kind that - # cannot appear in a boolean expression (e.g. ``"user.age"``). + except (SyntaxError, UnsupportedExpression) as err: + # ``UnsupportedExpression`` comes from the AST allowlist rejecting a node + # kind that cannot appear in a boolean expression (e.g. ``"user.age"``). + # Errors raised while resolving names are not parse errors and propagate. raise InvalidDefinition( _("Failed to parse boolean expression '{}'").format(spec.func) ) from err diff --git a/statemachine/spec_parser.py b/statemachine/spec_parser.py index c4b0b035..9f9713af 100644 --- a/statemachine/spec_parser.py +++ b/statemachine/spec_parser.py @@ -19,6 +19,15 @@ } +class UnsupportedExpression(ValueError): + """The expression contains a structure that is not on the parser allowlist. + + Distinguishes a rejected expression from errors raised by the ``variable_hook`` + while resolving names, so callers can report each one properly. Inherits from + ``ValueError`` to keep the previous behavior for callers catching it. + """ + + def _unique_key(left, right, operator) -> str: left_key = getattr(left, "unique_key", "") right_key = getattr(right, "unique_key", "") @@ -128,7 +137,7 @@ def register(func): def get(cls, func_id): func_id = func_id.lower() if func_id not in cls.registry: - raise ValueError(f"Unsupported function: {func_id}") + raise UnsupportedExpression(f"Unsupported function: {func_id}") return cls.registry[func_id] @@ -300,14 +309,16 @@ def recurse(child): # without underscore-attribute access or method calls, it cannot reach # type objects. if attr.startswith("_"): - raise ValueError(f"Attribute access to '{attr}' is not allowed") + raise UnsupportedExpression(f"Attribute access to '{attr}' is not allowed") return build_attribute(recurse(node.value), attr) case ast.Name(id=name): return variable_hook(name) case ast.Constant(value=value): return build_constant(value) case _: - raise ValueError(f"Unsupported expression structure: {node.__class__.__name__}") + raise UnsupportedExpression( + f"Unsupported expression structure: {node.__class__.__name__}" + ) def parse_boolean_expr(expr, variable_hook, operator_mapping): diff --git a/tests/test_conditions_algebra.py b/tests/test_conditions_algebra.py index 286a1f60..525c8808 100644 --- a/tests/test_conditions_algebra.py +++ b/tests/test_conditions_algebra.py @@ -77,3 +77,21 @@ class AnyConditionSM(StateChart): with pytest.raises(InvalidDefinition, match="Failed to parse boolean expression 'user.age'"): AnyConditionSM() + + +def test_should_not_mask_errors_raised_while_resolving_names(): + # Resolving a name reads attributes from the model, which runs user code. An + # error raised there is not a parse error and must not be reported as one. + class Model: + @property + def is_ready(self): + raise ValueError("the model is not configured") + + class AnyConditionSM(StateChart): + start = State(initial=True) + end = State(final=True) + + submit = start.to(end, cond="is_ready") + + with pytest.raises(ValueError, match="the model is not configured"): + AnyConditionSM(Model()) diff --git a/tests/test_spec_parser.py b/tests/test_spec_parser.py index 902c7e91..8b4a9db7 100644 --- a/tests/test_spec_parser.py +++ b/tests/test_spec_parser.py @@ -210,31 +210,27 @@ def test_classical_operators_name(): ) # name reflects expression structure -def test_classical_operators_without_spaces(): - # "^" and "v" are operators even without surrounding whitespace, like "!", - # so they must not be swallowed into a single variable name. - for unspaced, spaced in [ - ("frodo_has_ring^sam_is_loyal", "frodo_has_ring ^ sam_is_loyal"), - ("(frodo_has_ring)v(sauron_alive)", "(frodo_has_ring) v (sauron_alive)"), - ]: - got = parse_boolean_expr(unspaced, variable_hook, operator_mapping)() - want = parse_boolean_expr(spaced, variable_hook, operator_mapping)() - assert got is want, unspaced - - -def test_unspaced_not_equal_is_a_comparison(): - # "!" is an operator, but "!=" is not a negation: it must keep parsing - # as a comparison even without surrounding whitespace. - got = parse_boolean_expr("frodo_age!=51", variable_hook, operator_mapping)() - want = parse_boolean_expr("frodo_age != 51", variable_hook, operator_mapping)() - assert got is want is True - - -def test_unspaced_comparison(): - # A comparison is not a variable name, even without surrounding whitespace. - got = parse_boolean_expr("frodo_age>=50", variable_hook, operator_mapping)() - want = parse_boolean_expr("frodo_age >= 50", variable_hook, operator_mapping)() - assert got is want is True +@pytest.mark.parametrize( + ("unspaced", "spaced", "expected"), + [ + ("frodo_has_ring^sam_is_loyal", "frodo_has_ring ^ sam_is_loyal", True), + ("frodo_has_ring^sauron_alive", "frodo_has_ring ^ sauron_alive", False), + ("(frodo_has_ring)v(sauron_alive)", "(frodo_has_ring) v (sauron_alive)", True), + # "!" is an operator, but "!=" is not a negation: it must keep parsing + # as a comparison. Note the "!=51" (and not "!=50"): a misparse resolves + # the whole string as an unknown variable, which defaults to False, so a + # "!=50" case would pass for the wrong reason. + ("frodo_age!=51", "frodo_age != 51", True), + ("frodo_age>=50", "frodo_age >= 50", True), + ], +) +def test_operators_without_surrounding_spaces(unspaced, spaced, expected): + # Operators do not require surrounding whitespace, so an unspaced expression + # must parse just like its spaced equivalent instead of being swallowed into + # a single variable name. + got = parse_boolean_expr(unspaced, variable_hook, operator_mapping)() + want = parse_boolean_expr(spaced, variable_hook, operator_mapping)() + assert got is want is expected def test_bare_v_is_a_variable_name(): From 7019e3e7ef8d65973963eaddc09040ccdb0f94ab Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Sat, 1 Aug 2026 08:54:25 -0300 Subject: [PATCH 4/4] test: keep a single raising call inside pytest.raises SonarCloud python:S5778: the `with pytest.raises(...)` block held two calls that could raise (`Model()` and the state machine constructor), so the test could pass for the wrong reason if the model constructor started raising `ValueError` on its own. Build the model outside the block. Signed-off-by: Fernando Macedo --- tests/test_conditions_algebra.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_conditions_algebra.py b/tests/test_conditions_algebra.py index 525c8808..bf46f90d 100644 --- a/tests/test_conditions_algebra.py +++ b/tests/test_conditions_algebra.py @@ -93,5 +93,6 @@ class AnyConditionSM(StateChart): submit = start.to(end, cond="is_ready") + model = Model() with pytest.raises(ValueError, match="the model is not configured"): - AnyConditionSM(Model()) + AnyConditionSM(model)