mbe: Refactor concat diagnostics

Author: tgross35

compiler/rustc_expand/messages.ftl

@@ -133,20 +133,23 @@ expand_module_multiple_candidates =
 expand_must_repeat_once =
     this must repeat at least once
 
-expand_mve_concat_invalid =
-    invalid item within a `{"${concat(...)}"}` expression
-    .expr_ident = expanding this `concat(...)` expression
-    .invalid_ident = this literal produced an invalid identifier
+expand_mve_concat_invalid_in =
+    invalid item within a {"`${concat(..)}`"} expression
+    .metavar_label = expanding this metavariable
     .float_lit = float literals cannot be concatenated
     .c_str_lit = C string literals cannot be concatenated
     .b_str_lit = byte literals cannot be concatenated
     .raw_ident = raw identifiers cannot be concatenated
-    .unsupported = unsupported input for `concat(...)`
+    .unsupported = unsupported input for `concat(..)`
     .valid_types = `concat` can join {$valid}
-    .expected_metavar = expected an identifier; got `{$found}`
-    .expected_metavar_dollar = todo
-    .unexpected_group = todo
-    .hi_label = todo
+    .expected_metavar = expected an identifier
+    .expected_metavar_dollar = `$` indicates the start of a metavariable
+    .invalid_metavar = expanding something
+    .valid_metavars = {"`${concat(..)}`"} can join metavariables of type {$valid}
+
+expand_mve_concat_invalid_out =
+    invalid item within a {"`${concat(..)}`"} would produce an invalid identifier
+    .label = todo
 
 expand_mve_expected_ident =
     expected an identifier

compiler/rustc_expand/src/errors.rs

