fix: better error cases with bad/missing identifiers in MBEs

Author: EliseZeroTwo

compiler/rustc_parse/messages.ftl

@@ -468,6 +468,8 @@ parse_macro_name_remove_bang = macro names aren't followed by a `!`
 parse_macro_rules_missing_bang = expected `!` after `macro_rules`
     .suggestion = add a `!`
 
+parse_macro_rules_named_macro_rules = user-defined macros may not be named `macro_rules`
+
 parse_macro_rules_visibility = can't qualify macro_rules invocation with `{$vis}`
     .suggestion = try exporting the macro
 
@@ -497,6 +499,8 @@ parse_maybe_fn_typo_with_impl = you might have meant to write `impl` instead of
 
 parse_maybe_missing_let = you might have meant to continue the let-chain
 
+parse_maybe_missing_macro_rules_name = maybe you have forgotten to define a name for this `macro_rules!`
+
 parse_maybe_recover_from_bad_qpath_stage_2 =
     missing angle brackets in associated item path
     .suggestion = types that don't start with an identifier need to be surrounded with angle brackets in qualified paths

compiler/rustc_parse/src/errors.rs

@@ -2664,6 +2664,13 @@ pub(crate) struct MacroRulesMissingBang {
     pub hi: Span,
 }
 
+#[derive(Diagnostic)]
+#[diag(parse_macro_rules_named_macro_rules)]
+pub(crate) struct MacroRulesNamedMacroRules {
+    #[primary_span]
+    pub span: Span,
+}
+
 #[derive(Diagnostic)]
 #[diag(parse_macro_name_remove_bang)]
 pub(crate) struct MacroNameRemoveBang {

compiler/rustc_parse/src/parser/item.rs

@@ -2036,23 +2036,26 @@ impl<'a> Parser<'a> {
 
     /// Is this a possibly malformed start of a `macro_rules! foo` item definition?
     fn is_macro_rules_item(&mut self) -> IsMacroRulesItem {
-        if self.check_keyword(kw::MacroRules) {
-            let macro_rules_span = self.token.span;
-
-            if self.look_ahead(1, |t| *t == token::Not) && self.look_ahead(2, |t| t.is_ident()) {
-                return IsMacroRulesItem::Yes { has_bang: true };
-            } else if self.look_ahead(1, |t| (t.is_ident())) {
-                // macro_rules foo
-                self.sess.emit_err(errors::MacroRulesMissingBang {
-                    span: macro_rules_span,
-                    hi: macro_rules_span.shrink_to_hi(),
-                });
+        if !self.check_keyword(kw::MacroRules) {
+            return IsMacroRulesItem::No;
+        }
+
+        let macro_rules_span = self.token.span;
+        let has_bang = self.look_ahead(1, |t| *t == token::Not);
 
-                return IsMacroRulesItem::Yes { has_bang: false };
+        // macro_rules foo
+        if !has_bang {
+            if !self.look_ahead(1, |t| (t.is_ident())) {
+                return IsMacroRulesItem::No;
             }
+
+            self.sess.emit_err(errors::MacroRulesMissingBang {
+                span: macro_rules_span,
+                hi: macro_rules_span.shrink_to_hi(),
+            });
         }
 
-        IsMacroRulesItem::No
+        IsMacroRulesItem::Yes { has_bang }
     }
 
     /// Parses a `macro_rules! foo { ... }` declarative macro.
@@ -2066,7 +2069,34 @@ impl<'a> Parser<'a> {
         if has_bang {
             self.expect(&token::Not)?; // `!`
         }
-        let ident = self.parse_ident()?;
+
+        let ident = match self.parse_ident() {
+            Ok(ident) => ident,
+            Err(mut err) => {
+                if let TokenKind::OpenDelim(Delimiter::Parenthesis) = self.token.kind
+                    && let Some((Ident { name, .. }, _)) = self.look_ahead(1, |token| token.ident())
+                    && let Some(closing_span) = self.look_ahead(2, |token| {
+                        (token.kind == TokenKind::CloseDelim(Delimiter::Parenthesis))
+                            .then_some(token.span)
+                    })
+                {
+                    err.span_suggestion(
+                        self.token.span.with_hi(closing_span.hi()),
+                        "try removing the parenthesis around the name for this `macro_rules!`",
+                        name,
+                        Applicability::MachineApplicable,
+                    );
+                } else {
+                    err.help(fluent::parse_maybe_missing_macro_rules_name);
+                }
+
+                return Err(err);
+            }
+        };
+
+        if ident.name == sym::macro_rules {
+            self.sess.emit_err(errors::MacroRulesNamedMacroRules { span: ident.span });
+        }
 
         if self.eat(&token::Not) {
             // Handle macro_rules! foo!

compiler/rustc_resolve/messages.ftl

@@ -181,8 +181,6 @@ resolve_method_not_member_of_trait =
     method `{$method}` is not a member of trait `{$trait_}`
     .label = not a member of trait `{$trait_}`
 
-resolve_missing_macro_rules_name = maybe you have forgotten to define a name for this `macro_rules!`
-
 resolve_module_only =
     visibility must resolve to a module
 

compiler/rustc_resolve/src/diagnostics.rs

@@ -28,7 +28,7 @@ use rustc_span::{BytePos, Span, SyntaxContext};
 use thin_vec::{thin_vec, ThinVec};
 
 use crate::errors::{AddedMacroUse, ChangeImportBinding, ChangeImportBindingSuggestion};
-use crate::errors::{ConsiderAddingADerive, ExplicitUnsafeTraits, MaybeMissingMacroRulesName};
+use crate::errors::{ConsiderAddingADerive, ExplicitUnsafeTraits};
 use crate::imports::{Import, ImportKind};
 use crate::late::{PatternSource, Rib};
 use crate::path_names_to_string;
@@ -1419,11 +1419,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             "",
         );
 
-        if macro_kind == MacroKind::Bang && ident.name == sym::macro_rules {
-            err.subdiagnostic(MaybeMissingMacroRulesName { span: ident.span });
-            return;
-        }
-
         if macro_kind == MacroKind::Derive && (ident.name == sym::Send || ident.name == sym::Sync) {
             err.subdiagnostic(ExplicitUnsafeTraits { span: ident.span, ident });
             return;

compiler/rustc_resolve/src/errors.rs

@@ -665,13 +665,6 @@ pub(crate) struct ExplicitUnsafeTraits {
     pub(crate) ident: Ident,
 }
 
-#[derive(Subdiagnostic)]
-#[note(resolve_missing_macro_rules_name)]
-pub(crate) struct MaybeMissingMacroRulesName {
-    #[primary_span]
-    pub(crate) span: Span,
-}
-
 #[derive(Subdiagnostic)]
 #[help(resolve_added_macro_use)]
 pub(crate) struct AddedMacroUse;

tests/ui/macros/issue-118786.rs

@@ -5,9 +5,7 @@
 macro_rules! make_macro {
     ($macro_name:tt) => {
         macro_rules! $macro_name {
-        //~^ ERROR macros that expand to items must be delimited with braces or followed by a semicolon
-        //~| ERROR macro expansion ignores token `{` and any following
-        //~| ERROR cannot find macro `macro_rules` in this scope
+            //~^ ERROR: expected identifier, found `(`
             () => {}
         }
     }

tests/ui/macros/issue-118786.stderr

@@ -1,47 +1,13 @@
-error: macros that expand to items must be delimited with braces or followed by a semicolon
+error: expected identifier, found `(`
   --> $DIR/issue-118786.rs:7:22
    |
 LL |         macro_rules! $macro_name {
-   |                      ^^^^^^^^^^^
+   |                      ^^^^^^^^^^^ expected identifier
    |
-help: change the delimiters to curly braces
+help: try removing the parenthesis around the name for this `macro_rules!`
    |
-LL |         macro_rules! {} {
-   |                      ~          +
-help: add a semicolon
-   |
-LL |         macro_rules! $macro_name; {
-   |                                 +
-
-error: macro expansion ignores token `{` and any following
-  --> $DIR/issue-118786.rs:7:34
-   |
-LL |         macro_rules! $macro_name {
-   |                                  ^
-...
-LL | make_macro!((meow));
-   | ------------------- caused by the macro expansion here
-   |
-   = note: the usage of `make_macro!` is likely invalid in item context
-
-error: cannot find macro `macro_rules` in this scope
-  --> $DIR/issue-118786.rs:7:9
-   |
-LL |         macro_rules! $macro_name {
-   |         ^^^^^^^^^^^
-...
-LL | make_macro!((meow));
-   | ------------------- in this macro invocation
-   |
-note: maybe you have forgotten to define a name for this `macro_rules!`
-  --> $DIR/issue-118786.rs:7:9
-   |
-LL |         macro_rules! $macro_name {
-   |         ^^^^^^^^^^^
-...
-LL | make_macro!((meow));
-   | ------------------- in this macro invocation
-   = note: this error originates in the macro `make_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
+LL |         macro_rules! meow {
+   |                      ~~~~
 
-error: aborting due to 3 previous errors
+error: aborting due to 1 previous error
 

tests/ui/macros/mbe-missing-ident-error.rs

@@ -0,0 +1,7 @@
+// Ensures MBEs with a missing ident produce a readable error
+
+macro_rules! {
+    //~^ ERROR: expected identifier, found `{`
+    //~| HELP: maybe you have forgotten to define a name for this `macro_rules!`
+    () => {}
+}

tests/ui/macros/mbe-missing-ident-error.stderr

@@ -0,0 +1,10 @@
+error: expected identifier, found `{`
+  --> $DIR/mbe-missing-ident-error.rs:3:14
+   |
+LL | macro_rules! {
+   |              ^ expected identifier
+   |
+   = help: maybe you have forgotten to define a name for this `macro_rules!`
+
+error: aborting due to 1 previous error
+

tests/ui/macros/mbe-parenthesis-ident-error.rs

@@ -0,0 +1,7 @@
+// Ensures MBEs with a invalid ident produce a readable error
+
+macro_rules! (meepmeep) {
+    //~^ ERROR: expected identifier, found `(`
+    //~| HELP: try removing the parenthesis around the name for this `macro_rules!`
+    () => {}
+}

tests/ui/macros/mbe-parenthesis-ident-error.stderr

@@ -0,0 +1,13 @@
+error: expected identifier, found `(`
+  --> $DIR/mbe-parenthesis-ident-error.rs:3:14
+   |
+LL | macro_rules! (meepmeep) {
+   |              ^ expected identifier
+   |
+help: try removing the parenthesis around the name for this `macro_rules!`
+   |
+LL | macro_rules! meepmeep {
+   |              ~~~~~~~~
+
+error: aborting due to 1 previous error
+

tests/ui/macros/user-defined-macro-rules.rs

@@ -1,9 +1,8 @@
-// check-pass
+// check-fail
 
-macro_rules! macro_rules { () => { struct S; } } // OK
+macro_rules! macro_rules { () => {} }
+//~^ ERROR: user-defined macros may not be named `macro_rules`
 
-macro_rules! {} // OK, calls the macro defined above
-
-fn main() {
-    let s = S;
-}
+macro_rules! {}
+//~^ ERROR: expected identifier, found `{`
+//~| HELP: maybe you have forgotten to define a name for this `macro_rules!`

tests/ui/macros/user-defined-macro-rules.stderr

@@ -0,0 +1,16 @@
+error: user-defined macros may not be named `macro_rules`
+  --> $DIR/user-defined-macro-rules.rs:3:14
+   |
+LL | macro_rules! macro_rules { () => {} }
+   |              ^^^^^^^^^^^
+
+error: expected identifier, found `{`
+  --> $DIR/user-defined-macro-rules.rs:6:14
+   |
+LL | macro_rules! {}
+   |              ^ expected identifier
+   |
+   = help: maybe you have forgotten to define a name for this `macro_rules!`
+
+error: aborting due to 2 previous errors
+