fix(semantic): reject the malformed macro rule declarations that silently do nothing

[orizi]

A pattern capturing into $defsite / $callsite, a ? repetition taking a
separator and an empty $() body are all accepted today and then contribute
nothing, and a rule whose pattern has a parse error is dropped silently, so
every call written for it fails with a bare "no matching rule".

[orizi]

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

• fix(semantic): reject a + expansion repetition over zero groups #10315
• doc(semantic): document the macro expansion error arms as defensive #10314
• fix(semantic): reject a trailing macro repetition separator like rustc #10313
• feat(semantic): match and emit non-comma macro repetition separators #10312
• feat(parser): accept any single-token macro repetition separator #10311
• refactor(syntax): generalize the macro repetition separator node #10310
• fix(semantic): parenthesize an expression capture spliced into an expansion #10309
• fix(semantic): keep the trailing trivia of an expansion's placeholders and blocks #10308
• fix(semantic): capture an expr placeholder by what the parser consumed #10307
• test(semantic): pin the macro matcher behaviors that already agree with rustc #10306
• fix(semantic): reject a pattern that follows an expr placeholder with a token it would swallow #10305
• fix(semantic): reject the malformed macro rule declarations that silently do nothing #10304 👈 (View in Graphite)
• fix(semantic): match a pattern subtree only against a call subtree with the same delimiters #10303
• fix(semantic): reject a macro rule pattern that reuses a placeholder name #10302
• fix(semantic): expand a macro repetition over its own group of captures #10301
• test(semantic): pin the macro repetition operator and trailing separator behaviors #10300
• refactor(semantic): build a nested capture tree alongside the flat macro captures #10299
• fix(semantic): reject a macro expansion block with nothing repeating at its depth #10298
• fix(formatter): stop adding trailing separators in macro call token trees #10354
• main

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

[reviewable-StarkWare]

This change is 

[orizi]

the explaination seems like too much.

[orizi]

Shortened both messages to the bare statement.

[orizi]

Shortened to the bare statement.

[orizi]

the explaination seems like too much.

[orizi]

Shortened.

[orizi]

Shortened to the bare statement.

[cursor]

PR Summary

Medium Risk
Changes macro declaration and rule matching behavior; invalid rules now error at definition time and no longer participate in matching, which may surface new errors in existing code that relied on silent dead rules.

Overview
Adds declaration-time validation for user-defined inline macro rules so several malformed patterns no longer fail silently at call sites with only “no matching rule”.

Macro declaration analysis now walks pattern repetitions and subtrees to reject empty $() bodies (E2207), ? repetitions with separators in both pattern and expansion (E2206), and placeholders named exactly $defsite / $callsite (E2205). Rules whose pattern still has parse errors emit E2208 and are excluded from matching instead of being dropped quietly. Expansion placeholder checks are skipped when the pattern has modifier-named or duplicate placeholders so secondary driver errors are not reported.

New diagnostics E2205–E2208 are wired in diagnostic.rs; inline_macros tests cover these cases and adjust the infinite-loop guard test now that ($()* is rejected at declaration time.

^{Reviewed by Cursor Bugbot for commit 8372386. Bugbot is set up for automated code reviews on this repo. Configure here.}

[eytan-starkware]

@eytan-starkware+AGNT made 7 comments.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on eytan-starkware, orizi, and TomerStarkware).

---

