Skip to content

fix(compiler): emit grouping parentheses from operator precedence - #98

Open
awli wants to merge 2 commits into
mainfrom
allen-compiler-precedence-printing
Open

awli wants to merge 2 commits into
mainfrom
allen-compiler-precedence-printing

Conversation

@awli

@awli awli commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

[AI] This PR was drafted by an AI agent on behalf of @awli and reviewed before posting.

What

The compiler now adds grouping parentheses based on operator precedence, instead of looking for parenthesized_expression nodes in the CST. Grouping comes only from the shape of the AST and Python's precedence rules, because the compiled expression runs as Python. getParenthesizedCst and the CST-based comparison spacing logic are removed.

Why

Follow-up to #94 / #95. #95 fixed binary, unary and comparison operands by climbing the CST from each operand to find the source parentheses. That approach needs every compile function to remember the check, and some didn't. These still produce wrong or invalid output on main (ace15eb2):

Source main This PR
(1 if True else 2) if False else 3 1 if True else 2 if False else 3 (evaluates to 1 instead of 3) (1 if True else 2) if False else 3
1 if (2 if True else 3) else 4 1 if 2 if True else 3 else 4 (SyntaxError) 1 if (2 if True else 3) else 4
(1 + 2)[0] 1 + 2[0] (1 + 2)[0]
(@variables.a or @variables.b).x state.a or state.b.x (state.a or state.b).x
(a + b)() a + b() (a + b)()
(1).real 1.real (SyntaxError: 1. lexes as a float) (1).real
AST built in code: BinaryExpression(BinaryExpression(1,'+',2),'*',3) 1 + 2 * 3 (no CST to inspect) (1 + 2) * 3

The #94/#95 cases stay correct: (1 + 2) * (3 + 4), not (True and False), -(1 + 2), (True or False) == False, and @variables.a and not (@variables.b or @variables.c) → state.a and not (state.b or state.c).

How

packages/compiler/src/expressions/compile-expression.ts:

  • One precedence table, following Python's order from lowest to highest: ternary < or < and < not < comparison < + - < * / < unary + - < primary (literal, reference, call, subscript, member access).
  • compileOperand(expr, minPrecedence) wraps an operand that binds more loosely than its position allows. The rules:
    • Binary operators are left-associative, so a right operand with equal precedence is wrapped: 1 - (2 - 3), 1 / (2 * 3).
    • Comparisons: Python chains them (a < b < c means a < b and b < c), so a comparison used as an operand of a comparison is always wrapped, on either side.
    • Ternaries are right-associative. Only the alternative may be an unwrapped ternary; a ternary consequence, condition or operand is wrapped.
    • not wraps operands looser than not, so not (a or b) is wrapped, while not a == b and a == (not b) need no extra handling. Unary +/- wrap any non-primary operand, including a nested sign, so -(-1) stays -(-1) rather than --1.
    • Targets of member access, subscript and call must be primary. .length becomes len(...), which needs no extra parentheses. A number literal before . is always wrapped, because 1.real lexes as the float 1..
    • Negative number literals, which can only be built in code, rank as unary because they print with a leading -. So MemberExpression(NumberLiteral(-1), 'real') compiles to (-1).real.
  • Behavior changes in compiled output. These don't change meaning:
    • Redundant source parentheses are no longer echoed. (1 + 2) compiles to 1 + 2, and (a == 1) and (b == 2) to a == 1 and b == 2.
    • Comparisons are emitted as left op right. The old code copied the source whitespace around the operator by slicing CST text, so 1 == 1 came out as 1 == 1. That logic dates from the original compiler. The unit tests from fix(compiler): preserve parentheses on both binary operands #95 that pinned the spacing now expect the normalized form.
    • Golden fixtures: 2 of 293 change (.snake and .camel for each), regenerated with pnpm fixtures:regen. connection_with_unified_email drops parentheses that and-over-or precedence makes redundant, e.g. X or (A and B and C) becomes X or A and B and C. This fixture was already stale on main after fix(compiler): preserve parentheses on both binary operands #95. post_action_conditionals only loses the doubled space in state.email is not None. No test in this repo reads these fixtures, so they had drifted silently.
  • One behavior change worth knowing. An unparenthesized chain such as 3 > 2 > 1 parses left-nested in both grammars, so it now compiles to (3 > 2) > 1 (False). main emitted Python's chained form 3 > 2 > 1 (True). The AST can't tell the chain apart from an explicit (3 > 2) > 1, so this output follows the tree. No fixture contains a chain.
  • Splice sites unchanged. I audited every place in compiler and agentforce that combines compiled conditions with and/or/not (compile-tool, compile-directives, else-if-chain, compile-workflow-node, compile-collect, file-reference-capture). Each already parenthesizes the parts it joins, so none needed changes.