@@ -498,48 +498,58 @@ mod metavar_exprs {
     use super::*;
 
     #[derive(Diagnostic)]
-    #[diag(expand_mve_concat_invalid)]
-    pub(crate) struct MveConcatInvalid {
+    #[diag(expand_mve_concat_invalid_in)]
+    pub(crate) struct MveConcatInvalidTy {
         #[primary_span]
+        // #[label]
         pub span: Span,
+        #[label(expand_metavar_label)]
+        pub metavar_span: Option<Span>,
         #[subdiagnostic]
-        pub reason: MveConcatInvalidReason,
-        #[help(expand_expr_ident)]
-        pub ident_span: Span,
+        pub reason: MveConcatInvalidTyReason,
         pub valid: &'static str,
     }
 
     // TODO: can these be labels rather than notes?
     #[derive(Subdiagnostic)]
-    pub(crate) enum MveConcatInvalidReason {
-        #[note(expand_invalid_ident)]
+    pub(crate) enum MveConcatInvalidTyReason {
         InvalidIdent,
         #[note(expand_float_lit)]
-        #[help(expand_valid_types)]
+        #[note(expand_valid_types)]
         FloatLit,
         #[note(expand_c_str_lit)]
-        #[help(expand_valid_types)]
+        #[note(expand_valid_types)]
         CStrLit,
         #[note(expand_b_str_lit)]
-        #[help(expand_valid_types)]
+        #[note(expand_valid_types)]
         ByteStrLit,
         #[note(expand_expected_metavar)]
         #[label(expand_expected_metavar_dollar)]
         ExpectedMetavarIdent {
-            found: String,
             #[primary_span]
             dollar: Span,
         },
         #[note(expand_raw_ident)]
         RawIdentifier,
         #[note(expand_unsupported)]
-        #[help(expand_valid_types)]
+        #[note(expand_valid_types)]
         UnsupportedInput,
-        #[note(expand_unexpected_group)]
-        UnexpectedGroup,
+        #[note(expand_invalid_metavar)]
+        #[note(expand_valid_metavars)]
+        InvalidMetavarTy,
+        /// Nothing to point out because an error was already emitted.
         InvalidLiteral,
     }
 
+    #[derive(Diagnostic)]
+    #[note]
+    #[diag(expand_mve_concat_invalid_out)]
+    pub(crate) struct MveConcatInvalidOut {
+        #[primary_span]
+        #[label]
+        pub span: Span,
+    }
+
     #[derive(Diagnostic)]
     #[diag(expand_mve_expected_ident)]
     pub(crate) struct MveExpectedIdent {

compiler/rustc_expand/src/mbe/metavar_expr.rs

@@ -2,16 +2,15 @@ use rustc_ast::token::{self, Delimiter, IdentIsRaw, Token, TokenKind};
 use rustc_ast::tokenstream::{TokenStream, TokenStreamIter, TokenTree};
 use rustc_ast::{self as ast, LitIntType, LitKind};
 use rustc_ast_pretty::pprust;
-use rustc_errors::PResult;
+use rustc_errors::{DiagCtxtHandle, PResult};
 use rustc_lexer::is_id_continue;
 use rustc_macros::{Decodable, Encodable};
 use rustc_session::errors::create_lit_error;
 use rustc_session::parse::ParseSess;
 use rustc_span::{Ident, Span, Symbol};
 
-use crate::errors::{self, MveConcatInvalidReason, MveExpectedIdentContext};
+use crate::errors::{self, MveConcatInvalidTyReason, MveExpectedIdentContext};
 
-pub(crate) const RAW_IDENT_ERR: &str = "`${concat(..)}` currently does not support raw identifiers";
 pub(crate) const VALID_EXPR_CONCAT_TYPES: &str =
     "metavariables, identifiers, string literals, and integer literals";
 
@@ -193,74 +192,32 @@ fn parse_concat<'psess>(
         };
 
         let make_err = |reason| {
-            let err = errors::MveConcatInvalid {
+            let err = errors::MveConcatInvalidTy {
                 span: tt.span(),
-                ident_span: expr_ident_span,
+                metavar_span: None,
                 reason,
                 valid: VALID_EXPR_CONCAT_TYPES,
             };
             Err(dcx.create_err(err))
         };
 
         let token = match tt {
-            TokenTree::Token(token, _) => token,
+            TokenTree::Token(token, _) => *token,
             TokenTree::Delimited(..) => {
-                return make_err(MveConcatInvalidReason::UnexpectedGroup);
+                return make_err(MveConcatInvalidTyReason::UnsupportedInput);
             }
         };
 
         let element = if let Some(dollar) = dollar {
             // Expecting a metavar
             let Some((ident, _)) = token.ident() else {
-                return make_err(MveConcatInvalidReason::ExpectedMetavarIdent {
-                    found: pprust::token_to_string(token).into_owned(),
-                    dollar,
-                });
+                return make_err(MveConcatInvalidTyReason::ExpectedMetavarIdent { dollar });
             };
 
             // Variables get passed untouched
             MetaVarExprConcatElem::Var(ident)
-        } else if let TokenKind::Literal(lit) = token.kind {
-            // Preprocess with `from_token_lit` to handle unescaping, float / int literal suffix
-            // stripping.
-            //
-            // For consistent user experience, please keep this in sync with the handling of
-            // literals in `rustc_builtin_macros::concat`!
-            let s = match ast::LitKind::from_token_lit(lit.clone()) {
-                Ok(ast::LitKind::Str(s, _)) => s.to_string(),
-                Ok(ast::LitKind::Float(..)) => {
-                    return make_err(MveConcatInvalidReason::FloatLit);
-                }
-                Ok(ast::LitKind::Char(c)) => c.to_string(),
-                Ok(ast::LitKind::Int(i, _)) => i.to_string(),
-                Ok(ast::LitKind::Bool(b)) => b.to_string(),
-                Ok(ast::LitKind::CStr(..)) => return make_err(MveConcatInvalidReason::CStrLit),
-                Ok(ast::LitKind::Byte(..) | ast::LitKind::ByteStr(..)) => {
-                    return make_err(MveConcatInvalidReason::ByteStrLit);
-                }
-                Ok(ast::LitKind::Err(_guarantee)) => {
-                    // REVIEW: a diagnostic was already emitted, should we just break?
-                    return make_err(MveConcatInvalidReason::InvalidLiteral);
-                }
-                Err(err) => return Err(create_lit_error(psess, err, lit, token.span)),
-            };
-
-            if !s.chars().all(|ch| is_id_continue(ch)) {
-                // Check that all characters are valid in the middle of an identifier. This doesn't
-                // guarantee that the final identifier is valid (we still need to check it later),
-                // but it allows us to catch errors with specific arguments before expansion time;
-                // for example, string literal "foo.bar" gets flagged before the macro is invoked.
-                return make_err(MveConcatInvalidReason::InvalidIdent);
-            }
-
-            MetaVarExprConcatElem::Ident(s)
-        } else if let Some((elem, is_raw)) = token.ident() {
-            if is_raw == IdentIsRaw::Yes {
-                return make_err(MveConcatInvalidReason::RawIdentifier);
-            }
-            MetaVarExprConcatElem::Ident(elem.as_str().to_string())
         } else {
-            return make_err(MveConcatInvalidReason::UnsupportedInput);
+            MetaVarExprConcatElem::Ident(parse_tok_for_concat(psess, token)?)
         };
 
         result.push(element);
@@ -327,6 +284,68 @@ fn parse_depth<'psess>(
     }
 }
 
+/// Validate that a token can be concatenated as an identifier, then stringify it.
+pub(super) fn parse_tok_for_concat<'psess>(
+    psess: &'psess ParseSess,
+    token: Token,
+) -> PResult<'psess, String> {
+    let dcx = psess.dcx();
+    let make_err = |reason| {
+        let err = errors::MveConcatInvalidTy {
+            span: token.span,
+            metavar_span: None,
+            reason,
+            valid: VALID_EXPR_CONCAT_TYPES,
+        };
+        Err(dcx.create_err(err))
+    };
+
+    let elem = if let TokenKind::Literal(lit) = token.kind {
+        // Preprocess with `from_token_lit` to handle unescaping, float / int literal suffix
+        // stripping.
+        //
+        // For consistent user experience, please keep this in sync with the handling of
+        // literals in `rustc_builtin_macros::concat`!
+        let s = match ast::LitKind::from_token_lit(lit.clone()) {
+            Ok(ast::LitKind::Str(s, _)) => s.to_string(),
+            Ok(ast::LitKind::Float(..)) => {
+                return make_err(MveConcatInvalidTyReason::FloatLit);
+            }
+            Ok(ast::LitKind::Char(c)) => c.to_string(),
+            Ok(ast::LitKind::Int(i, _)) => i.to_string(),
+            Ok(ast::LitKind::Bool(b)) => b.to_string(),
+            Ok(ast::LitKind::CStr(..)) => return make_err(MveConcatInvalidTyReason::CStrLit),
+            Ok(ast::LitKind::Byte(..) | ast::LitKind::ByteStr(..)) => {
+                return make_err(MveConcatInvalidTyReason::ByteStrLit);
+            }
+            Ok(ast::LitKind::Err(_guarantee)) => {
+                // REVIEW: a diagnostic was already emitted, should we just break?
+                return make_err(MveConcatInvalidTyReason::InvalidLiteral);
+            }
+            Err(err) => return Err(create_lit_error(psess, err, lit, token.span)),
+        };
+
+        if !s.chars().all(|ch| is_id_continue(ch)) {
+            // Check that all characters are valid in the middle of an identifier. This doesn't
+            // guarantee that the final identifier is valid (we still need to check it later),
+            // but it allows us to catch errors with specific arguments before expansion time;
+            // for example, string literal "foo.bar" gets flagged before the macro is invoked.
+            return make_err(MveConcatInvalidTyReason::InvalidIdent);
+        }
+
+        s
+    } else if let Some((elem, is_raw)) = token.ident() {
+        if is_raw == IdentIsRaw::Yes {
+            return make_err(MveConcatInvalidTyReason::RawIdentifier);
+        }
+        elem.as_str().to_string()
+    } else {
+        return make_err(MveConcatInvalidTyReason::UnsupportedInput);
+    };
+
+    Ok(elem)
+}
+
 /// Tries to parse a generic ident. If this fails, create a missing identifier diagnostic with
 /// `context` explanation.
 fn parse_ident<'psess>(

