feat(semantic): match and emit non-comma macro repetition separators - #10312
Conversation
fe3b861 to
82bf327
Compare
3ca8704 to
c863a6e
Compare
82bf327 to
7002ac1
Compare
c863a6e to
bec3c71
Compare
7002ac1 to
96e7d61
Compare
PR SummaryMedium Risk Overview Calls like Declaration diagnostics routed through the same helper now fire for non-comma cases too: E2206 on New expansion and diagnostics goldens cover Reviewed by Cursor Bugbot for commit 5300e32. Bugbot is set up for automated code reviews on this repo. Configure here. |
96e7d61 to
9b11f1c
Compare
bec3c71 to
1ae8fbe
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9b11f1c. Configure here.
9b11f1c to
e612131
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi+AGNT made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.
e612131 to
b85c935
Compare
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>
b85c935 to
5300e32
Compare


The parser already accepts any single token in a repetition's separator slot, but
repetition_separatorstill returnedNonefor anything but a comma, so thesemantic side silently ignored it:
$($x:ident);*matchedm!(a b)and did notmatch
m!(a; b). The helper now returns the declared token as written, and everyreader of it - the matcher, the expansion, the
?-operator check and theexprfollow-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
SyntaxNoderather 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:
?block, now fires for$($x:ident);?. A separatoronly ever appears between two groups and
?allows at most one, and now thatthe matcher consumes the
;this is exactly the hole the check exists to close.rustc reports "the
?macro repetition operator does not take a separator" forthe same pattern.
exprcapture followed by a token that would swallow the pattern'sown, 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:exprisfollowed by
|, which is not allowed forexprfragments; 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 asm!(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 nodeconflicted in one region ofmacro_declaration.rs: thesemantic stack had replaced the whole
expand_macro_rule_exfree function with theExpansionContextstruct that the parser track's diff was anchored to. Theresolution keeps
ExpansionContextverbatim and hand-ports the parser track's twointents onto it - the
repetition_separatorhelper, moved next tocheck_repetition_separatorsincefind_first_repetition_param, the function ithad been placed before, no longer exists - and the three remaining
ast::OptionTerminalCommareaders that the semantic stack had added or moved:check_repetition_separator,check_expr_follow_setandExpansionContext::expand_repetition. The fourth reader, inis_macro_rule_match_ex, applied cleanly.feat(parser): accept any single-token macro repetition separatorapplied with no conflict. No golden moved during therestack.
Goldens
Four expansion goldens (expr position,
test_expand_expr) and three diagnosticsgoldens, 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.$($($x:ident)+);*, which only a non-comma separator makesexpressible, 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)).parts!([] [A; B])expanding to((), (A,B)); rustc prints((), (1, 2)). Thetwo-group list is what makes this golden discriminate: an empty list alone
matches a rule that ignores its separator just as well.
,, and a call writing nothing, where the rule writes;- bothreport E2158
No matching rule found, not a panic. The separator-lessm!(a b)call is the discriminator here for the same reason: the
,call was alreadyrejected before this change, by the leftover-input check.
Every one of the seven fails with only this change reverted - that is, with
repetition_separatorput back to its comma-only body on top of this same stack: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
.cairofile undercrates/,corelib/ortests/declares arepetition 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) andcairo-test tests/bug_samples --starknet(69 passed) are all green.Co-Authored-By: Claude Fable 5 noreply@anthropic.com