a discussion (no related file):
Note: the comments below are from an automatic orizi-review run (Claude agents reviewing in Ori's style, findings adversarially verified before posting). Treat with the usual bot skepticism.

---

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

//! > ==========================================================================

//! > Test a pattern capturing into the name `$defsite`.

every new case here is a top-level pattern element. check_pattern_elements recurses into repetitions and subtrees and none of that recursion is pinned - add ($($defsite:ident)*), ([$defsite:ident]) and a ?-with-separator nested inside an outer repetition.

---

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

/// `Err` if any was reported, for the caller to mark the rule with - a defective element makes the
/// rule's meaning unclear, so it must not expand.
fn check_pattern_elements<'db>(

third full traversal of the pattern with the same four match arms - PatternPlaceholders::collect right below already visits every Param/Repetition/Subtree and already computes the placeholder name (so check_placeholder_name re-interns it). fold the name check into collect - it has both the name and the ptr - and this shrinks to the two repetition checks.

---

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

    diagnostics: &mut SemanticDiagnostics<'db>,
) -> Maybe<()> {
    let ast::OptionTerminalComma::TerminalComma(separator) = repetition.separator(db) else {

let-chain:

    if let ast::OptionTerminalComma::TerminalComma(separator) = repetition.separator(db)
        && matches!(repetition.operator(db), ast::MacroRepetitionOperator::ZeroOrOne(_))
    {
        return Err(diagnostics.report(
            separator.stable_ptr(db).untyped(),
            SemanticDiagnosticKind::MacroRepetitionSeparatorWithZeroOrOne,
        ));
    }
    Ok(())

---

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

        // which a reused name leaves ambiguous - checking it then reports the ambiguity as
        // a defect of the expansion, which it is not.
        if placeholders.reused_names.is_empty() {

this was rule_err.is_ok(), which for an unparsed pattern was always true, so the expansion was checked. now it's reused_names.is_empty(), which for an unparsed pattern depends on whether recovery happened to leave two nameless placeholders - i.e. a coin flip on whether a dropped rule's expansion gets diagnostics.

make it explicit: hoist the !pattern_is_parsed report + continue to the top of the loop, right after let pattern = rule_syntax.lhs(db);. then the if pattern_is_parsed wrapper, the nameless-placeholders comment above it and this whole question go away.

---

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

            // it. It is reported so that a call written for it points at the parse error rather
            // than failing with a bare "no matching rule".
            diagnostics.report(

the comment says this is so "a call written for it points at the parse error rather than failing with a bare 'no matching rule'" - but it doesn't. look at the pair golden: E1008 from the parser on the same rule, then this E2208 on an overlapping span, then the same E2158 on the call, unchanged. three diagnostics for one typo, and the confusing one is still there.

fix the comment, or make it actually replace the E2158.

---

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

        if let Some(repetition) = ast::MacroRepetition::cast(db, node) {
            self.rule_err =

expand_repetition never looks at the operator - in an expansion ? is just *. so $(...),? on the rhs today expands over all the driver's groups and emits the separator between them, correctly. this call turns working code into an error, and it isn't "silently does nothing" as the PR title claims. the doc's "allows at most one group" rationale only holds for the pattern.

either make the expansion honor ? (fail when the driver has more than one group) and then reject the separator, or keep the check pattern-only and drop this call.

[orizi]

@orizi+AGNT made 6 comments and resolved 5 discussions.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on eytan-starkware+AGNT and TomerStarkware).

---

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

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

every new case here is a top-level pattern element. check_pattern_elements recurses into repetitions and subtrees and none of that recursion is pinned - add ($($defsite:ident)*), ([$defsite:ident]) and a ?-with-separator nested inside an outer repetition.

Added all three: ($($defsite:ident)*), ([$defsite:ident]), and a ,? nested inside an outer repetition.

---

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

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

third full traversal of the pattern with the same four match arms - PatternPlaceholders::collect right below already visits every Param/Repetition/Subtree and already computes the placeholder name (so check_placeholder_name re-interns it). fold the name check into collect - it has both the name and the ptr - and this shrinks to the two repetition checks.

Folded into PatternPlaceholders::collect; check_pattern_elements shrank to the repetition checks.

---

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

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

let-chain:

    if let ast::OptionTerminalComma::TerminalComma(separator) = repetition.separator(db)
        && matches!(repetition.operator(db), ast::MacroRepetitionOperator::ZeroOrOne(_))
    {
        return Err(diagnostics.report(
            separator.stable_ptr(db).untyped(),
            SemanticDiagnosticKind::MacroRepetitionSeparatorWithZeroOrOne,
        ));
    }
    Ok(())

Done.

---

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

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

this was rule_err.is_ok(), which for an unparsed pattern was always true, so the expansion was checked. now it's reused_names.is_empty(), which for an unparsed pattern depends on whether recovery happened to leave two nameless placeholders - i.e. a coin flip on whether a dropped rule's expansion gets diagnostics.

make it explicit: hoist the !pattern_is_parsed report + continue to the top of the loop, right after let pattern = rule_syntax.lhs(db);. then the if pattern_is_parsed wrapper, the nameless-placeholders comment above it and this whole question go away.

Done - report + continue at the top of the loop; the wrapper, the nameless-placeholder comment and the coin flip are gone.

---

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

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

the comment says this is so "a call written for it points at the parse error rather than failing with a bare 'no matching rule'" - but it doesn't. look at the pair golden: E1008 from the parser on the same rule, then this E2208 on an overlapping span, then the same E2158 on the call, unchanged. three diagnostics for one typo, and the confusing one is still there.

fix the comment, or make it actually replace the E2158.

Fixed the comment - it claims only the no-shadowing effect now.

---

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

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

expand_repetition never looks at the operator - in an expansion ? is just *. so $(...),? on the rhs today expands over all the driver's groups and emits the separator between them, correctly. this call turns working code into an error, and it isn't "silently does nothing" as the PR title claims. the doc's "allows at most one group" rationale only holds for the pattern.

either make the expansion honor ? (fail when the driver has more than one group) and then reject the separator, or keep the check pattern-only and drop this call.

Half-taken: the check stays because rustc rejects a separator on a ? expansion block with the same error (the golden quotes it), so dropping it would open a parity gap. What was wrong was the rationale - the doc now says a separator would simply never be emitted there, and the ? promise itself becomes enforced at expansion in the E2210 PR up the stack.