From 8d13b2a046d35e657c7e497cfeda6b2165dc931f Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 21:39:57 +0200 Subject: [PATCH 1/7] Store `ErrorGuaranteed` in `ErrorReported` --- compiler/rustc_expand/src/mbe/macro_parser.rs | 7 ++++--- compiler/rustc_expand/src/mbe/macro_rules.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index c8bdc39311c65..aa7a06f66d722 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -72,6 +72,7 @@ pub(crate) use NamedMatch::*; pub(crate) use ParseResult::*; +use rustc_errors::ErrorGuaranteed; use crate::mbe::{KleeneOp, TokenTree}; @@ -270,7 +271,7 @@ pub(crate) enum ParseResult { Failure(Token, &'static str), /// Fatal error (malformed macro?). Abort compilation. Error(rustc_span::Span, String), - ErrorReported, + ErrorReported(ErrorGuaranteed), } /// A `ParseResult` where the `Success` variant contains a mapping of @@ -612,14 +613,14 @@ impl TtParser { // edition-specific matching behavior for non-terminals. let nt = match parser.to_mut().parse_nonterminal(kind) { Err(mut err) => { - err.span_label( + let guarantee = err.span_label( span, format!( "while parsing argument for this `{kind}` macro fragment" ), ) .emit(); - return ErrorReported; + return ErrorReported(guarantee); } Ok(nt) => nt, }; diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index f6fe38174f7c5..3ddea80c84445 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -333,7 +333,7 @@ fn expand_macro<'cx>( cx.struct_span_err(span, &msg).emit(); return DummyResult::any(span); } - ErrorReported => return DummyResult::any(sp), + ErrorReported(_) => return DummyResult::any(sp), } // The matcher was not `Success(..)`ful. @@ -470,7 +470,7 @@ pub fn compile_declarative_macro( .emit(); return dummy_syn_ext(); } - ErrorReported => { + ErrorReported(_) => { return dummy_syn_ext(); } }; From 6c47848c2553ea2703ef12e1e767cf583ff390c6 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 21:43:48 +0200 Subject: [PATCH 2/7] Small parser cleanups --- compiler/rustc_expand/src/mbe/macro_parser.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index aa7a06f66d722..6513928e78c85 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -451,7 +451,7 @@ impl TtParser { // Try zero matches of this sequence, by skipping over it. self.cur_mps.push(MatcherPos { idx: idx_first_after, - matches: mp.matches.clone(), // a cheap clone + matches: Lrc::clone(&mp.matches), }); } @@ -464,8 +464,8 @@ impl TtParser { // sequence. If that's not possible, `ending_mp` will fail quietly when it is // processed next time around the loop. let ending_mp = MatcherPos { - idx: mp.idx + 1, // +1 skips the Kleene op - matches: mp.matches.clone(), // a cheap clone + idx: mp.idx + 1, // +1 skips the Kleene op + matches: Lrc::clone(&mp.matches), }; self.cur_mps.push(ending_mp); @@ -480,8 +480,8 @@ impl TtParser { // separator yet. Try ending the sequence. If that's not possible, `ending_mp` // will fail quietly when it is processed next time around the loop. let ending_mp = MatcherPos { - idx: mp.idx + 2, // +2 skips the separator and the Kleene op - matches: mp.matches.clone(), // a cheap clone + idx: mp.idx + 2, // +2 skips the separator and the Kleene op + matches: Lrc::clone(&mp.matches), }; self.cur_mps.push(ending_mp); From 2f8a068cb760d0845e59b39444cb67479b2d2163 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 21:48:28 +0200 Subject: [PATCH 3/7] Add `Tracker` to track matching operations This should allow us to collect detailed information without slowing down the inital hot path. --- compiler/rustc_expand/src/mbe.rs | 2 +- compiler/rustc_expand/src/mbe/macro_parser.rs | 32 +++++++++++-------- compiler/rustc_expand/src/mbe/macro_rules.rs | 31 ++++++++++++++++-- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_expand/src/mbe.rs b/compiler/rustc_expand/src/mbe.rs index f42576b16f520..63bafd7b046fb 100644 --- a/compiler/rustc_expand/src/mbe.rs +++ b/compiler/rustc_expand/src/mbe.rs @@ -52,7 +52,7 @@ impl KleeneToken { /// A Kleene-style [repetition operator](https://en.wikipedia.org/wiki/Kleene_star) /// for token sequences. #[derive(Clone, PartialEq, Encodable, Decodable, Debug, Copy)] -enum KleeneOp { +pub(crate) enum KleeneOp { /// Kleene star (`*`) for zero or more repetitions ZeroOrMore, /// Kleene plus (`+`) for one or more repetitions diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index 6513928e78c85..0402c51247af0 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -70,21 +70,20 @@ //! eof: [a $( a )* a b ·] //! ``` +use rustc_errors::ErrorGuaranteed; pub(crate) use NamedMatch::*; pub(crate) use ParseResult::*; -use rustc_errors::ErrorGuaranteed; -use crate::mbe::{KleeneOp, TokenTree}; +use crate::mbe::{macro_rules::Tracker, KleeneOp, TokenTree}; use rustc_ast::token::{self, DocComment, Nonterminal, NonterminalKind, Token}; +use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::sync::Lrc; use rustc_lint_defs::pluralize; use rustc_parse::parser::{NtOrTt, Parser}; +use rustc_span::symbol::Ident; use rustc_span::symbol::MacroRulesNormalizedIdent; use rustc_span::Span; - -use rustc_data_structures::fx::FxHashMap; -use rustc_data_structures::sync::Lrc; -use rustc_span::symbol::Ident; use std::borrow::Cow; use std::collections::hash_map::Entry::{Occupied, Vacant}; @@ -97,7 +96,8 @@ use std::collections::hash_map::Entry::{Occupied, Vacant}; /// /// This means a matcher can be represented by `&[MatcherLoc]`, and traversal mostly involves /// simply incrementing the current matcher position index by one. -pub(super) enum MatcherLoc { +#[derive(Debug, Clone, PartialEq)] +pub(crate) enum MatcherLoc { Token { token: Token, }, @@ -401,17 +401,21 @@ impl TtParser { /// /// `Some(result)` if everything is finished, `None` otherwise. Note that matches are kept /// track of through the mps generated. - fn parse_tt_inner( + fn parse_tt_inner<'matcher, T: Tracker<'matcher>>( &mut self, - matcher: &[MatcherLoc], + matcher: &'matcher [MatcherLoc], token: &Token, + track: &mut T, ) -> Option { // Matcher positions that would be valid if the macro invocation was over now. Only // modified if `token == Eof`. let mut eof_mps = EofMatcherPositions::None; while let Some(mut mp) = self.cur_mps.pop() { - match &matcher[mp.idx] { + let matcher_loc = &matcher[mp.idx]; + track.before_match_loc(self, matcher_loc); + + match matcher_loc { MatcherLoc::Token { token: t } => { // If it's a doc comment, we just ignore it and move on to the next tt in the // matcher. This is a bug, but #95267 showed that existing programs rely on @@ -553,10 +557,11 @@ impl TtParser { } /// Match the token stream from `parser` against `matcher`. - pub(super) fn parse_tt( + pub(super) fn parse_tt<'matcher, T: Tracker<'matcher>>( &mut self, parser: &mut Cow<'_, Parser<'_>>, - matcher: &[MatcherLoc], + matcher: &'matcher [MatcherLoc], + track: &mut T, ) -> NamedParseResult { // A queue of possible matcher positions. We initialize it with the matcher position in // which the "dot" is before the first token of the first token tree in `matcher`. @@ -572,7 +577,8 @@ impl TtParser { // Process `cur_mps` until either we have finished the input or we need to get some // parsing from the black-box parser done. - if let Some(res) = self.parse_tt_inner(matcher, &parser.token) { + let res = self.parse_tt_inner(matcher, &parser.token, track); + if let Some(res) = res { return res; } diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index 3ddea80c84445..9c676a41f3a88 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -33,6 +33,8 @@ use std::borrow::Cow; use std::collections::hash_map::Entry; use std::{mem, slice}; +use super::macro_parser::NamedParseResult; + pub(crate) struct ParserAnyMacro<'a> { parser: Parser<'a>, @@ -205,6 +207,29 @@ fn trace_macros_note(cx_expansions: &mut FxIndexMap>, sp: Span cx_expansions.entry(sp).or_default().push(message); } +pub(super) trait Tracker<'matcher> { + /// This is called before trying to match next MatcherLoc on the current token + fn before_match_loc(&mut self, parser: &TtParser, matcher: &'matcher MatcherLoc); + + /// This is called after an arm has been parsed, either successfully or unsuccessfully. When this is called, + /// `before_match_loc` was called at least once (with a `MatcherLoc::Eof`) + fn after_arm(&mut self, result: &NamedParseResult); + + /// For tracing + fn description() -> &'static str; +} + +/// A noop tracker that is used in the hot path of the expansion, has zero overhead thanks to monomorphization +struct NoopTracker; + +impl<'matcher> Tracker<'matcher> for NoopTracker { + fn before_match_loc(&mut self, _: &TtParser, _: &'matcher MatcherLoc) {} + fn after_arm(&mut self, _: &NamedParseResult) {} + fn description() -> &'static str { + "none" + } +} + /// Expands the rules based macro defined by `lhses` and `rhses` for a given /// input `arg`. fn expand_macro<'cx>( @@ -262,7 +287,7 @@ fn expand_macro<'cx>( // are not recorded. On the first `Success(..)`ful matcher, the spans are merged. let mut gated_spans_snapshot = mem::take(&mut *sess.gated_spans.spans.borrow_mut()); - match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs) { + match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, &mut NoopTracker) { Success(named_matches) => { // The matcher was `Success(..)`ful. // Merge the gated spans from parsing the matcher with the pre-existing ones. @@ -354,7 +379,7 @@ fn expand_macro<'cx>( if let Some((arg, comma_span)) = arg.add_comma() { for lhs in lhses { let parser = parser_from_cx(sess, arg.clone()); - if let Success(_) = tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs) { + if let Success(_) = tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, &mut NoopTracker) { if comma_span.is_dummy() { err.note("you might be missing a comma"); } else { @@ -452,7 +477,7 @@ pub fn compile_declarative_macro( let parser = Parser::new(&sess.parse_sess, body, true, rustc_parse::MACRO_ARGUMENTS); let mut tt_parser = TtParser::new(Ident::with_dummy_span(if macro_rules { kw::MacroRules } else { kw::Macro })); - let argument_map = match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), &argument_gram) { + let argument_map = match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), &argument_gram, &mut NoopTracker) { Success(m) => m, Failure(token, msg) => { let s = parse_failure_msg(&token); From 39584b153b5f0b86a7efda9bba0a7d2cbe9b56e2 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 21:55:32 +0200 Subject: [PATCH 4/7] Factor out matching into `try_match_macro` This moves out the matching part of expansion into a new function. This function will try to match the macro and return an error if it failed to match. A tracker can be used to get more information about the matching. --- compiler/rustc_expand/src/mbe/macro_parser.rs | 6 +- compiler/rustc_expand/src/mbe/macro_rules.rs | 247 +++++++++--------- 2 files changed, 129 insertions(+), 124 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index 0402c51247af0..d2dbd190c8ec2 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -277,7 +277,11 @@ pub(crate) enum ParseResult { /// A `ParseResult` where the `Success` variant contains a mapping of /// `MacroRulesNormalizedIdent`s to `NamedMatch`es. This represents the mapping /// of metavars to the token trees they bind to. -pub(crate) type NamedParseResult = ParseResult>; +pub(crate) type NamedParseResult = ParseResult; + +/// Contains a mapping of `MacroRulesNormalizedIdent`s to `NamedMatch`es. +/// This represents the mapping of metavars to the token trees they bind to. +pub(crate) type NamedMatches = FxHashMap; /// Count how many metavars declarations are in `matcher`. pub(super) fn count_metavar_decls(matcher: &[TokenTree]) -> usize { diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index 9c676a41f3a88..b35ec453145f3 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -14,7 +14,9 @@ use rustc_ast::{NodeId, DUMMY_NODE_ID}; use rustc_ast_pretty::pprust; use rustc_attr::{self as attr, TransparencyError}; use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; -use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, DiagnosticMessage}; +use rustc_errors::{ + Applicability, Diagnostic, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, +}; use rustc_feature::Features; use rustc_lint_defs::builtin::{ RUST_2021_INCOMPATIBLE_OR_PATTERNS, SEMICOLON_IN_EXPRESSIONS_FROM_MACROS, @@ -33,7 +35,7 @@ use std::borrow::Cow; use std::collections::hash_map::Entry; use std::{mem, slice}; -use super::macro_parser::NamedParseResult; +use super::macro_parser::{NamedMatches, NamedParseResult}; pub(crate) struct ParserAnyMacro<'a> { parser: Parser<'a>, @@ -253,9 +255,87 @@ fn expand_macro<'cx>( trace_macros_note(&mut cx.expansions, sp, msg); } - // Which arm's failure should we report? (the one furthest along) - let mut best_failure: Option<(Token, &str)> = None; + // Track nothing for the best performance + let try_success_result = try_match_macro(sess, name, &arg, lhses, &mut NoopTracker); + + match try_success_result { + Ok((i, named_matches)) => { + let (rhs, rhs_span): (&mbe::Delimited, DelimSpan) = match &rhses[i] { + mbe::TokenTree::Delimited(span, delimited) => (&delimited, *span), + _ => cx.span_bug(sp, "malformed macro rhs"), + }; + let arm_span = rhses[i].span(); + + let rhs_spans = rhs.tts.iter().map(|t| t.span()).collect::>(); + // rhs has holes ( `$id` and `$(...)` that need filled) + let mut tts = match transcribe(cx, &named_matches, &rhs, rhs_span, transparency) { + Ok(tts) => tts, + Err(mut err) => { + err.emit(); + return DummyResult::any(arm_span); + } + }; + + // Replace all the tokens for the corresponding positions in the macro, to maintain + // proper positions in error reporting, while maintaining the macro_backtrace. + if rhs_spans.len() == tts.len() { + tts = tts.map_enumerated(|i, tt| { + let mut tt = tt.clone(); + let mut sp = rhs_spans[i]; + sp = sp.with_ctxt(tt.span().ctxt()); + tt.set_span(sp); + tt + }); + } + + if cx.trace_macros() { + let msg = format!("to `{}`", pprust::tts_to_string(&tts)); + trace_macros_note(&mut cx.expansions, sp, msg); + } + + let mut p = Parser::new(sess, tts, false, None); + p.last_type_ascription = cx.current_expansion.prior_type_ascription; + + if is_local { + cx.resolver.record_macro_rule_usage(node_id, i); + } + + // Let the context choose how to interpret the result. + // Weird, but useful for X-macros. + return Box::new(ParserAnyMacro { + parser: p, + + // Pass along the original expansion site and the name of the macro + // so we can print a useful error message if the parse of the expanded + // macro leaves unparsed tokens. + site_span: sp, + macro_ident: name, + lint_node_id: cx.current_expansion.lint_node_id, + is_trailing_mac: cx.current_expansion.is_trailing_mac, + arm_span, + is_local, + }); + } + Err(()) => { + todo!("Retry macro invocation while tracking diagnostics info and emit error"); + + return DummyResult::any(sp); + } + } + + DummyResult::any(sp) +} +/// Try expanding the macro. Returns the index of the sucessful arm and its named_matches if it was successful, +/// and nothing if it failed. On failure, it's the callers job to use `track` accordingly to record all errors +/// correctly. +fn try_match_macro<'matcher, T: Tracker<'matcher>>( + sess: &ParseSess, + name: Ident, + arg: &TokenStream, + lhses: &'matcher [Vec], + track: &mut T, +) -> Result<(usize, NamedMatches), ()> { // We create a base parser that can be used for the "black box" parts. // Every iteration needs a fresh copy of that parser. However, the parser // is not mutated on many of the iterations, particularly when dealing with @@ -277,7 +357,6 @@ fn expand_macro<'cx>( // this situation.) // FIXME(Nilstrieb): Stop recovery from happening on this parser and retry later with recovery if the macro failed to match. let parser = parser_from_cx(sess, arg.clone()); - // Try each arm's matchers. let mut tt_parser = TtParser::new(name); for (i, lhs) in lhses.iter().enumerate() { @@ -287,115 +366,36 @@ fn expand_macro<'cx>( // are not recorded. On the first `Success(..)`ful matcher, the spans are merged. let mut gated_spans_snapshot = mem::take(&mut *sess.gated_spans.spans.borrow_mut()); - match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, &mut NoopTracker) { + let result = tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, track); + + track.after_arm(&result); + + match result { Success(named_matches) => { // The matcher was `Success(..)`ful. // Merge the gated spans from parsing the matcher with the pre-existing ones. sess.gated_spans.merge(gated_spans_snapshot); - let (rhs, rhs_span): (&mbe::Delimited, DelimSpan) = match &rhses[i] { - mbe::TokenTree::Delimited(span, delimited) => (&delimited, *span), - _ => cx.span_bug(sp, "malformed macro rhs"), - }; - let arm_span = rhses[i].span(); - - let rhs_spans = rhs.tts.iter().map(|t| t.span()).collect::>(); - // rhs has holes ( `$id` and `$(...)` that need filled) - let mut tts = match transcribe(cx, &named_matches, &rhs, rhs_span, transparency) { - Ok(tts) => tts, - Err(mut err) => { - err.emit(); - return DummyResult::any(arm_span); - } - }; - - // Replace all the tokens for the corresponding positions in the macro, to maintain - // proper positions in error reporting, while maintaining the macro_backtrace. - if rhs_spans.len() == tts.len() { - tts = tts.map_enumerated(|i, tt| { - let mut tt = tt.clone(); - let mut sp = rhs_spans[i]; - sp = sp.with_ctxt(tt.span().ctxt()); - tt.set_span(sp); - tt - }); - } - - if cx.trace_macros() { - let msg = format!("to `{}`", pprust::tts_to_string(&tts)); - trace_macros_note(&mut cx.expansions, sp, msg); - } - - let mut p = Parser::new(sess, tts, false, None); - p.last_type_ascription = cx.current_expansion.prior_type_ascription; - - if is_local { - cx.resolver.record_macro_rule_usage(node_id, i); - } - - // Let the context choose how to interpret the result. - // Weird, but useful for X-macros. - return Box::new(ParserAnyMacro { - parser: p, - - // Pass along the original expansion site and the name of the macro - // so we can print a useful error message if the parse of the expanded - // macro leaves unparsed tokens. - site_span: sp, - macro_ident: name, - lint_node_id: cx.current_expansion.lint_node_id, - is_trailing_mac: cx.current_expansion.is_trailing_mac, - arm_span, - is_local, - }); + return Ok((i, named_matches)); } - Failure(token, msg) => match best_failure { - Some((ref best_token, _)) if best_token.span.lo() >= token.span.lo() => {} - _ => best_failure = Some((token, msg)), - }, - Error(err_sp, ref msg) => { - let span = err_sp.substitute_dummy(sp); - cx.struct_span_err(span, &msg).emit(); - return DummyResult::any(span); + Failure(_, _) => { + // Try the next arm + } + Error(_, _) => { + // We haven't emitted an error yet + return Err(()); + } + ErrorReported(_) => { + return Err(()); } - ErrorReported(_) => return DummyResult::any(sp), } // The matcher was not `Success(..)`ful. // Restore to the state before snapshotting and maybe try again. mem::swap(&mut gated_spans_snapshot, &mut sess.gated_spans.spans.borrow_mut()); } - drop(parser); - - let (token, label) = best_failure.expect("ran no matchers"); - let span = token.span.substitute_dummy(sp); - let mut err = cx.struct_span_err(span, &parse_failure_msg(&token)); - err.span_label(span, label); - if !def_span.is_dummy() && !cx.source_map().is_imported(def_span) { - err.span_label(cx.source_map().guess_head_span(def_span), "when calling this macro"); - } - annotate_doc_comment(&mut err, sess.source_map(), span); - // Check whether there's a missing comma in this macro call, like `println!("{}" a);` - if let Some((arg, comma_span)) = arg.add_comma() { - for lhs in lhses { - let parser = parser_from_cx(sess, arg.clone()); - if let Success(_) = tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, &mut NoopTracker) { - if comma_span.is_dummy() { - err.note("you might be missing a comma"); - } else { - err.span_suggestion_short( - comma_span, - "missing comma here", - ", ", - Applicability::MachineApplicable, - ); - } - } - } - } - err.emit(); - cx.trace_macros_diag(); - DummyResult::any(sp) + + Err(()) } // Note that macro-by-example's input is also matched against a token tree: @@ -477,28 +477,29 @@ pub fn compile_declarative_macro( let parser = Parser::new(&sess.parse_sess, body, true, rustc_parse::MACRO_ARGUMENTS); let mut tt_parser = TtParser::new(Ident::with_dummy_span(if macro_rules { kw::MacroRules } else { kw::Macro })); - let argument_map = match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), &argument_gram, &mut NoopTracker) { - Success(m) => m, - Failure(token, msg) => { - let s = parse_failure_msg(&token); - let sp = token.span.substitute_dummy(def.span); - let mut err = sess.parse_sess.span_diagnostic.struct_span_err(sp, &s); - err.span_label(sp, msg); - annotate_doc_comment(&mut err, sess.source_map(), sp); - err.emit(); - return dummy_syn_ext(); - } - Error(sp, msg) => { - sess.parse_sess - .span_diagnostic - .struct_span_err(sp.substitute_dummy(def.span), &msg) - .emit(); - return dummy_syn_ext(); - } - ErrorReported(_) => { - return dummy_syn_ext(); - } - }; + let argument_map = + match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), &argument_gram, &mut NoopTracker) { + Success(m) => m, + Failure(token, msg) => { + let s = parse_failure_msg(&token); + let sp = token.span.substitute_dummy(def.span); + let mut err = sess.parse_sess.span_diagnostic.struct_span_err(sp, &s); + err.span_label(sp, msg); + annotate_doc_comment(&mut err, sess.source_map(), sp); + err.emit(); + return dummy_syn_ext(); + } + Error(sp, msg) => { + sess.parse_sess + .span_diagnostic + .struct_span_err(sp.substitute_dummy(def.span), &msg) + .emit(); + return dummy_syn_ext(); + } + ErrorReported(_) => { + return dummy_syn_ext(); + } + }; let mut valid = true; From 5f73eac51bebc2c2a9768a84e029cbd719cdb6dc Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 21:59:09 +0200 Subject: [PATCH 5/7] Retry matching with tracking for diagnostics For now, we only collect the small info for the `best_failure`, but using this tracker, we can easily extend it in the future to track things with more performance overhead. We cannot retry cases where the macro failed with a parser error that was emitted already, as that would cause us to emit the same error to the user twice. --- compiler/rustc_expand/src/mbe/macro_rules.rs | 118 +++++++++++++++++-- 1 file changed, 109 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index b35ec453145f3..b1214543e5db7 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -316,16 +316,116 @@ fn expand_macro<'cx>( is_local, }); } - Err(()) => { - todo!("Retry macro invocation while tracking diagnostics info and emit error"); - + Err(CanRetry::No(_)) => { + debug!("Will not retry matching as an error was emitted already"); return DummyResult::any(sp); } + Err(CanRetry::Yes) => { + // Retry and emit a better error below. + } + } + + // An error occured, try the expansion again, tracking the expansion closely for better diagnostics + let mut tracker = CollectTrackerAndEmitter::new(cx, sp); + + let try_success_result = try_match_macro(sess, name, &arg, lhses, &mut tracker); + assert!(try_success_result.is_err(), "Macro matching returned a success on the second try"); + + if let Some(result) = tracker.result { + // An irrecoverable error occured and has been emitted. + return result; + } + + let Some((token, label)) = tracker.best_failure else { + return tracker.result.expect("must have encountered Error or ErrorReported"); + }; + + let span = token.span.substitute_dummy(sp); + + let mut err = cx.struct_span_err(span, &parse_failure_msg(&token)); + err.span_label(span, label); + if !def_span.is_dummy() && !cx.source_map().is_imported(def_span) { + err.span_label(cx.source_map().guess_head_span(def_span), "when calling this macro"); } + annotate_doc_comment(&mut err, sess.source_map(), span); + + // Check whether there's a missing comma in this macro call, like `println!("{}" a);` + if let Some((arg, comma_span)) = arg.add_comma() { + for lhs in lhses { + let parser = parser_from_cx(sess, arg.clone()); + let mut tt_parser = TtParser::new(name); + + if let Success(_) = + tt_parser.parse_tt(&mut Cow::Borrowed(&parser), lhs, &mut NoopTracker) + { + if comma_span.is_dummy() { + err.note("you might be missing a comma"); + } else { + err.span_suggestion_short( + comma_span, + "missing comma here", + ", ", + Applicability::MachineApplicable, + ); + } + } + } + } + err.emit(); + cx.trace_macros_diag(); DummyResult::any(sp) } +/// The tracker used for the slow error path that collects useful info for diagnostics +struct CollectTrackerAndEmitter<'a, 'cx> { + cx: &'a mut ExtCtxt<'cx>, + /// Which arm's failure should we report? (the one furthest along) + best_failure: Option<(Token, &'static str)>, + root_span: Span, + result: Option>, +} + +impl<'a, 'cx, 'matcher> Tracker<'matcher> for CollectTrackerAndEmitter<'a, 'cx> { + fn before_match_loc(&mut self, _parser: &TtParser, _matcher: &'matcher MatcherLoc) { + // Empty for now. + } + + fn after_arm(&mut self, result: &NamedParseResult) { + match result { + Success(_) => { + unreachable!("should not collect detailed info for successful macro match"); + } + Failure(token, msg) => match self.best_failure { + Some((ref best_token, _)) if best_token.span.lo() >= token.span.lo() => {} + _ => self.best_failure = Some((token.clone(), msg)), + }, + Error(err_sp, msg) => { + let span = err_sp.substitute_dummy(self.root_span); + self.cx.struct_span_err(span, msg).emit(); + self.result = Some(DummyResult::any(span)); + } + ErrorReported(_) => self.result = Some(DummyResult::any(self.root_span)), + } + } + + fn description() -> &'static str { + "detailed" + } +} + +impl<'a, 'cx> CollectTrackerAndEmitter<'a, 'cx> { + fn new(cx: &'a mut ExtCtxt<'cx>, root_span: Span) -> Self { + Self { cx, best_failure: None, root_span, result: None } + } +} + +enum CanRetry { + Yes, + /// We are not allowed to retry macro expansion as a fatal error has been emitted already. + No(ErrorGuaranteed), +} + /// Try expanding the macro. Returns the index of the sucessful arm and its named_matches if it was successful, /// and nothing if it failed. On failure, it's the callers job to use `track` accordingly to record all errors /// correctly. @@ -335,7 +435,7 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( arg: &TokenStream, lhses: &'matcher [Vec], track: &mut T, -) -> Result<(usize, NamedMatches), ()> { +) -> Result<(usize, NamedMatches), CanRetry> { // We create a base parser that can be used for the "black box" parts. // Every iteration needs a fresh copy of that parser. However, the parser // is not mutated on many of the iterations, particularly when dealing with @@ -383,10 +483,10 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( } Error(_, _) => { // We haven't emitted an error yet - return Err(()); + return Err(CanRetry::Yes); } - ErrorReported(_) => { - return Err(()); + ErrorReported(guarantee) => { + return Err(CanRetry::No(guarantee)); } } @@ -395,7 +495,7 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( mem::swap(&mut gated_spans_snapshot, &mut sess.gated_spans.spans.borrow_mut()); } - Err(()) + Err(CanRetry::Yes) } // Note that macro-by-example's input is also matched against a token tree: @@ -478,7 +578,7 @@ pub fn compile_declarative_macro( let mut tt_parser = TtParser::new(Ident::with_dummy_span(if macro_rules { kw::MacroRules } else { kw::Macro })); let argument_map = - match tt_parser.parse_tt(&mut Cow::Borrowed(&parser), &argument_gram, &mut NoopTracker) { + match tt_parser.parse_tt(&mut Cow::Owned(parser), &argument_gram, &mut NoopTracker) { Success(m) => m, Failure(token, msg) => { let s = parse_failure_msg(&token); From 1e21b3cfa38c5040ae0faf99178b0112fc90fd93 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Thu, 27 Oct 2022 22:03:34 +0200 Subject: [PATCH 6/7] Add some debug logs to macro matching These were useful while debugging, so I'll leave them here. --- compiler/rustc_expand/src/mbe/macro_rules.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index b1214543e5db7..78b3fa337ae7b 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -234,6 +234,7 @@ impl<'matcher> Tracker<'matcher> for NoopTracker { /// Expands the rules based macro defined by `lhses` and `rhses` for a given /// input `arg`. +#[instrument(skip(cx, transparency, arg, lhses, rhses))] fn expand_macro<'cx>( cx: &'cx mut ExtCtxt<'_>, sp: Span, @@ -429,6 +430,7 @@ enum CanRetry { /// Try expanding the macro. Returns the index of the sucessful arm and its named_matches if it was successful, /// and nothing if it failed. On failure, it's the callers job to use `track` accordingly to record all errors /// correctly. +#[instrument(level = "debug", skip(sess, arg, lhses, track), fields(tracking = %T::description()))] fn try_match_macro<'matcher, T: Tracker<'matcher>>( sess: &ParseSess, name: Ident, @@ -460,6 +462,8 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( // Try each arm's matchers. let mut tt_parser = TtParser::new(name); for (i, lhs) in lhses.iter().enumerate() { + let _tracing_span = trace_span!("Matching arm", %i); + // Take a snapshot of the state of pre-expansion gating at this point. // This is used so that if a matcher is not `Success(..)`ful, // then the spans which became gated when parsing the unsuccessful matcher @@ -472,6 +476,7 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( match result { Success(named_matches) => { + debug!("Parsed arm successfully"); // The matcher was `Success(..)`ful. // Merge the gated spans from parsing the matcher with the pre-existing ones. sess.gated_spans.merge(gated_spans_snapshot); @@ -479,13 +484,16 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( return Ok((i, named_matches)); } Failure(_, _) => { + trace!("Failed to match arm, trying the next one"); // Try the next arm } Error(_, _) => { + debug!("Fatal error occurred during matching"); // We haven't emitted an error yet return Err(CanRetry::Yes); } ErrorReported(guarantee) => { + debug!("Fatal error occurred and was reported during matching"); return Err(CanRetry::No(guarantee)); } } From ebfa2ab68e806ce4eecb09525b82724a064c1de3 Mon Sep 17 00:00:00 2001 From: nils <48135649+Nilstrieb@users.noreply.github.com> Date: Fri, 4 Nov 2022 09:44:59 +0100 Subject: [PATCH 7/7] Small style improvements --- compiler/rustc_expand/src/mbe/macro_parser.rs | 4 ++-- compiler/rustc_expand/src/mbe/macro_rules.rs | 21 ++++++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index d2dbd190c8ec2..95cec8d7ae299 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -70,7 +70,6 @@ //! eof: [a $( a )* a b ·] //! ``` -use rustc_errors::ErrorGuaranteed; pub(crate) use NamedMatch::*; pub(crate) use ParseResult::*; @@ -79,6 +78,7 @@ use crate::mbe::{macro_rules::Tracker, KleeneOp, TokenTree}; use rustc_ast::token::{self, DocComment, Nonterminal, NonterminalKind, Token}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sync::Lrc; +use rustc_errors::ErrorGuaranteed; use rustc_lint_defs::pluralize; use rustc_parse::parser::{NtOrTt, Parser}; use rustc_span::symbol::Ident; @@ -96,7 +96,7 @@ use std::collections::hash_map::Entry::{Occupied, Vacant}; /// /// This means a matcher can be represented by `&[MatcherLoc]`, and traversal mostly involves /// simply incrementing the current matcher position index by one. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug)] pub(crate) enum MatcherLoc { Token { token: Token, diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index 78b3fa337ae7b..99af91072882e 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -210,18 +210,18 @@ fn trace_macros_note(cx_expansions: &mut FxIndexMap>, sp: Span } pub(super) trait Tracker<'matcher> { - /// This is called before trying to match next MatcherLoc on the current token + /// This is called before trying to match next MatcherLoc on the current token. fn before_match_loc(&mut self, parser: &TtParser, matcher: &'matcher MatcherLoc); /// This is called after an arm has been parsed, either successfully or unsuccessfully. When this is called, - /// `before_match_loc` was called at least once (with a `MatcherLoc::Eof`) + /// `before_match_loc` was called at least once (with a `MatcherLoc::Eof`). fn after_arm(&mut self, result: &NamedParseResult); - /// For tracing + /// For tracing. fn description() -> &'static str; } -/// A noop tracker that is used in the hot path of the expansion, has zero overhead thanks to monomorphization +/// A noop tracker that is used in the hot path of the expansion, has zero overhead thanks to monomorphization. struct NoopTracker; impl<'matcher> Tracker<'matcher> for NoopTracker { @@ -256,7 +256,7 @@ fn expand_macro<'cx>( trace_macros_note(&mut cx.expansions, sp, msg); } - // Track nothing for the best performance + // Track nothing for the best performance. let try_success_result = try_match_macro(sess, name, &arg, lhses, &mut NoopTracker); match try_success_result { @@ -326,7 +326,7 @@ fn expand_macro<'cx>( } } - // An error occured, try the expansion again, tracking the expansion closely for better diagnostics + // An error occurred, try the expansion again, tracking the expansion closely for better diagnostics. let mut tracker = CollectTrackerAndEmitter::new(cx, sp); let try_success_result = try_match_macro(sess, name, &arg, lhses, &mut tracker); @@ -378,7 +378,7 @@ fn expand_macro<'cx>( DummyResult::any(sp) } -/// The tracker used for the slow error path that collects useful info for diagnostics +/// The tracker used for the slow error path that collects useful info for diagnostics. struct CollectTrackerAndEmitter<'a, 'cx> { cx: &'a mut ExtCtxt<'cx>, /// Which arm's failure should we report? (the one furthest along) @@ -427,7 +427,7 @@ enum CanRetry { No(ErrorGuaranteed), } -/// Try expanding the macro. Returns the index of the sucessful arm and its named_matches if it was successful, +/// Try expanding the macro. Returns the index of the successful arm and its named_matches if it was successful, /// and nothing if it failed. On failure, it's the callers job to use `track` accordingly to record all errors /// correctly. #[instrument(level = "debug", skip(sess, arg, lhses, track), fields(tracking = %T::description()))] @@ -485,15 +485,16 @@ fn try_match_macro<'matcher, T: Tracker<'matcher>>( } Failure(_, _) => { trace!("Failed to match arm, trying the next one"); - // Try the next arm + // Try the next arm. } Error(_, _) => { debug!("Fatal error occurred during matching"); - // We haven't emitted an error yet + // We haven't emitted an error yet, so we can retry. return Err(CanRetry::Yes); } ErrorReported(guarantee) => { debug!("Fatal error occurred and was reported during matching"); + // An error has been reported already, we cannot retry as that would cause duplicate errors. return Err(CanRetry::No(guarantee)); } }