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

Author: EliseZeroTwo

compiler/rustc_parse/messages.ftl

@@ -472,6 +472,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
 
@@ -501,6 +503,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

@@ -2671,6 +2671,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

@@ -2040,23 +2040,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.
@@ -2070,7 +2073,42 @@ 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) => {
+                match (
+                    &self.token.kind,
+                    self.look_ahead(1, |token| token.ident()),
+                    self.look_ahead(2, |token| {
+                        (token.kind == TokenKind::CloseDelim(Delimiter::Parenthesis))
+                            .then_some(token.span)
+                    }),
+                ) {
+                    (
+                        TokenKind::OpenDelim(Delimiter::Parenthesis),
+                        Some((Ident { name, .. }, _)),
+                        Some(closing_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,
+                        );
+                    }
+                    _ => {
+                        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/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,5 @@
-// 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 `{`

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:5:14
+   |
+LL | macro_rules! {}
+   |              ^ expected identifier
+   |
+   = note: maybe you have forgotten to define a name for this `macro_rules!`
+
+error: aborting due to 2 previous errors
+