mbe: Refactor diagnostics for missing identifiers

The current messages have the potential to be more accurate; "expected
identifier or string literal" is printed in a few cases where only an
identifier should be expected, and it suggests removing string literals
when that might not solve the problem. Add a new diagnostic for these
kind of errors that gives more context and a better description.

For `count` and `ignore` this should likely be combined with the
diagnositcs for `eat_dollar` to produce a helpful error if they get
anything other than a metavariable first argument. I am planning to do
this in a followup.

Author: tgross35

compiler/rustc_expand/messages.ftl

@@ -133,6 +133,14 @@ expand_module_multiple_candidates =
 expand_must_repeat_once =
     this must repeat at least once
 
+expand_mve_expected_ident =
+    expected an identifier
+    .not_ident = not a valid identifier
+    .expr_name = expected a metavariable expression name: `{"${expr( /* ... */ )}"}`
+    .expr_name_note = valid metavariable expressions are {$valid_expr_list}
+    .ignore_expr_note = `ignore` takes a metavariable argument
+    .count_expr_note = `count` takes a metavariable argument
+
 expand_mve_extra_tokens =
     unexpected trailing tokens
     .label = for this metavariable expression

compiler/rustc_expand/src/errors.rs

@@ -540,6 +540,30 @@ mod metavar_exprs {
         pub valid_expr_list: &'static str,
     }
 
+    #[derive(Diagnostic)]
+    #[diag(expand_mve_expected_ident)]
+    pub(crate) struct MveExpectedIdent {
+        #[primary_span]
+        pub span: Span,
+        #[label(expand_not_ident)]
+        pub not_ident_label: Option<Span>,
+        /// This error is reused a handful of places, the context here tells us how to customize
+        /// the message.
+        #[subdiagnostic]
+        pub context: MveExpectedIdentContext,
+    }
+
+    #[derive(Subdiagnostic)]
+    pub(crate) enum MveExpectedIdentContext {
+        #[note(expand_expr_name)]
+        #[note(expand_expr_name_note)]
+        ExprName { valid_expr_list: &'static str },
+        #[note(expand_ignore_expr_note)]
+        Ignore,
+        #[note(expand_count_expr_note)]
+        Count,
+    }
+
     #[derive(Diagnostic)]
     #[diag(expand_mve_unrecognized_var)]
     pub(crate) struct MveUnrecognizedVar {

compiler/rustc_expand/src/mbe/metavar_expr.rs

@@ -7,7 +7,7 @@ use rustc_macros::{Decodable, Encodable};
 use rustc_session::parse::ParseSess;
 use rustc_span::{Ident, Span, Symbol};
 
-use crate::errors;
+use crate::errors::{self, MveExpectedIdentContext};
 
 pub(crate) const RAW_IDENT_ERR: &str = "`${concat(..)}` currently does not support raw identifiers";
 pub(crate) const UNSUPPORTED_CONCAT_ELEM_ERR: &str = "expected identifier or string literal";
@@ -64,7 +64,13 @@ impl MetaVarExpr {
         psess: &'psess ParseSess,
     ) -> PResult<'psess, MetaVarExpr> {
         let mut iter = input.iter();
-        let ident = parse_ident(&mut iter, psess, outer_span)?;
+        let ident = parse_ident(
+            &mut iter,
+            psess,
+            outer_span,
+            MveExpectedIdentContext::ExprName { valid_expr_list: VALID_METAVAR_EXPR_NAMES },
+        )?;
+
         let next = iter.next();
         let Some(TokenTree::Delimited(.., Delimiter::Parenthesis, args)) = next else {
             // No `()`; wrong or no delimiters. Point at a problematic span or a place to
@@ -97,7 +103,9 @@ impl MetaVarExpr {
             "count" => parse_count(&mut iter, psess, ident.span)?,
             "ignore" => {
                 eat_dollar(&mut iter, psess, ident.span)?;
-                MetaVarExpr::Ignore(parse_ident(&mut iter, psess, ident.span)?)
+                let ident =
+                    parse_ident(&mut iter, psess, outer_span, MveExpectedIdentContext::Ignore)?;
+                MetaVarExpr::Ignore(ident)
             }
             "index" => MetaVarExpr::Index(parse_depth(&mut iter, psess, ident.span)?),
             "len" => MetaVarExpr::Len(parse_depth(&mut iter, psess, ident.span)?),
@@ -241,7 +249,7 @@ fn parse_count<'psess>(
     span: Span,
 ) -> PResult<'psess, MetaVarExpr> {
     eat_dollar(iter, psess, span)?;
-    let ident = parse_ident(iter, psess, span)?;
+    let ident = parse_ident(iter, psess, span, MveExpectedIdentContext::Count)?;
     let depth = if try_eat_comma(iter) {
         if iter.peek().is_none() {
             return Err(psess.dcx().struct_span_err(
@@ -279,14 +287,32 @@ fn parse_depth<'psess>(
     }
 }
 
-/// Parses an generic ident
+/// Tries to parse a generic ident. If this fails, create a missing identifier diagnostic with
+/// `context` explanation.
 fn parse_ident<'psess>(
     iter: &mut TokenStreamIter<'_>,
     psess: &'psess ParseSess,
     fallback_span: Span,
+    context: MveExpectedIdentContext,
 ) -> PResult<'psess, Ident> {
-    let token = parse_token(iter, psess, fallback_span)?;
-    parse_ident_from_token(psess, token)
+    let Some(tt) = iter.next() else {
+        let err = errors::MveExpectedIdent { span: fallback_span, not_ident_label: None, context };
+        return Err(psess.dcx().create_err(err));
+    };
+
+    let TokenTree::Token(token, _) = tt else {
+        let span = tt.span();
+        let err = errors::MveExpectedIdent { span, not_ident_label: Some(span), context };
+        return Err(psess.dcx().create_err(err));
+    };
+
+    let Some((elem, _)) = token.ident() else {
+        let span = token.span;
+        let err = errors::MveExpectedIdent { span, not_ident_label: Some(span), context };
+        return Err(psess.dcx().create_err(err));
+    };
+
+    Ok(elem)
 }
 
 fn parse_ident_from_token<'psess>(

tests/ui/macros/metavar-expressions/syntax-errors.rs

@@ -36,13 +36,13 @@ macro_rules! mve_without_parens {
 #[rustfmt::skip]
 macro_rules! empty_expression {
     () => { ${} };
-    //~^ ERROR expected identifier or string literal
+    //~^ ERROR expected an identifier
 }
 
 #[rustfmt::skip]
 macro_rules! open_brackets_with_lit {
      () => { ${ "hi" } };
-     //~^ ERROR expected identifier
+     //~^ ERROR expected an identifier
  }
 
 macro_rules! mvs_missing_paren {
@@ -57,13 +57,13 @@ macro_rules! mve_wrong_delim {
 
 macro_rules! invalid_metavar {
     () => { ${ignore($123)} }
-    //~^ ERROR expected identifier, found `123`
+    //~^ ERROR expected an identifier
 }
 
 #[rustfmt::skip]
 macro_rules! open_brackets_with_group {
     ( $( $i:ident ),* ) => { ${ {} } };
-    //~^ ERROR expected identifier
+    //~^ ERROR expected an identifier
 }
 
 macro_rules! extra_garbage_after_metavar {

tests/ui/macros/metavar-expressions/syntax-errors.stderr

@@ -68,11 +68,13 @@ LL |     ( $( $i:ident ),* ) => { ${ count{i} } };
    |
    = note: metavariable expressions use function-like parentheses syntax
 
-error: expected identifier, found `123`
+error: expected an identifier
   --> $DIR/syntax-errors.rs:59:23
    |
 LL |     () => { ${ignore($123)} }
-   |                       ^^^ help: try removing `123`
+   |                       ^^^ not a valid identifier
+   |
+   = note: `ignore` takes a metavariable argument
 
 error: unexpected trailing tokens
   --> $DIR/syntax-errors.rs:71:19
@@ -200,23 +202,32 @@ LL |     ( $( $i:ident ),* ) => { ${ aaaaaaaaaaaaaa(i) } };
    |
    = note: valid metavariable expressions are `count`, `ignore`, `index`, `len`, and `concat`
 
-error: expected identifier or string literal
+error: expected an identifier
   --> $DIR/syntax-errors.rs:38:14
    |
 LL |     () => { ${} };
    |              ^^
+   |
+   = note: expected a metavariable expression name: `${expr( /* ... */ )}`
+   = note: valid metavariable expressions are `count`, `ignore`, `index`, `len`, and `concat`
 
-error: expected identifier, found `"hi"`
+error: expected an identifier
   --> $DIR/syntax-errors.rs:44:17
    |
 LL |      () => { ${ "hi" } };
-   |                 ^^^^ help: try removing `"hi"`
+   |                 ^^^^ not a valid identifier
+   |
+   = note: expected a metavariable expression name: `${expr( /* ... */ )}`
+   = note: valid metavariable expressions are `count`, `ignore`, `index`, `len`, and `concat`
 
-error: expected identifier or string literal
+error: expected an identifier
   --> $DIR/syntax-errors.rs:65:33
    |
 LL |     ( $( $i:ident ),* ) => { ${ {} } };
-   |                                 ^^
+   |                                 ^^ not a valid identifier
+   |
+   = note: expected a metavariable expression name: `${expr( /* ... */ )}`
+   = note: valid metavariable expressions are `count`, `ignore`, `index`, `len`, and `concat`
 
 error: aborting due to 27 previous errors