compiler/rustc_expand/src/mbe/transcribe.rs

@@ -16,15 +16,18 @@ use rustc_span::{
 };
 use smallvec::{SmallVec, smallvec};
 
+use super::metavar_expr::parse_tok_for_concat;
 use crate::errors::{
-    CountRepetitionMisplaced, MetaVarsDifSeqMatchers, MustRepeatOnce, MveUnrecognizedVar,
+    self, CountRepetitionMisplaced, MetaVarsDifSeqMatchers, MustRepeatOnce, MveUnrecognizedVar,
     NoSyntaxVarsExprRepeat, VarStillRepeating,
 };
 use crate::mbe::macro_parser::NamedMatch;
 use crate::mbe::macro_parser::NamedMatch::*;
-use crate::mbe::metavar_expr::{MetaVarExprConcatElem, RAW_IDENT_ERR};
+use crate::mbe::metavar_expr::{MetaVarExprConcatElem, VALID_EXPR_CONCAT_TYPES};
 use crate::mbe::{self, KleeneOp, MetaVarExpr};
 
+const VALID_METAVAR_CONCAT_TYPES: &str = "`ident`, `literal`, and `tt`";
+
 /// Context needed to perform transcription of metavariable expressions.
 struct TranscrCtx<'psess, 'itp> {
     psess: &'psess ParseSess,
@@ -905,48 +908,37 @@ fn out_of_bounds_err<'a>(dcx: DiagCtxtHandle<'a>, max: usize, span: Span, ty: &s
 }
 
 /// Extracts an metavariable symbol that can be an identifier, a token tree or a literal.
