Skip to content

Commit

Permalink
Remove NtPat.
Browse files Browse the repository at this point in the history
The one notable test change is `tests/ui/macros/trace_faulty_macros.rs`.
This commit removes the complicated `Interpolated` handling in
`expected_expression_found` that results in a longer error message. But
I think the new, shorter message is actually an improvement.

The original complaint was in rust-lang#71039, when the error message started
with "error: expected expression, found `1 + 1`". That was confusing
because `1 + 1` is an expression. Other than that, the reporter said
"the whole error message is not too bad if you ignore the first line".

Subsequently, extra complexity and wording was added to the error
message. But I don't think the extra wording actually helps all that
much. In particular, it still says of the `1+1` that "this is expected
to be expression". This repeats the problem from the original complaint!

This commit removes the extra complexity, reverting to a simpler error
message. This is primarily because the traversal a pain without
`Interpolated` tokens. Nonetheless, I think the error message is
*improved*. It now starts with "expected expression, found `pat`
metavariable", which is much clearer and the real problem. It also
doesn't say anything specific about `1+1`, which is good, because the
`1+1` isn't really relevant to the error -- it's the `$e:pat` that's
important.
  • Loading branch information
nnethercote authored and compiler-errors committed Sep 14, 2024
1 parent 23bb0bf commit e97c945
Show file tree
Hide file tree
Showing 16 changed files with 66 additions and 97 deletions.
2 changes: 0 additions & 2 deletions compiler/rustc_ast/src/ast_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ impl HasTokens for Nonterminal {
Nonterminal::NtItem(item) => item.tokens(),
Nonterminal::NtStmt(stmt) => stmt.tokens(),
Nonterminal::NtExpr(expr) | Nonterminal::NtLiteral(expr) => expr.tokens(),
Nonterminal::NtPat(pat) => pat.tokens(),
Nonterminal::NtMeta(attr_item) => attr_item.tokens(),
Nonterminal::NtPath(path) => path.tokens(),
Nonterminal::NtBlock(block) => block.tokens(),
Expand All @@ -213,7 +212,6 @@ impl HasTokens for Nonterminal {
Nonterminal::NtItem(item) => item.tokens_mut(),
Nonterminal::NtStmt(stmt) => stmt.tokens_mut(),
Nonterminal::NtExpr(expr) | Nonterminal::NtLiteral(expr) => expr.tokens_mut(),
Nonterminal::NtPat(pat) => pat.tokens_mut(),
Nonterminal::NtMeta(attr_item) => attr_item.tokens_mut(),
Nonterminal::NtPath(path) => path.tokens_mut(),
Nonterminal::NtBlock(block) => block.tokens_mut(),
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_ast/src/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,6 @@ fn visit_nonterminal<T: MutVisitor>(vis: &mut T, nt: &mut token::Nonterminal) {
vis.flat_map_stmt(stmt).expect_one("expected visitor to produce exactly one item")
})
}),
token::NtPat(pat) => vis.visit_pat(pat),
token::NtExpr(expr) => vis.visit_expr(expr),
token::NtLiteral(expr) => vis.visit_expr(expr),
token::NtMeta(item) => {
Expand Down
5 changes: 0 additions & 5 deletions compiler/rustc_ast/src/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,6 @@ impl Token {
| NtExpr(..)
| NtLiteral(..)
| NtMeta(..)
| NtPat(..)
| NtPath(..)
),
OpenDelim(Delimiter::Invisible(InvisibleOrigin::MetaVar(
Expand Down Expand Up @@ -1061,7 +1060,6 @@ pub enum Nonterminal {
NtItem(P<ast::Item>),
NtBlock(P<ast::Block>),
NtStmt(P<ast::Stmt>),
NtPat(P<ast::Pat>),
NtExpr(P<ast::Expr>),
NtLiteral(P<ast::Expr>),
/// Stuff inside brackets for attributes
Expand Down Expand Up @@ -1158,7 +1156,6 @@ impl Nonterminal {
NtItem(item) => item.span,
NtBlock(block) => block.span,
NtStmt(stmt) => stmt.span,
NtPat(pat) => pat.span,
NtExpr(expr) | NtLiteral(expr) => expr.span,
NtMeta(attr_item) => attr_item.span(),
NtPath(path) => path.span,
Expand All @@ -1170,7 +1167,6 @@ impl Nonterminal {
NtItem(..) => "item",
NtBlock(..) => "block",
NtStmt(..) => "statement",
NtPat(..) => "pattern",
NtExpr(..) => "expression",
NtLiteral(..) => "literal",
NtMeta(..) => "attribute",
Expand All @@ -1195,7 +1191,6 @@ impl fmt::Debug for Nonterminal {
NtItem(..) => f.pad("NtItem(..)"),
NtBlock(..) => f.pad("NtBlock(..)"),
NtStmt(..) => f.pad("NtStmt(..)"),
NtPat(..) => f.pad("NtPat(..)"),
NtExpr(..) => f.pad("NtExpr(..)"),
NtLiteral(..) => f.pad("NtLiteral(..)"),
NtMeta(..) => f.pad("NtMeta(..)"),
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_ast/src/tokenstream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,6 @@ impl TokenStream {
TokenStream::token_alone(token::Semi, stmt.span)
}
Nonterminal::NtStmt(stmt) => TokenStream::from_ast(stmt),
Nonterminal::NtPat(pat) => TokenStream::from_ast(pat),
Nonterminal::NtMeta(attr) => TokenStream::from_ast(attr),
Nonterminal::NtPath(path) => TokenStream::from_ast(path),
Nonterminal::NtExpr(expr) | Nonterminal::NtLiteral(expr) => TokenStream::from_ast(expr),
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_expand/src/mbe/transcribe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ pub(super) fn transcribe<'a>(
let kind = token::NtLifetime(*ident, *is_raw);
TokenTree::token_alone(kind, sp)
}
MatchedSingle(ParseNtResult::Pat(pat, pat_kind)) => {
mk_delimited(MetaVarKind::Pat(*pat_kind), TokenStream::from_ast(pat))
}
MatchedSingle(ParseNtResult::Ty(ty)) => {
mk_delimited(MetaVarKind::Ty, TokenStream::from_ast(ty))
}
Expand Down
51 changes: 2 additions & 49 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ use ast::token::IdentIsRaw;
use rustc_ast as ast;
use rustc_ast::ptr::P;
use rustc_ast::token::{self, Delimiter, Lit, LitKind, Token, TokenKind};
use rustc_ast::tokenstream::AttrTokenTree;
use rustc_ast::util::parser::AssocOp;
use rustc_ast::{
AngleBracketedArg, AngleBracketedArgs, AnonConst, AttrVec, BinOpKind, BindingMode, Block,
BlockCheckMode, Expr, ExprKind, GenericArg, Generics, HasTokens, Item, ItemKind, Param, Pat,
PatKind, Path, PathSegment, QSelf, Recovered, Ty, TyKind,
BlockCheckMode, Expr, ExprKind, GenericArg, Generics, Item, ItemKind, Param, Pat, PatKind,
Path, PathSegment, QSelf, Recovered, Ty, TyKind,
};
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxHashSet;
Expand Down Expand Up @@ -2426,52 +2425,6 @@ impl<'a> Parser<'a> {
err.subdiagnostic(ExprParenthesesNeeded::surrounding(*sp));
}
err.span_label(span, "expected expression");

// Walk the chain of macro expansions for the current token to point at how the original
// code was interpreted. This helps the user realize when a macro argument of one type is
// later reinterpreted as a different type, like `$x:expr` being reinterpreted as `$x:pat`
// in a subsequent macro invocation (#71039).
let mut tok = self.token.clone();
let mut labels = vec![];
while let TokenKind::Interpolated(nt) = &tok.kind {
let tokens = nt.tokens();
labels.push(nt.clone());
if let Some(tokens) = tokens
&& let tokens = tokens.to_attr_token_stream()
&& let tokens = tokens.0.deref()
&& let [AttrTokenTree::Token(token, _)] = &tokens[..]
{
tok = token.clone();
} else {
break;
}
}
let mut iter = labels.into_iter().peekable();
let mut show_link = false;
while let Some(nt) = iter.next() {
let descr = nt.descr();
if let Some(next) = iter.peek() {
let next_descr = next.descr();
if next_descr != descr {
err.span_label(next.use_span(), format!("this is expected to be {next_descr}"));
err.span_label(
nt.use_span(),
format!(
"this is interpreted as {}, but it is expected to be {}",
next_descr, descr,
),
);
show_link = true;
}
}
}
if show_link {
err.note(
"when forwarding a matched fragment to another macro-by-example, matchers in the \
second macro will see an opaque AST of the fragment type, not the underlying \
tokens",
);
}
err
}

Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_parse/src/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::mem;
use ast::token::IdentIsRaw;
use rustc_ast::ast::*;
use rustc_ast::ptr::P;
use rustc_ast::token::{self, Delimiter, TokenKind};
use rustc_ast::token::{self, Delimiter, InvisibleOrigin, MetaVarKind, TokenKind};
use rustc_ast::tokenstream::{DelimSpan, TokenStream, TokenTree};
use rustc_ast::util::case::Case;
use rustc_ast::{self as ast};
Expand Down Expand Up @@ -2976,8 +2976,10 @@ impl<'a> Parser<'a> {

fn is_named_param(&self) -> bool {
let offset = match &self.token.kind {
token::Interpolated(nt) => match &**nt {
token::NtPat(..) => return self.look_ahead(1, |t| t == &token::Colon),
token::OpenDelim(Delimiter::Invisible(origin)) => match origin {
InvisibleOrigin::MetaVar(MetaVarKind::Pat(_)) => {
return self.check_noexpect_past_close_delim(&token::Colon);
}
_ => 0,
},
token::BinOp(token::And) | token::AndAnd => 1,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ pub use pat::{CommaRecoveryMode, RecoverColon, RecoverComma};
use path::PathStyle;
use rustc_ast::ptr::P;
use rustc_ast::token::{
self, Delimiter, IdentIsRaw, InvisibleOrigin, MetaVarKind, Nonterminal, Token, TokenKind,
self, Delimiter, IdentIsRaw, InvisibleOrigin, MetaVarKind, Nonterminal, NtPatKind, Token,
TokenKind,
};
use rustc_ast::tokenstream::{
AttrsTarget, DelimSpacing, DelimSpan, Spacing, TokenStream, TokenTree, TokenTreeCursor,
Expand Down Expand Up @@ -1747,6 +1748,7 @@ pub enum ParseNtResult {
Tt(TokenTree),
Ident(Ident, IdentIsRaw),
Lifetime(Ident, IdentIsRaw),
Pat(P<ast::Pat>, NtPatKind),
Ty(P<ast::Ty>),
Vis(P<ast::Visibility>),

Expand Down
24 changes: 13 additions & 11 deletions compiler/rustc_parse/src/parser/nonterminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ impl<'a> Parser<'a> {
fn nt_may_be_ident(nt: &Nonterminal) -> bool {
match nt {
NtStmt(_)
| NtPat(_)
| NtExpr(_)
| NtLiteral(_) // `true`, `false`
| NtMeta(_)
Expand Down Expand Up @@ -99,7 +98,7 @@ impl<'a> Parser<'a> {
token::NtLifetime(..) => true,
token::Interpolated(nt) => match &**nt {
NtBlock(_) | NtStmt(_) | NtExpr(_) | NtLiteral(_) => true,
NtItem(_) | NtPat(_) | NtMeta(_) | NtPath(_) => false,
NtItem(_) | NtMeta(_) | NtPath(_) => false,
},
token::OpenDelim(Delimiter::Invisible(InvisibleOrigin::MetaVar(k))) => match k {
MetaVarKind::Block
Expand Down Expand Up @@ -170,15 +169,18 @@ impl<'a> Parser<'a> {
}
},
NonterminalKind::Pat(pat_kind) => {
NtPat(self.collect_tokens_no_attrs(|this| match pat_kind {
PatParam { .. } => this.parse_pat_no_top_alt(None, None),
PatWithOr => this.parse_pat_allow_top_alt(
None,
RecoverComma::No,
RecoverColon::No,
CommaRecoveryMode::EitherTupleOrPipe,
),
})?)
return Ok(ParseNtResult::Pat(
self.collect_tokens_no_attrs(|this| match pat_kind {
PatParam { .. } => this.parse_pat_no_top_alt(None, None),
PatWithOr => this.parse_pat_allow_top_alt(
None,
RecoverComma::No,
RecoverColon::No,
CommaRecoveryMode::EitherTupleOrPipe,
),
})?,
pat_kind,
));
}
NonterminalKind::Expr(_) => NtExpr(self.parse_expr_force_collect()?),
NonterminalKind::Literal => {
Expand Down
37 changes: 30 additions & 7 deletions compiler/rustc_parse/src/parser/pat.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use rustc_ast::mut_visit::{walk_pat, MutVisitor};
use rustc_ast::ptr::P;
use rustc_ast::token::{self, BinOpToken, Delimiter, IdentIsRaw, Token};
use rustc_ast::token::NtPatKind::*;
use rustc_ast::token::{self, BinOpToken, Delimiter, IdentIsRaw, MetaVarKind, Token};
use rustc_ast::{
self as ast, AttrVec, BindingMode, ByRef, Expr, ExprKind, MacCall, Mutability, Pat, PatField,
PatFieldsRest, PatKind, Path, QSelf, RangeEnd, RangeSyntax,
Expand All @@ -25,8 +26,8 @@ use crate::errors::{
UnexpectedParenInRangePat, UnexpectedParenInRangePatSugg,
UnexpectedVertVertBeforeFunctionParam, UnexpectedVertVertInPattern, WrapInParens,
};
use crate::maybe_recover_from_interpolated_ty_qpath;
use crate::parser::expr::could_be_unclosed_char_literal;
use crate::{maybe_recover_from_interpolated_ty_qpath, maybe_whole};

#[derive(PartialEq, Copy, Clone)]
pub enum Expected {
Expand Down Expand Up @@ -431,6 +432,27 @@ impl<'a> Parser<'a> {
None
}

fn eat_metavar_pat(&mut self) -> Option<P<Pat>> {
// Must try both kinds of pattern nonterminals.
if let Some(pat) = self.eat_metavar_seq_with_matcher(
|mv_kind| matches!(mv_kind, MetaVarKind::Pat(PatParam { .. })),
|this| this.parse_pat_no_top_alt(None, None),
) {
Some(pat)
} else if let Some(pat) = self.eat_metavar_seq(MetaVarKind::Pat(PatWithOr), |this| {
this.parse_pat_allow_top_alt(
None,
RecoverComma::No,
RecoverColon::No,
CommaRecoveryMode::EitherTupleOrPipe,
)
}) {
Some(pat)
} else {
None
}
}

/// Parses a pattern, with a setting whether modern range patterns (e.g., `a..=b`, `a..b` are
/// allowed).
fn parse_pat_with_range_pat(
Expand All @@ -440,7 +462,10 @@ impl<'a> Parser<'a> {
syntax_loc: Option<PatternLocation>,
) -> PResult<'a, P<Pat>> {
maybe_recover_from_interpolated_ty_qpath!(self, true);
maybe_whole!(self, NtPat, |pat| pat);

if let Some(pat) = self.eat_metavar_pat() {
return Ok(pat);
}

let mut lo = self.token.span;

Expand Down Expand Up @@ -776,10 +801,8 @@ impl<'a> Parser<'a> {
self.recover_additional_muts();

// Make sure we don't allow e.g. `let mut $p;` where `$p:pat`.
if let token::Interpolated(nt) = &self.token.kind {
if let token::NtPat(..) = &**nt {
self.expected_ident_found_err().emit();
}
if let Some(MetaVarKind::Pat(_)) = self.token.is_metavar_seq() {
self.expected_ident_found_err().emit();
}

// Parse the pattern we hope to be an identifier.
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/macros/trace_faulty_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ macro_rules! test {
(let $p:pat = $e:expr) => {test!(($p,$e))};
// this should be expr
// vvv
(($p:pat, $e:pat)) => {let $p = $e;}; //~ ERROR expected expression, found pattern `1+1`
(($p:pat, $e:pat)) => {let $p = $e;}; //~ ERROR expected expression, found `pat` metavariable
}

fn foo() {
Expand Down
13 changes: 3 additions & 10 deletions tests/ui/macros/trace_faulty_macros.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ LL | my_recursive_macro!();
= note: expanding `my_recursive_macro! { }`
= note: to `my_recursive_macro! ();`

error: expected expression, found pattern `A { a : a, b : 0, c : _, .. }`
error: expected expression, found `pat` metavariable
--> $DIR/trace_faulty_macros.rs:16:9
|
LL | $a
Expand All @@ -69,22 +69,15 @@ LL | #[derive(Debug)]
LL | fn use_derive_macro_as_attr() {}
| -------------------------------- not a `struct`, `enum` or `union`

error: expected expression, found pattern `1+1`
error: expected expression, found `pat` metavariable
--> $DIR/trace_faulty_macros.rs:49:37
|
LL | (let $p:pat = $e:expr) => {test!(($p,$e))};
| -- this is interpreted as expression, but it is expected to be pattern
...
LL | (($p:pat, $e:pat)) => {let $p = $e;};
| ^^ expected expression
...
LL | test!(let x = 1+1);
| ------------------
| | |
| | this is expected to be expression
| in this macro invocation
| ------------------ in this macro invocation
|
= note: when forwarding a matched fragment to another macro-by-example, matchers in the second macro will see an opaque AST of the fragment type, not the underlying tokens
= note: this error originates in the macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: trace_macro
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ macro_rules! mac2 {
($eval:pat) => {
let mut $eval = ();
//~^ ERROR `mut` must be followed by a named binding
//~| ERROR expected identifier, found `does_not_exist!()`
//~| ERROR expected identifier, found metavariable
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ LL - let mut $eval = ();
LL + let $eval = ();
|

error: expected identifier, found `does_not_exist!()`
error: expected identifier, found metavariable
--> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:13:17
|
LL | let mut $eval = ();
| ^^^^^ expected identifier
| ^^^^^ expected identifier, found metavariable
...
LL | mac2! { does_not_exist!() }
| --------------------------- in this macro invocation
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/parser/mut-patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub fn main() {
// Make sure we don't accidentally allow `mut $p` where `$p:pat`.
macro_rules! foo {
($p:pat) => {
let mut $p = 0; //~ ERROR expected identifier, found `x`
let mut $p = 0; //~ ERROR expected identifier, found metavariable
}
}
foo!(x);
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/parser/mut-patterns.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ help: add `mut` to each binding
LL | let W(mut a, W(mut b, W(ref c, W(mut d, B { box mut f }))))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: expected identifier, found `x`
error: expected identifier, found metavariable
--> $DIR/mut-patterns.rs:44:21
|
LL | let mut $p = 0;
| ^^ expected identifier
| ^^ expected identifier, found metavariable
...
LL | foo!(x);
| ------- in this macro invocation
Expand Down

0 comments on commit e97c945

Please sign in to comment.