Skip to content

feat(semantic): match and emit non-comma macro repetition separators - #10312

Open
orizi wants to merge 1 commit into
graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enablefrom
graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion
Open

orizi wants to merge 1 commit into
graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enablefrom
graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion

Conversation

@orizi

@orizi orizi commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

The parser already accepts any single token in a repetition's separator slot, but
repetition_separator still returned None for anything but a comma, so the
semantic side silently ignored it: $($x:ident);* matched m!(a b) and did not
match m!(a; b). The helper now returns the declared token as written, and every
reader of it - the matcher, the expansion, the ?-operator check and the expr
follow-set check - honors it.

The matcher and the expansion needed no change of their own: the matcher already
compared the separator by text against the next call token, and the expansion
already pushed the separator's text between groups. Both were reached only for a
comma; both are now reached for any token. The helper returns a SyntaxNode
rather than a typed terminal, as no reader looks at which token it is - they take
its text, or its stable pointer to report on.

Two declaration checks change with it, which is the point of routing them through
the same helper rather than a scope slip:

  • E2206, a separator on a ? block, now fires for $($x:ident);?. A separator
    only ever appears between two groups and ? allows at most one, and now that
    the matcher consumes the ; this is exactly the hole the check exists to close.
    rustc reports "the ? macro repetition operator does not take a separator" for
    the same pattern.
  • E2209, an expr capture followed by a token that would swallow the pattern's
    own, now sees a non-comma separator as a follower. $($x:expr)|* is rejected,
    $($x:expr);* is not, as ; is in the follow set. rustc reports "$x:expr is
    followed by |, which is not allowed for expr fragments; allowed there are:
    =>, , or ;".

The trailing-separator behavior is byte-identical to before this PR. The matcher
loop that absorbs a trailing separator is untouched - it consumes a separator and
then fails to match another group, exactly as it did for a comma - so a call
writing m!(a; b;) against $($x:ident);* is accepted just as m!(a, b,) was.
F13, aligning that with rustc's rejection, is deliberately left for the next PR;
the existing golden "Test a trailing separator absorbed by a repetition
(deliberate divergence from rustc)" still passes unchanged.

Restack of the parser track onto the semantic stack

This branch is the head of the capture-fidelity sub-stack (72de34a80) with the two
parser-track commits cherry-picked on top. refactor(syntax): generalize the macro repetition separator node conflicted in one region of macro_declaration.rs: the
semantic stack had replaced the whole expand_macro_rule_ex free function with the
ExpansionContext struct that the parser track's diff was anchored to. The
resolution keeps ExpansionContext verbatim and hand-ports the parser track's two
intents onto it - the repetition_separator helper, moved next to
check_repetition_separator since find_first_repetition_param, the function it
had been placed before, no longer exists - and the three remaining
ast::OptionTerminalComma readers that the semantic stack had added or moved:
check_repetition_separator, check_expr_follow_set and
ExpansionContext::expand_repetition. The fourth reader, in
is_macro_rule_match_ex, applied cleanly. feat(parser): accept any single-token macro repetition separator applied with no conflict. No golden moved during the
restack.

Goldens

Four expansion goldens (expr position, test_expand_expr) and three diagnostics
goldens, every one of them cross-checked against rustc's macro_rules!:

  • $($x:expr);* expanding $(total = total + $x);*; into two statements -
    totals!(1 + 2; 3); rustc prints 6.
  • $($x:ident)|* expanding $($x)|* - bits!(A | B); rustc prints 3.
  • the nested $($($x:ident)+);*, which only a non-comma separator makes
    expressible, with two captures per group and two outer groups -
    nested!(A B; C D) expanding to ((A,B),(C,D)); rustc prints ((1, 2), (4, 8)).
  • a zero-match list beside a two-group list in one call -
    parts!([] [A; B]) expanding to ((), (A,B)); rustc prints ((), (1, 2)). The
    two-group list is what makes this golden discriminate: an empty list alone
    matches a rule that ignores its separator just as well.
  • a call writing ,, and a call writing nothing, where the rule writes ; - both
    report E2158 No matching rule found, not a panic. The separator-less m!(a b)
    call is the discriminator here for the same reason: the , call was already
    rejected before this change, by the leftover-input check.
  • the two declaration checks above.