-// TODO: use the same logic as for metavar_expr
 fn extract_symbol_from_pnr<'tx>(
     tscx: &mut TranscrCtx<'tx, '_>,
     pnr: &ParseNtResult,
-    span_err: Span,
-) -> PResult<'tx, Symbol> {
-    let dcx = tscx.psess.dcx();
-    match pnr {
-        ParseNtResult::Ident(nt_ident, is_raw) => {
-            if let IdentIsRaw::Yes = is_raw {
-                Err(dcx.struct_span_err(span_err, RAW_IDENT_ERR))
-            } else {
-                Ok(nt_ident.name)
-            }
-        }
-        ParseNtResult::Tt(TokenTree::Token(
-            Token { kind: TokenKind::Ident(symbol, is_raw), .. },
-            _,
-        )) => {
-            if let IdentIsRaw::Yes = is_raw {
-                Err(dcx.struct_span_err(span_err, RAW_IDENT_ERR))
-            } else {
-                Ok(*symbol)
-            }
+    metavar_span: Span,
+) -> PResult<'tx, String> {
+    // Reconstruct a `Token` so we can share logic with expression parsing.
+    let token = match pnr {
+        ParseNtResult::Tt(TokenTree::Token(tok, _)) => *tok,
+        ParseNtResult::Ident(Ident { name, span }, ident_is_raw) => {
+            Token { kind: TokenKind::Ident(*name, *ident_is_raw), span: *span }
+        }
+
+        ParseNtResult::Literal(l) if let ExprKind::Lit(lit) = l.kind => {
+            Token { kind: TokenKind::Literal(lit), span: l.span }
+        }
+        _ => {
+            let err = errors::MveConcatInvalidTy {
+                span: pnr.span(),
+                metavar_span: Some(metavar_span),
+                reason: errors::MveConcatInvalidTyReason::InvalidMetavarTy,
+                valid: VALID_METAVAR_CONCAT_TYPES,
+            };
+            return Err(tscx.psess.dcx().create_err(err));
         }
-        ParseNtResult::Tt(TokenTree::Token(
-            Token {
-                kind: TokenKind::Literal(Lit { kind: LitKind::Str, symbol, suffix: None }),
-                ..
-            },
-            _,
-        )) => Ok(*symbol),
-        ParseNtResult::Literal(expr)
-            if let ExprKind::Lit(Lit { kind: LitKind::Str, symbol, suffix: None }) = &expr.kind =>
-        {
-            Ok(*symbol)
-        }
-        _ => Err(dcx
-            .struct_err(
-                "metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt`",
-            )
-            .with_note("currently only string literals are supported")
-            .with_span(span_err)),
-    }
+    };
+
+    parse_tok_for_concat(tscx.psess, token)
+    //     _ => Err(dcx
+    //         .struct_err(
+    //             "metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt`",
+    //         )
+    //         .with_note("currently only string literals are supported")
+    //         .with_span(span_err)),
 }

compiler/rustc_parse/src/parser/mod.rs

@@ -1657,3 +1657,24 @@ pub enum ParseNtResult {
     Path(P<ast::Path>),
     Vis(P<ast::Visibility>),
 }
+
+impl ParseNtResult {
+    #[inline]
+    pub fn span(&self) -> Span {
+        match self {
+            ParseNtResult::Tt(token_tree) => token_tree.span(),
+            ParseNtResult::Ident(ident, ..) => ident.span,
+            ParseNtResult::Lifetime(ident, ..) => ident.span,
+            ParseNtResult::Item(item) => item.span,
+            ParseNtResult::Block(block) => block.span,
+            ParseNtResult::Stmt(stmt) => stmt.span,
+            ParseNtResult::Pat(pat, ..) => pat.span,
+            ParseNtResult::Expr(expr, ..) => expr.span,
+            ParseNtResult::Literal(expr) => expr.span,
+            ParseNtResult::Ty(ty) => ty.span,
+            ParseNtResult::Meta(attr_item) => attr_item.span(),
+            ParseNtResult::Path(path) => path.span,
+            ParseNtResult::Vis(visibility) => visibility.span,
+        }
+    }
+}