Skip to content

Treat ^ and v as operators even without surrounding whitespace#639

Open
chuenchen309 wants to merge 1 commit into
fgmacedo:developfrom
chuenchen309:fix/spec-parser-unspaced-classical-operators
Open

Treat ^ and v as operators even without surrounding whitespace#639
chuenchen309 wants to merge 1 commit into
fgmacedo:developfrom
chuenchen309:fix/spec-parser-unspaced-classical-operators

Conversation

@chuenchen309

Copy link
Copy Markdown

parse_boolean_expr has a fast-path that returns the whole expression as a
single variable name when it looks operator-free. It only checked for !, not
the other two documented classical operators ^ (and) / v (or), so an
unspaced guard skipped operator replacement and was resolved as a variable:

cond="a ^ b"   # parsed as (a and b)
cond="a^b"     # resolved as a variable literally named "a^b"

a^b then evaluates wrong (or raises InvalidDefinition: Did not found name 'a^b' at class-definition time). Since !a already worked without spaces, the
inconsistency is surprising — users copy operators from the guards docs, where
^/v are listed with no whitespace requirement.

Gate the fast-path on the module's existing pattern (which already matches
!, ^, and the word-bounded v) instead of the hand-rolled "!" not in expr
check. Genuine single-name variables like avc (no word-boundary v) still
take the fast-path. Added a regression test asserting the unspaced forms match
their spaced equivalents.


This PR was authored by an AI coding agent (Claude Code) running on this account:
the AI found the bug, ran the repro, wrote the test, and wrote this description.
The human account holder reviews every change and is accountable for it. The
verification is real and re-runnable from the diff. If this isn't the kind of
contribution you want, say so and I'll close it.

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) <noreply@anthropic.com>
Signed-off-by: chuenchen309 <48723787+chuenchen309@users.noreply.github.com>
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##           develop      #639   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        52           
  Lines         5505      5505           
  Branches       869       869           
=========================================
  Hits          5505      5505           
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.

@fgmacedo fgmacedo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chuenchen309, thanks a lot for this contribution, and congratulations on your first PR to the project! 🎉

Great catch: the fast-path really does mishandle unspaced ^/v, and your diagnosis is spot on. While reviewing I found that gating on pattern introduces two behavior regressions that our test suite doesn't cover (which is why CI stayed green). Details and a suggested alternative inline.

Could you also add a short entry about the fix to the open release notes under docs/releases/? We document every user-facing bugfix there.

Comment on lines +318 to +321
# 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:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating on pattern fixes the reported cases, but it introduces two regressions. I verified both against develop (both fail loudly at class-definition time):

  1. Unspaced != stops working. pattern deliberately skips != via the \!(?!=) lookahead, so cond="steps!=2" no longer matches, takes the fast-path, and is resolved as a variable literally named steps!=2:

    InvalidDefinition: Did not found name 'steps!=2' from model or statemachine
    

    On develop, any ! character skipped the fast-path, so this parsed as a comparison and worked.

  2. A guard literally named v stops working. \bv\b matches a bare v, so cond="v" (a boolean attribute named v) now goes through replace_operators, becomes " or ", and fails with:

    InvalidDefinition: Failed to parse boolean expression 'v'
    

The invariant the fast-path wants is "this whole string is a single variable name", and Python can check that directly:

Suggested change
# 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():

This keeps your fix for a^b / (a)v(b), keeps unspaced != and bare v working, makes the " " and "In(" checks redundant, and as a bonus also fixes the pre-existing sibling bug where unspaced comparisons like x==1 / x>=1 were swallowed as variable names too.

One side effect to be aware of: inputs that were already invalid, like cond="user.age", now fail with ValueError: Unsupported expression structure: Attribute instead of InvalidDefinition: Did not found name .... If we want to keep the friendlier error, dispatcher.build can catch ValueError alongside SyntaxError.

Comment thread tests/test_spec_parser.py
]:
got = parse_boolean_expr(unspaced, variable_hook, operator_mapping)()
want = parse_boolean_expr(spaced, variable_hook, operator_mapping)()
assert got is want, unspaced

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test, thanks for including it. Since CI stayed green despite the regressions mentioned in spec_parser.py, could you extend the coverage here to pin the behaviors that must keep working? These two cases would have caught them:

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_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"

Note the !=51 (not !=50): a misparse resolves the whole string as an unknown variable, which this variable_hook defaults to False, so a !=50 case would pass for the wrong reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants