mbe: Rework diagnostics for metavariable expressions

compiler/rustc_expand/messages.ftl

@@ -133,6 +133,38 @@ 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_in_braces =
+    unexpected trailing tokens in metavariable expression braces
+    .suggestion = try removing these tokens
+
+expand_mve_extra_tokens_in_expr =
+    unexpected trailing tokens in metavariable expression
+    .label = for this metavariable expression
+    .note= the `{$name}` metavariable expression takes up to {$max} arguments
+    .suggestion = try removing {$count ->
+        [one] this token
+        *[other] these tokens
+    }
+
+expand_mve_missing_paren =
+    expected `(`
+    .label = for this this metavariable expression
+    .note = metavariable expressions use function-like parentheses syntax
+    .suggestion = try adding parentheses
+
+expand_mve_unrecognized_expr =
+    unrecognized metavariable expression
+    .label = not a valid metavariable expression
+    .note = valid metavariable expressions are {$valid_expr_list}
+
 expand_mve_unrecognized_var =
     variable `{$key}` is not recognized in meta-variable expression
 

compiler/rustc_expand/src/errors.rs

@@ -496,6 +496,72 @@ pub(crate) use metavar_exprs::*;
 mod metavar_exprs {
     use super::*;
 
+    #[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_extra_tokens_in_braces)]
+    pub(crate) struct MveExtraTokensInBraces {
+        #[primary_span]
+        #[suggestion(code = "", applicability = "machine-applicable")]
+        pub span: Span,
+    }
+
+    #[derive(Diagnostic)]
+    #[note]
+    #[diag(expand_mve_extra_tokens_in_expr)]
+    pub(crate) struct MveExtraTokensInExpr {
+        #[primary_span]
+        #[suggestion(code = "", applicability = "machine-applicable")]
+        pub span: Span,
+        #[label]
+        pub ident_span: Span,
+        pub count: usize,
+        pub max: usize,
+        pub name: &'static str,
+    }
+
+    #[derive(Diagnostic)]
+    #[note]
+    #[diag(expand_mve_missing_paren)]
+    pub(crate) struct MveMissingParen {
+        #[primary_span]
+        pub span: Span,
+        #[suggestion(code = "( /* ... */ )", applicability = "has-placeholders")]
+        pub insert_span: Option<Span>,
+    }
+
+    #[derive(Diagnostic)]
+    #[note]
+    #[diag(expand_mve_unrecognized_expr)]
+    pub(crate) struct MveUnrecognizedExpr {
+        #[primary_span]
+        #[label]
+        pub span: Span,
+        pub valid_expr_list: &'static str,
+    }
+
     #[derive(Diagnostic)]
     #[diag(expand_mve_unrecognized_var)]
     pub(crate) struct MveUnrecognizedVar {

compiler/rustc_expand/src/mbe/metavar_expr.rs

@@ -7,9 +7,23 @@ use rustc_macros::{Decodable, Encodable};
 use rustc_session::parse::ParseSess;
 use rustc_span::{Ident, Span, Symbol};
 
+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";
 
+/// List of the below list for diagnostics.
+const VALID_METAVAR_EXPR_NAMES: &str = "`count`, `ignore`, `index`, `len`, and `concat`";
+
+/// Map from expression names to the maximum arg count.
+const EXPR_NAME_ARG_MAP: &[(&str, Option<usize>)] = &[
+    ("concat", None),
+    ("count", Some(2)),
+    ("ignore", Some(1)),
+    ("index", Some(2)),
+    ("len", Some(2)),
+];
+
 /// A meta-variable expression, for expansions based on properties of meta-variables.
 #[derive(Debug, PartialEq, Encodable, Decodable)]
 pub(crate) enum MetaVarExpr {
@@ -39,35 +53,53 @@ impl MetaVarExpr {
         psess: &'psess ParseSess,
     ) -> PResult<'psess, MetaVarExpr> {
         let mut iter = input.iter();
-        let ident = parse_ident(&mut iter, psess, outer_span)?;
-        let Some(TokenTree::Delimited(.., Delimiter::Parenthesis, args)) = iter.next() else {
-            let msg = "meta-variable expression parameter must be wrapped in parentheses";
-            return Err(psess.dcx().struct_span_err(ident.span, msg));
+        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
+            let (span, insert_span) = match next {
+                Some(TokenTree::Delimited(delim, ..)) => (delim.open, None),
+                Some(tt) => (tt.span(), Some(ident.span.shrink_to_hi())),
+                None => (ident.span.shrink_to_hi(), Some(ident.span.shrink_to_hi())),
+            };
+            let err = errors::MveMissingParen { span, insert_span };
+            return Err(psess.dcx().create_err(err));
         };
-        check_trailing_token(&mut iter, psess)?;
+
+        // Ensure there are no other tokens in the
+        if iter.peek().is_some() {
+            let span = iter_span(&iter).expect("checked is_some above");
+            let err = errors::MveExtraTokensInBraces { span };
+            return Err(psess.dcx().create_err(err));
+        }
+
         let mut iter = args.iter();
         let rslt = match ident.as_str() {
             "concat" => parse_concat(&mut iter, psess, outer_span, ident.span)?,
             "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)?),
             _ => {
-                let err_msg = "unrecognized meta-variable expression";
-                let mut err = psess.dcx().struct_span_err(ident.span, err_msg);
-                err.span_suggestion(
-                    ident.span,
-                    "supported expressions are count, ignore, index and len",
-                    "",
-                    Applicability::MachineApplicable,
-                );
-                return Err(err);
+                let err = errors::MveUnrecognizedExpr {
+                    span: ident.span,
+                    valid_expr_list: VALID_METAVAR_EXPR_NAMES,
+                };
+                return Err(psess.dcx().create_err(err));
             }
         };
-        check_trailing_token(&mut iter, psess)?;
+        check_trailing_tokens(&mut iter, psess, ident)?;
         Ok(rslt)
     }
 
@@ -87,20 +119,44 @@ impl MetaVarExpr {
     }
 }
 
-// Checks if there are any remaining tokens. For example, `${ignore(ident ... a b c ...)}`
-fn check_trailing_token<'psess>(
+/// Checks if there are any remaining tokens. For example, `${ignore(ident ... a b c ...)}`
+fn check_trailing_tokens<'psess>(
     iter: &mut TokenStreamIter<'_>,
     psess: &'psess ParseSess,
+    ident: Ident,
 ) -> PResult<'psess, ()> {
-    if let Some(tt) = iter.next() {
-        let mut diag = psess
-            .dcx()
-            .struct_span_err(tt.span(), format!("unexpected token: {}", pprust::tt_to_string(tt)));
-        diag.span_note(tt.span(), "meta-variable expression must not have trailing tokens");
-        Err(diag)
-    } else {
-        Ok(())
+    if iter.peek().is_none() {
+        // All tokens used, no problem
+        return Ok(());
     }
+
+    let (name, max) = EXPR_NAME_ARG_MAP
+        .iter()
+        .find(|(name, _)| *name == ident.as_str())
+        .expect("called with an invalid name");
+
+    let Some(max) = *max else {
+        // For expressions like `concat`, all tokens should be consumed already
+        panic!("{name} takes unlimited tokens but didn't eat them all");
+    };
+
+    let err = errors::MveExtraTokensInExpr {
+        span: iter_span(iter).expect("checked is_none above"),
+        ident_span: ident.span,
+        count: iter.count(),
+        max,
+        name,
+    };
+    Err(psess.dcx().create_err(err))
+}
+
+/// Returns a span encompassing all tokens in the iterator if there is at least one item.
+fn iter_span(iter: &TokenStreamIter<'_>) -> Option<Span> {
+    let mut iter = iter.clone(); // cloning is cheap
+    let first_sp = iter.next()?.span();
+    let last_sp = iter.last().map(TokenTree::span).unwrap_or(first_sp);
+    let span = first_sp.with_hi(last_sp.hi());
+    Some(span)
 }
 
 /// Indicates what is placed in a `concat` parameter. For example, literals
@@ -168,7 +224,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(
@@ -206,14 +262,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

@@ -30,62 +30,62 @@ macro_rules! metavar_with_literal_suffix {
 
 macro_rules! mve_without_parens {
     ( $( $i:ident ),* ) => { ${ count } };
-    //~^ ERROR meta-variable expression parameter must be wrapped in parentheses
+    //~^ ERROR expected `(`
 }
 
 #[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! mve_wrong_delim {
     ( $( $i:ident ),* ) => { ${ count{i} } };
-    //~^ ERROR meta-variable expression parameter must be wrapped in parentheses
+    //~^ ERROR expected `(`
 }
 
 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 {
     ( $( $i:ident ),* ) => {
         ${count() a b c}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${count($i a b c)}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${count($i, 1 a b c)}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${count($i) a b c}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
 
         ${ignore($i) a b c}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${ignore($i a b c)}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
 
         ${index() a b c}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${index(1 a b c)}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
 
         ${index() a b c}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
         ${index(1 a b c)}
-        //~^ ERROR unexpected token: a
+        //~^ ERROR unexpected trailing tokens
     };
 }
 
@@ -111,7 +111,7 @@ macro_rules! unknown_ignore_ident {
 
 macro_rules! unknown_metavar {
     ( $( $i:ident ),* ) => { ${ aaaaaaaaaaaaaa(i) } };
-    //~^ ERROR unrecognized meta-variable expression
+    //~^ ERROR unrecognized metavariable expression
 }
 
 fn main() {}