Test Plan

  • Existing tests pass: compiler 1167 passed (3 skipped), agentforce 320 passed. Golden fixtures regenerated as described above.
  • New/updated tests cover the change:
  • Python check: 46 hand-written literal cases, plus 15,000 random ASTs (3 seeds × 5,000). The trees cover ints, bools and negative literals with every operator: ternary, comparisons, not, unary signs, member access (.real, .numerator, …) and method calls (.bit_length()). For each tree, Python evaluated the compiled output against a fully parenthesized rendering of the same tree: 0 mismatches. As a check on the harness itself, the first commit of this PR fails about 2,100 trees per seed on the number-before-. bug, and main fails the two ternary rows above.
  • Linting and type checks pass: pnpm lint, pnpm typecheck, tsc --noEmit, eslint and prettier.
  • Tree-sitter backend: compiled output was identical between the TypeScript parser and native tree-sitter (adapted through the parser's adaptNode) on 5,063 inputs. Two unrelated issues already on main got in the way. The parser's built-in tree-sitter entry can't load on Node 22: it require()s an ESM package that uses top-level await. And FORCE_COLOR=3 in the shell breaks binding.gyp's has_scanner check.

Checklist

  • My code follows the project's coding style
  • I have reviewed my own diff
  • I have added/updated documentation as needed (no documentation change needed)
  • This change does not introduce new warnings

awli added 2 commits October 15, 2026 11:40
Compiled expressions are Python source, but the compiler recovered
grouping by looking for parenthesized_expression nodes around each
operand in the CST. That missed ternary operands and the targets of
member access, subscript and call, and it cannot work for ASTs built
without a CST.

Add parentheses purely from the shape of the tree, using Python's
precedence table: ternary < or < and < not < comparison < additive <
multiplicative < unary sign < primary.

- Binary operators are left-associative, so an equal-precedence right
  operand is wrapped (1 - (2 - 3)).
- Python chains comparisons, so a comparison that is an operand of a
  comparison is always wrapped on either side.
- Ternaries are right-associative: only the alternative may be an
  unparenthesized ternary; any other ternary operand is wrapped.
- not wraps operands that bind more loosely than not; unary +/- wrap
  anything below unary.
- Targets of member access, subscript and call must be primary.

The comparison path also copied the source whitespace around the
operator by slicing CST text. Nothing depends on it, so comparisons are
now emitted as `left op right`.

Redundant source parentheses are no longer echoed, e.g. `(1 + 2)` now
compiles to `1 + 2`. Supersedes the CST-based checks from #95; follows
up #94.
…igns

A number literal before `.` lexes as a float (`1.real`), so it is now
parenthesized. Negative literals built in code print with a leading `-` and
now rank as unary, and a signed operand of unary +/- is always wrapped so
`-(-1)` no longer prints as `--1`.

Regenerate the two golden fixtures whose compiled conditions change: one
drops parentheses made redundant by and-over-or precedence, the other drops
doubled spaces before `is not`. Reword a compile-collect comment that said
the expression emitter never adds parentheses.

@BichengWang BichengWang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Preserve the existing behavior of unparenthesized comparison chains

At packages/compiler/src/expressions/compile-expression.ts:558–560, always wrapping a comparison operand changes accepted source programs. I saw the behavior-change note in the description, but this also silently enables out-of-range guards: 0 < @variables.amount < 10 compiles to (0 < state.amount) < 10, which evaluates to True for amount = 100 (and also for negative amounts). The base emits 0 < state.amount < 10, which correctly evaluates to False for those values.

I reproduced this through parse → lint → compile with no diagnostics:

config:
    agent_name: "TestBot"
    agent_type: "AgentforceServiceAgent"
access:
    default_agent_user: "test@example.com"
variables:
    amount: mutable number = 100
    allowed: mutable boolean = False
start_agent main:
    description: "Test"
    reasoning:
        instructions: ->
            if 0 < @variables.amount < 10:
                set @variables.allowed = True
            | Test

The emitted AgentScriptInternal_condition_1 is (0 < state.amount) < 10, so the runtime takes the branch and sets allowed to True. There is a second failure mode: "a" < "b" < "c" changes from True to a Python TypeError because the outer comparison receives a boolean and a string.

Please preserve unparenthesized chains while keeping explicit grouping such as (3 > 2) > 1. The current AST shape conflates these cases, but the original CST still distinguishes them; retaining that distinction in parsing or emission would avoid changing the language behavior as part of this printing fix. Add source-to-runtime regression cases for numeric range guards and string chains; comparing emitted code only against a fully parenthesized rendering of the AST cannot catch this loss of source semantics.

Reviewed 0498226aaad0ff7232b16fb40d2a2b60c8138824 against ace15eb2dc75564fd9e214385ac045c539056770. Both JavaScript and native tree-sitter parsing reproduced the expression regressions, with Python evaluating the base and head outputs; explicitly parenthesized comparison controls retained their behavior. Offline validation: compiler tests 1,167 passed / 3 skipped, SDK tests 320 passed, compiler typecheck and changed-TypeScript ESLint passed. The normal Turbo build was blocked by the missing tree-sitter CLI; the compiler dependencies and SDK were built separately, and native checks used the installed binding directly.

This branch has not been deployed

No deployments
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.

2 participants