Every one of the seven fails with only this change reverted - that is, with
repetition_separator put back to its comma-only body on top of this same stack:

test expr::test::expand_inline_macros::inline_macros ... FAILED
test expr::test::expr_diagnostics::inline_macros ... FAILED
Test "Test a `;`-separated repetition matched and expanded with its separator." failed.
  `expect_diagnostics` is false, but diagnostics were generated
Test "Test a repetition separated by a token that is neither a comma nor a semicolon." failed.
Test "Test a nested repetition whose outer groups a `;` separates." failed.
Test "Test a `;`-separated repetition matching no input at all." failed.
Test "Test a call separating groups with `,` where the rule separates them with `;`." failed.
Test "Test a `?` pattern repetition taking a non-comma separator." failed.
Test "Test an expr placeholder followed by a separator outside its follow set." failed.
test result: FAILED. 1 passed; 2 failed

No pre-existing golden fails in that run, and none churned when blessing.

Because the two widened declaration checks can reach macro declarations written in
test data of other crates, the churn claim was checked beyond the three gate
crates: no .cairo file under crates/, corelib/ or tests/ declares a
repetition with a non-comma separator, and cairo-lang-starknet,
cairo-lang-plugins, cairo-lang-lowering, cairo-lang-sierra-generator,
cairo-lang-defs, cairo-lang-compiler, cairo-lang-formatter, cairo-lang-doc,
tests, cairo-test corelib (737 passed) and cairo-test tests/bug_samples --starknet (69 passed) are all green.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

orizi commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from fe3b861 to 82bf327 Compare August 4, 2026 08:41
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 3ca8704 to c863a6e Compare August 4, 2026 08:56
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 82bf327 to 7002ac1 Compare August 4, 2026 08:56
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from c863a6e to bec3c71 Compare August 5, 2026 10:54
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 7002ac1 to 96e7d61 Compare August 5, 2026 10:54
@orizi
orizi marked this pull request as ready for review August 5, 2026 11:10
@cursor

cursor Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes macro matching/expansion behavior for user-defined inline macros; incorrect separator handling could break macro-heavy code, but scope is localized to semantic macro expansion with extensive golden tests.

Overview
Macro repetition separators (;, |, word tokens, etc.) are now honored end-to-end in matching and expansion, not only commas. Calls must use the separator declared in the rule (e.g. m!(a; b) for $($x:ident);*), and expansions emit that token between groups—including cases that turn ; into real statement boundaries in the expanded code.

repetition_separator is wired through the same path for the matcher, expansion, ?-separator checks (E2206), and expr follow-set checks (E2209). In expand_repetition, the separator is resolved once per block, and leading trivia before the closing ) is copied into the output when writing separators so author spacing around separators is preserved.

New inline-macro expansion and diagnostic goldens cover ;/|/nested/empty repetitions, separator mismatch (E2158), and identifier separators (including unspaced glue vs rustc).

Reviewed by Cursor Bugbot for commit f0ac551. Bugbot is set up for automated code reviews on this repo. Configure here.

@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 96e7d61 to 9b11f1c Compare August 5, 2026 11:59
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from bec3c71 to 1ae8fbe Compare August 5, 2026 11:59
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 21f5d10 to 1b02458 Compare August 17, 2026 19:07
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from cbd3fa7 to 55f3321 Compare August 17, 2026 19:07
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 1b02458 to 1e738c8 Compare August 23, 2026 08:43
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch 2 times, most recently from 01fb5a0 to ecd166f Compare August 23, 2026 08:50
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 1e738c8 to 4caf608 Compare August 23, 2026 08:50
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from ecd166f to 57ebb82 Compare August 23, 2026 10:31
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 4caf608 to 9563a80 Compare August 23, 2026 10:31
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 9563a80 to 7b603d8 Compare August 23, 2026 11:22
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 57ebb82 to 31efc88 Compare August 23, 2026 11:22

