Skip to content

Properly calculate best failure in macro matching #105570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 30 additions & 10 deletions compiler/rustc_expand/src/mbe/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub(super) fn failed_to_match_macro<'cx>(
return result;
}

let Some((token, label, remaining_matcher)) = tracker.best_failure else {
let Some(BestFailure { token, msg: label, remaining_matcher, .. }) = tracker.best_failure else {
return DummyResult::any(sp);
};

Expand Down Expand Up @@ -95,11 +95,24 @@ struct CollectTrackerAndEmitter<'a, 'cx, 'matcher> {
cx: &'a mut ExtCtxt<'cx>,
remaining_matcher: Option<&'matcher MatcherLoc>,
/// Which arm's failure should we report? (the one furthest along)
best_failure: Option<(Token, &'static str, MatcherLoc)>,
best_failure: Option<BestFailure>,
root_span: Span,
result: Option<Box<dyn MacResult + 'cx>>,
}

struct BestFailure {
token: Token,
position_in_tokenstream: usize,
msg: &'static str,
remaining_matcher: MatcherLoc,
}

impl BestFailure {
fn is_better_position(&self, position: usize) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The polarity of this fn is a bit confusing -- it reads as it's asking the question "is self better than position", but it's actually "is position better than self"?

position > self.position_in_tokenstream
}
}

impl<'a, 'cx, 'matcher> Tracker<'matcher> for CollectTrackerAndEmitter<'a, 'cx, 'matcher> {
fn before_match_loc(&mut self, parser: &TtParser, matcher: &'matcher MatcherLoc) {
if self.remaining_matcher.is_none()
Expand All @@ -119,18 +132,25 @@ impl<'a, 'cx, 'matcher> Tracker<'matcher> for CollectTrackerAndEmitter<'a, 'cx,
"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(),
Failure(token, approx_position, msg) => {
debug!(?token, ?msg, "a new failure of an arm");

if self
.best_failure
.as_ref()
.map_or(true, |failure| failure.is_better_position(*approx_position))
{
self.best_failure = Some(BestFailure {
token: token.clone(),
position_in_tokenstream: *approx_position,
msg,
self.remaining_matcher
remaining_matcher: self
.remaining_matcher
.expect("must have collected matcher already")
.clone(),
))
})
}
},
}
Error(err_sp, msg) => {
let span = err_sp.substitute_dummy(self.root_span);
self.cx.struct_span_err(span, msg).emit();
Expand Down
13 changes: 11 additions & 2 deletions compiler/rustc_expand/src/mbe/macro_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,8 @@ pub(crate) enum ParseResult<T> {
Success(T),
/// Arm failed to match. If the second parameter is `token::Eof`, it indicates an unexpected
/// end of macro invocation. Otherwise, it indicates that no rules expected the given token.
Failure(Token, &'static str),
/// The usize is the approximate position of the token in the input token stream.
Failure(Token, usize, &'static str),
/// Fatal error (malformed macro?). Abort compilation.
Error(rustc_span::Span, String),
ErrorReported(ErrorGuaranteed),
Expand Down Expand Up @@ -455,6 +456,7 @@ impl TtParser {
&mut self,
matcher: &'matcher [MatcherLoc],
token: &Token,
approx_position: usize,
track: &mut T,
) -> Option<NamedParseResult> {
// Matcher positions that would be valid if the macro invocation was over now. Only
Expand Down Expand Up @@ -598,6 +600,7 @@ impl TtParser {
token::Eof,
if token.span.is_dummy() { token.span } else { token.span.shrink_to_hi() },
),
approx_position,
"missing tokens in macro arguments",
),
})
Expand Down Expand Up @@ -627,7 +630,12 @@ 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.
let res = self.parse_tt_inner(matcher, &parser.token, track);
let res = self.parse_tt_inner(
matcher,
&parser.token,
parser.approx_token_stream_pos(),
track,
);
if let Some(res) = res {
return res;
}
Expand All @@ -642,6 +650,7 @@ impl TtParser {
// parser: syntax error.
return Failure(
parser.token.clone(),
parser.approx_token_stream_pos(),
"no rules expected this token in macro call",
);
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_expand/src/mbe/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,8 @@ pub(super) fn try_match_macro<'matcher, T: Tracker<'matcher>>(

return Ok((i, named_matches));
}
Failure(_, _) => {
trace!("Failed to match arm, trying the next one");
Failure(_, reached_position, _) => {
trace!(%reached_position, "Failed to match arm, trying the next one");
// Try the next arm.
}
Error(_, _) => {
Expand Down Expand Up @@ -432,7 +432,7 @@ pub fn compile_declarative_macro(
let argument_map =
match tt_parser.parse_tt(&mut Cow::Owned(parser), &argument_gram, &mut NoopTracker) {
Success(m) => m,
Failure(token, msg) => {
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);
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,10 @@ impl<'a> Parser<'a> {
pub fn clear_expected_tokens(&mut self) {
self.expected_tokens.clear();
}

pub fn approx_token_stream_pos(&self) -> usize {
self.token_cursor.num_next_calls
}
}

pub(crate) fn make_unclosed_delims_error(
Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/macros/best-failure.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
macro_rules! number {
(neg false, $self:ident) => { $self };
($signed:tt => $ty:ty;) => {
number!(neg $signed, $self);
//~^ ERROR no rules expected the token `$`
};
}

number! { false => u8; }

fn main() {}
21 changes: 21 additions & 0 deletions src/test/ui/macros/best-failure.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error: no rules expected the token `$`
--> $DIR/best-failure.rs:4:30
|
LL | macro_rules! number {
| ------------------- when calling this macro
...
LL | number!(neg $signed, $self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of a shame that we highlight the whole $self metavariable rather than just the $ token given the primary message of this diagnostic, but w/e

| ^^^^^ no rules expected this token in macro call
...
LL | number! { false => u8; }
| ------------------------ in this macro invocation
|
note: while trying to match meta-variable `$self:ident`
--> $DIR/best-failure.rs:2:17
|
LL | (neg false, $self:ident) => { $self };
| ^^^^^^^^^^^
= note: this error originates in the macro `number` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error