From e367efd9455d631540522f17c801572daa91af6b Mon Sep 17 00:00:00 2001 From: Ori Ziv Date: Tue, 4 Aug 2026 05:18:45 +0300 Subject: [PATCH] fix(semantic): reject a trailing macro repetition separator like rustc A repetition's separator only ever stands between two consecutive groups, so the matcher now consumes it together with the group that follows it, rather than after the group that precedes it. A separator that no group follows is no longer absorbed: it is left in the input for whatever comes after the repetition to match, and if nothing does, the rule does not match. This aligns Cairo with rustc's `macro_rules!`, which rejects `m!(1, 2,)` against `($($x:expr),*)` with "unexpected end of macro invocation". It is a behavior break in both directions, since it can also flip a call from one rule to another: against a macro whose first rule is `($($x:expr),*)` and whose second is `($($x:expr,)*)`, `m!(1, 2,)` used to stop at rule 1 and now falls through to rule 2 - which is what rustc picks too. Trailing-separator tolerance is now spelled explicitly, as in rustc: either `$($x:expr,)*`, putting the separator in the group body, or `$($x:expr),* $(,)?`. The four corelib call sites that relied on the absorbed separator are moved onto a macro that asks for it that way. --- .../test/language_features/macro_test.cairo | 20 ++- .../expr/expansion_test_data/inline_macros | 55 +++++++ .../src/expr/test_data/inline_macros | 135 ++++++++++++++++-- .../src/items/macro_declaration.rs | 29 ++-- 4 files changed, 209 insertions(+), 30 deletions(-) diff --git a/corelib/src/test/language_features/macro_test.cairo b/corelib/src/test/language_features/macro_test.cairo index bf5852da1b2..abfe1b737d4 100644 --- a/corelib/src/test/language_features/macro_test.cairo +++ b/corelib/src/test/language_features/macro_test.cairo @@ -211,22 +211,30 @@ mod repetition_macro_matcher { ($($x:ident) *) => { 444 }; } + // A separator only ever stands between two groups, so a trailing one is not part of the + // repetition and the calls above reject it - as rustc's `macro_rules!` does. A macro that wants + // to accept a trailing separator asks for it, and `$(,)?` is how rustc spells "an optional + // trailing comma". + macro matcher_trailing_comma { + ($($x:expr),* $(,)?) => { 555 }; + } + #[test] fn repetition_macro_matcher() { assert_eq!(matcher_plus!(1), 111); assert_eq!(matcher_plus!(1, 2), 111); - #[cairofmt::skip] - assert_eq!(matcher_plus!(1, 2,), 111); - #[cairofmt::skip] - assert_eq!(matcher_plus!(1,), 111); assert_eq!(matcher_star!(), 222); assert_eq!(matcher_star!(3), 222); assert_eq!(matcher_star!(4, 5), 222); + + assert_eq!(matcher_trailing_comma!(), 555); + assert_eq!(matcher_trailing_comma!(1), 555); + assert_eq!(matcher_trailing_comma!(4, 5), 555); #[cairofmt::skip] - assert_eq!(matcher_star!(3,), 222); + assert_eq!(matcher_trailing_comma!(3,), 555); #[cairofmt::skip] - assert_eq!(matcher_star!(4, 5,), 222); + assert_eq!(matcher_trailing_comma!(4, 5,), 555); assert_eq!(matcher_optional!(), 333); assert_eq!(matcher_optional!(9), 333); diff --git a/crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros b/crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros index 3bc0e391607..1435557fff7 100644 --- a/crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros +++ b/crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros @@ -1131,3 +1131,58 @@ error[E2200]: Plugin diagnostic: Macro cannot be parsed as legacy macro. Expecte --> lib.cairo:12:16 let expanded = m!(A and B); ^^^^^^^^^^^ + +//! > ========================================================================== + +//! > Test a trailing separator picking the rule that asks for it. + +//! > test_runner_name +test_expand_expr(expect_diagnostics: false) + +//! > module_code +// The two rules differ only in where the comma sits: rule 1 declares it as the repetition's +// separator, so it only ever stands between two groups, while rule 2 makes it part of the group's +// own body, so every group carries one - which is how rustc spells trailing-separator tolerance. +// A call with a trailing comma therefore falls through rule 1 to rule 2, and one without it stops +// at rule 1. This call used to stop at rule 1 and expand to `1`. rustc's `macro_rules!` picks rule +// 2 for it as well, printing `2`. +#[feature("user_defined_inline_macros")] +macro m { + ($($x:expr),*) => { 1 }; + + ($($x:expr,)*) => { 2 }; +} + +//! > expr_code +let expanded = m!(1, 2,); + +//! > expanded_code +expanded: 2 + +//! > diagnostics + +//! > ========================================================================== + +//! > Test a trailing `;` separator picking the rule that asks for it. + +//! > test_runner_name +test_expand_expr(expect_diagnostics: false) + +//! > module_code +// The comma case above with `;` as the separator, to pin that the rule choice is driven by the +// declared separator rather than by the comma being special. This call used to stop at rule 1 and +// expand to `1`. rustc's `macro_rules!` picks rule 2 for it as well, printing `2`. +#[feature("user_defined_inline_macros")] +macro m { + ($($x:expr);*) => { 1 }; + + ($($x:expr;)*) => { 2 }; +} + +//! > expr_code +let expanded = m!(1; 2;); + +//! > expanded_code +expanded: 2 + +//! > diagnostics diff --git a/crates/cairo-lang-semantic/src/expr/test_data/inline_macros b/crates/cairo-lang-semantic/src/expr/test_data/inline_macros index e73f35d1734..07c320818f2 100644 --- a/crates/cairo-lang-semantic/src/expr/test_data/inline_macros +++ b/crates/cairo-lang-semantic/src/expr/test_data/inline_macros @@ -3011,29 +3011,44 @@ fn foo() -> felt252 { //! > ========================================================================== -//! > Test a trailing separator absorbed by a repetition (deliberate divergence from rustc). +//! > Test a trailing separator rejected by a repetition, as rustc does. //! > test_runner_name -test_function_diagnostics(expect_diagnostics: false) +test_function_diagnostics(expect_diagnostics: true) //! > cairo_code -// The trailing separator is absorbed by the repetition, and the tuple type pins that it does not -// add an empty match. rustc's `macro_rules!` rejects the equivalent call, and aligning with it is a -// separate, deliberate decision. +// A separator only stands between two groups, so the trailing one belongs to no group and is left +// over - and nothing in the rule can match it. rustc's `macro_rules!` rejects the two-element call +// with "unexpected end of macro invocation" and the lone-separator call with "no rules expected +// `,`". The two-element call is the one that changed: it used to be accepted, with the trailing +// separator absorbed. macro m { ($($x:ident),*) => { ($($x),*) }; } fn foo() { - let long_named_first_argument_of_the_macro_call: felt252 = 1; - let long_named_second_argument_of_the_macro_call: felt252 = 2; - // The call is spread over several lines on purpose: the formatter strips a trailing separator - // from a single-line macro call, which would silently drop what this test pins. - let _t: (felt252, felt252) = m!( - long_named_first_argument_of_the_macro_call, long_named_second_argument_of_the_macro_call, - ); + let a: felt252 = 1; + let b: felt252 = 2; + // The `cairofmt::skip` is load-bearing: the formatter strips a trailing comma from a + // single-line macro call, which would silently drop what this test pins. + #[cairofmt::skip] + let _t = m!(a, b,); + // A lone separator is not a group either, so it is rejected for the same reason. + let _u = m!(,); + // Neither rejection is about the call having two elements or none: the same rule accepts these. + let _v: (felt252, felt252) = m!(a, b); + let _w: () = m!(); } //! > expected_diagnostics +error[E2158]: No matching rule found in inline macro `m`. + --> lib.cairo:15:14 + let _t = m!(a, b,); + ^^^^^^^^^ + +error[E2158]: No matching rule found in inline macro `m`. + --> lib.cairo:17:14 + let _u = m!(,); + ^^^^^ //! > ========================================================================== @@ -4021,3 +4036,99 @@ error[E2209]: Macro placeholder 'x' captures an expression, so the pattern may o --> lib.cairo:6:16 ($($x:expr)|*) => { 0 $(+ $x)* }; ^ + +//! > ========================================================================== + +//! > Test a trailing `;` separator rejected by a repetition, as rustc does. + +//! > test_runner_name +test_function_diagnostics(expect_diagnostics: true) + +//! > cairo_code +// The separator being `;` rather than `,` changes nothing: a trailing one still belongs to no +// group. rustc's `macro_rules!` rejects the two-element call with "unexpected end of macro +// invocation" and the lone-separator call with "no rules expected `;`". The two-element call is the +// one that changed: it used to be accepted, with the trailing separator absorbed. +macro m { + ($($x:ident);*) => { ($($x),*) }; +} +fn foo() { + let a: felt252 = 1; + let b: felt252 = 2; + // Unlike a trailing comma, a trailing `;` survives the formatter, so no `cairofmt::skip` here. + let _t = m!(a; b;); + // A lone separator is not a group either, so it is rejected for the same reason. + let _u = m!(;); + // Neither rejection is about the call having two elements or none: the same rule accepts these. + let _v: (felt252, felt252) = m!(a; b); + let _w: () = m!(); +} + +//! > expected_diagnostics +error[E2158]: No matching rule found in inline macro `m`. + --> lib.cairo:12:14 + let _t = m!(a; b;); + ^^^^^^^^^ + +error[E2158]: No matching rule found in inline macro `m`. + --> lib.cairo:14:14 + let _u = m!(;); + ^^^^^ + +//! > ========================================================================== + +//! > Test which rule a trailing separator picks, across every group count. + +//! > test_runner_name +test_function_diagnostics(expect_diagnostics: false) + +//! > cairo_code +// Rule 1 declares the comma as the repetition's separator, so it only ever stands between two +// groups; rule 2 makes it part of the group body, so every group carries one - which is how rustc +// spells trailing-separator tolerance. rustc's `macro_rules!` prints `(1, 2, 2, 1)` for these four +// calls against the equivalent macro. The two trailing-comma calls are what changed: both used to +// stop at rule 1. +macro m { + ($($x:expr),*) => { 1_u8 }; + + ($($x:expr,)*) => { 2_u16 }; +} +fn foo() { + // Each annotation is the assertion: rule 1 yields a `u8` and rule 2 a `u16`, so a call that + // picked the other rule would be a type mismatch here. The two `cairofmt::skip`s keep the + // formatter from stripping the trailing comma this test is about. + let _a: u8 = m!(1, 2); + #[cairofmt::skip] + let _b: u16 = m!(1, 2,); + #[cairofmt::skip] + let _c: u16 = m!(1,); + let _d: u8 = m!(); +} + +//! > expected_diagnostics + +//! > ========================================================================== + +//! > Test which rule a trailing `;` separator picks, across every group count. + +//! > test_runner_name +test_function_diagnostics(expect_diagnostics: false) + +//! > cairo_code +// The comma case above with `;` as the separator, to pin that the rule choice is driven by the +// declared separator rather than by the comma being special. rustc's `macro_rules!` prints +// `(1, 2, 2, 1)` for these four calls against the equivalent macro. The two trailing-`;` calls are +// what changed: both used to stop at rule 1. +macro m { + ($($x:expr);*) => { 1_u8 }; + + ($($x:expr;)*) => { 2_u16 }; +} +fn foo() { + let _a: u8 = m!(1; 2); + let _b: u16 = m!(1; 2;); + let _c: u16 = m!(1;); + let _d: u8 = m!(); +} + +//! > expected_diagnostics diff --git a/crates/cairo-lang-semantic/src/items/macro_declaration.rs b/crates/cairo-lang-semantic/src/items/macro_declaration.rs index 5913008151b..2033c7a56e3 100644 --- a/crates/cairo-lang-semantic/src/items/macro_declaration.rs +++ b/crates/cairo-lang-semantic/src/items/macro_declaration.rs @@ -954,6 +954,23 @@ fn is_macro_rule_match_ex<'db>( loop { let mut inner_ctx = ctx.clone(); let mut temp_iter = input_iter.clone(); + // A separator only ever stands *between* two groups, so from the second group + // on it has to be consumed before the group - and only together with it. A + // separator that no group follows is not part of the repetition, and is left + // in the input for whatever comes after it to match, exactly as rustc's + // `macro_rules!` does. + if match_count > 0 + && let Some(expected_sep) = &expected_separator + { + let Some(ast::TokenTree::Token(token_leaf)) = temp_iter.peek() else { + break; + }; + if token_leaf.as_syntax_node().get_text_without_trivia(db) != *expected_sep + { + break; + } + temp_iter.next(); + } let Some(true) = is_macro_rule_match_ex( db, elements.clone(), @@ -967,18 +984,6 @@ fn is_macro_rule_match_ex<'db>( *ctx = inner_ctx; *input_iter = temp_iter; match_count += 1; - if let Some(expected_sep) = &expected_separator { - if let Some(ast::TokenTree::Token(token_leaf)) = input_iter.peek() { - let actual = token_leaf.as_syntax_node().get_text_without_trivia(db); - if actual == *expected_sep { - input_iter.next(); - } else { - break; - } - } else { - break; - } - } } ctx.repetition_match_counts.insert(rep_id, match_count); ctx.repetition_operators.insert(rep_id, operator.clone());