@orizi orizi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@orizi+AGNT made 4 comments and resolved 4 discussions.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on TomerStarkware).


crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros line 999 at r4 (raw file):

Previously, eytan-starkware+AGNT (Agent AGNT for eytan-starkware) wrote…

this comment is wrong on both counts. the E2200 isn't on the m! call and isn't unrelated to separators - it's the array! in your expansion: A and B doesn't parse as an argument list, exactly like array![format!] in "Test macro calls that are not expanded due to errors." below. if the legacy path reported on any user-macro call with non-arg-list arguments, nested!(A B; C D) and parts!([] [A; B]) above would report too, and they don't. fix the comment (or drop the array! sink).

Rewritten - the comment now points at the array! in the expansion rejecting A and B, mirroring the errors test, with the location mapped back to the call.


crates/cairo-lang-semantic/src/expr/test_data/inline_macros line 3886 at r4 (raw file):

Previously, eytan-starkware+AGNT (Agent AGNT for eytan-starkware) wrote…

this is the same rule and the same diagnostic as "Test a ? repetition declaring a non-comma separator (rejected)." ~70 lines up - literally ($($x:ident);?) in both. check_repetition_separator read declared_separator_token, which was already any-token, so E2206 behaves identically before and after this PR. pins nothing this PR changes - remove.

Dropped - with the widening folded into #10311 the golden there covers it.


crates/cairo-lang-semantic/src/expr/test_data/inline_macros line 3912 at r4 (raw file):

Previously, eytan-starkware+AGNT (Agent AGNT for eytan-starkware) wrote…

same - duplicates "Test an expr placeholder whose repetition declares a separator outside its follow set." above, with | instead of .. check_expr_follow_set already pushed the declared token of any kind as a follower, so E2209 is unchanged by this PR. remove.

Dropped for the same reason.


crates/cairo-lang-semantic/src/items/macro_declaration.rs line 1268 at r4 (raw file):

Previously, eytan-starkware+AGNT (Agent AGNT for eytan-starkware) wrote…

you're guessing at spacing you already have. the separator's leading trivia is the repetition rparen's trailing trivia, and push_trailing_trivia exists for exactly this case. the char-class guess doesn't close the hole either (| still fuses with a body ending in |), and the second push(' ') is unconditional - it doesn't look at what follows at all.

all five new goldens come out byte-identical with this (every one of them writes the separator right after the ), or with the space the author wrote):

        let db = self.db;
        let group_count = self.group_count(repetition)?;
        let elements = repetition.elements(db);
        let separator = repetition_separator(db, repetition);
        for index in 0..group_count {
            self.group_indices.push(index);
            let expanded = elements
                .elements(db)
                .try_for_each(|element| self.expand_node(element.as_syntax_node()));
            self.group_indices.pop();
            expanded?;
            if index + 1 < group_count && let Some(sep) = separator {
                // The separator's leading trivia sits on the repetition's closing `)`, which is
                // not emitted - so the spacing written before it is pushed from there.
                self.push_trailing_trivia(repetition.rparen(db).as_syntax_node());
                self.res_buffer.push_str(sep.get_text(db));
            }
        }

note it also hoists the lookup out of the loop - it's the same node for every group, the matcher already computes it once.

Taken as suggested - the separator is spaced by the rparen's trailing trivia via push_trailing_trivia, the lookup is hoisted out of the loop, and the goldens came out byte-identical.

