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
New inline-macro expansion and diagnostic goldens cover Reviewed by Cursor Bugbot for commit f0ac551. Bugbot is set up for automated code reviews on this repo. Configure here. |
96e7d61 to
9b11f1c
Compare
bec3c71 to
1ae8fbe
Compare
21f5d10 to
1b02458
Compare
cbd3fa7 to
55f3321
Compare
1b02458 to
1e738c8
Compare
01fb5a0 to
ecd166f
Compare
1e738c8 to
4caf608
Compare
ecd166f to
57ebb82
Compare
4caf608 to
9563a80
Compare
9563a80 to
7b603d8
Compare
57ebb82 to
31efc88
Compare
orizi
left a comment
There was a problem hiding this comment.
@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 thearray!in your expansion:A and Bdoesn't parse as an argument list, exactly likearray![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)andparts!([] [A; B])above would report too, and they don't. fix the comment (or drop thearray!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_separatorreaddeclared_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_setalready 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_triviaexists for exactly this case. the char-class guess doesn't close the hole either (|still fuses with a body ending in|), and the secondpush(' ')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.
31efc88 to
0384589
Compare
0a4b462 to
73b293c
Compare
4cdeccf to
38af632
Compare
73b293c to
5ac6128
Compare
38af632 to
eab3c7e
Compare
38045cb to
a81aa41
Compare
eab3c7e to
8a7c04a
Compare
a81aa41 to
b3df58f
Compare
8a7c04a to
4b72f9c
Compare
b3df58f to
fc2cc2f
Compare
4b72f9c to
34fa52b
Compare
fc2cc2f to
74e1905
Compare
34fa52b to
4b49444
Compare
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>
4b49444 to
f0ac551
Compare
74e1905 to
d1c5cae
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