@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 31efc88 to 0384589 Compare August 23, 2026 12:03
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch 2 times, most recently from 0a4b462 to 73b293c Compare August 25, 2026 12:30
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch 2 times, most recently from 4cdeccf to 38af632 Compare August 26, 2026 19:59
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 73b293c to 5ac6128 Compare August 26, 2026 19:59
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 38af632 to eab3c7e Compare August 27, 2026 04:32
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 38045cb to a81aa41 Compare August 27, 2026 13:33
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from eab3c7e to 8a7c04a Compare August 27, 2026 13:33
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from a81aa41 to b3df58f Compare August 28, 2026 10:07
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 8a7c04a to 4b72f9c Compare August 28, 2026 10:07
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from b3df58f to fc2cc2f Compare August 28, 2026 12:58
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 4b72f9c to 34fa52b Compare August 28, 2026 12:58
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from fc2cc2f to 74e1905 Compare September 14, 2026 04:51
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 34fa52b to 4b49444 Compare September 14, 2026 04:51
The previous commit widened `repetition_separator` to return whatever token the
rule declares, with every reader honoring it. This commit fixes how the
expansion spaces the emitted separator, and adds the goldens for the widened
behavior.

The separator's text carries its trailing trivia but not its leading trivia -
that sits on the repetition's closing `)`, which is not emitted. The expansion
now pushes the rparen's trailing trivia before the separator, so the separator
is spaced exactly as the rule's author wrote it, and the separator lookup is
hoisted out of the per-group loop, where it re-read the same node once per
group. Spacing the author did not write is not invented: `$($x)and*` with no
spaces glues the groups into one identifier, pinned as a known divergence from
rustc, which keeps them separate tokens.

Goldens, cross-checked against rustc's `macro_rules!`:

* `$($x:expr);*` expanding `$(total = total + $x);*;` into two statements -
  `totals!(1 + 2; 3)`; rustc prints 6.
* `$($x:ident)|*` expanding `$($x)|*` - `bits!(A | B)`; rustc prints 3.
* the nested `$($($x:ident)+);*`, which only a non-comma separator makes
  expressible, with two captures per group and two outer groups -
  `nested!(A B; C D)` expanding to `((A,B),(C,D))`; rustc prints
  `((1, 2), (4, 8))`.
* a zero-match list beside a two-group list in one call -
  `parts!([] [A; B])` expanding to `((), (A,B))`; rustc prints `((), (1, 2))`.
  The two-group list is what makes this golden discriminate: an empty list
  alone matches a rule that ignores its separator just as well.
* a call writing `,`, and a call writing nothing, where the rule writes `;` -
  both report E2158 `No matching rule found`, not a panic. The separator-less
  `m!(a b)` call is the discriminator here for the same reason: the `,` call
  was already rejected before this change, by the leftover-input check.
* an identifier separator spaced as the rule wrote it (`$($x) and *`), and its
  unspaced counterpart gluing into one identifier - the divergence pin above.

The trailing-separator behavior is byte-identical to before this PR. The
matcher loop that absorbs a trailing separator is untouched - it consumes a
separator and then fails to match another group, exactly as it did for a comma
- so a call writing `m!(a; b;)` against `$($x:ident);*` is accepted just as
`m!(a, b,)` was. F13, aligning that with rustc's rejection, is deliberately
left for the next PR; the existing golden "Test a trailing separator absorbed
by a repetition (deliberate divergence from rustc)" still passes unchanged.

Because the widened separator behavior can reach macro declarations written in
test data of other crates, the churn claim was checked beyond the three gate
crates: no `.cairo` file under `crates/`, `corelib/` or `tests/` declares a
repetition with a non-comma separator, and `cairo-lang-starknet`,
`cairo-lang-plugins`, `cairo-lang-lowering`, `cairo-lang-sierra-generator`,
`cairo-lang-defs`, `cairo-lang-compiler`, `cairo-lang-formatter`,
`cairo-lang-doc`, `tests`, `cairo-test corelib` (737 passed) and `cairo-test
tests/bug_samples --starknet` (69 passed) are all green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-matcher-expansion branch from 4b49444 to f0ac551 Compare September 14, 2026 06:56
@orizi
orizi force-pushed the graph-plan/2026-08-03-macro-fixes/separator-grammar.sep-parser-enable branch from 74e1905 to d1c5cae Compare September 14, 2026 06:56
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.

3 participants