Skip to content
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

Remove various has_errors or err_count uses #120342

Merged
merged 6 commits into from
Jan 30, 2024
Merged
Changes from 1 commit
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
39 changes: 24 additions & 15 deletions compiler/rustc_expand/src/mbe/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,9 @@ pub fn compile_declarative_macro(
)
.pop()
.unwrap();
valid &= check_lhs_nt_follows(sess, def, &tt);
// We don't handle errors here, the driver will abort
// after parsing/expansion. we can report every error in every macro this way.
valid &= check_lhs_nt_follows(sess, def, &tt).is_ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but updating valid here is pointless when it's immediately followed by return.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which means check_lhs_nt_follows doesn't need to return a bool.

It would be good to know how/why the driver will abort after parsing/expansion, bonus points if you understand that and can augment this comment appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this return is in a closure

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return tt;
}
sess.dcx().span_bug(def.span, "wrong-structured lhs")
Expand Down Expand Up @@ -589,18 +591,19 @@ pub fn compile_declarative_macro(
(mk_syn_ext(expander), rule_spans)
}

fn check_lhs_nt_follows(sess: &Session, def: &ast::Item, lhs: &mbe::TokenTree) -> bool {
fn check_lhs_nt_follows(
sess: &Session,
def: &ast::Item,
lhs: &mbe::TokenTree,
) -> Result<(), ErrorGuaranteed> {
// lhs is going to be like TokenTree::Delimited(...), where the
// entire lhs is those tts. Or, it can be a "bare sequence", not wrapped in parens.
if let mbe::TokenTree::Delimited(.., delimited) = lhs {
check_matcher(sess, def, &delimited.tts)
} else {
let msg = "invalid macro matcher; matchers must be contained in balanced delimiters";
sess.dcx().span_err(lhs.span(), msg);
false
Err(sess.dcx().span_err(lhs.span(), msg))
}
// we don't abort on errors on rejection, the driver will do that for us
// after parsing/expansion. we can report every error in every macro this way.
}

fn is_empty_token_tree(sess: &Session, seq: &mbe::SequenceRepetition) -> bool {
Expand Down Expand Up @@ -675,12 +678,15 @@ fn check_rhs(sess: &Session, rhs: &mbe::TokenTree) -> bool {
false
}

fn check_matcher(sess: &Session, def: &ast::Item, matcher: &[mbe::TokenTree]) -> bool {
fn check_matcher(
sess: &Session,
def: &ast::Item,
matcher: &[mbe::TokenTree],
) -> Result<(), ErrorGuaranteed> {
let first_sets = FirstSets::new(matcher);
let empty_suffix = TokenSet::empty();
let err = sess.dcx().err_count();
check_matcher_core(sess, def, &first_sets, matcher, &empty_suffix);
err == sess.dcx().err_count()
check_matcher_core(sess, def, &first_sets, matcher, &empty_suffix)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the trailing ?; and the Ok(()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function returns Result<()> while the _core function returns an ok value

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Ok(())
}

fn has_compile_error_macro(rhs: &mbe::TokenTree) -> bool {
Expand Down Expand Up @@ -1020,11 +1026,13 @@ fn check_matcher_core<'tt>(
first_sets: &FirstSets<'tt>,
matcher: &'tt [mbe::TokenTree],
follow: &TokenSet<'tt>,
) -> TokenSet<'tt> {
) -> Result<TokenSet<'tt>, ErrorGuaranteed> {
use mbe::TokenTree;

let mut last = TokenSet::empty();

let mut errored = Ok(());

// 2. For each token and suffix [T, SUFFIX] in M:
// ensure that T can be followed by SUFFIX, and if SUFFIX may be empty,
// then ensure T can also be followed by any element of FOLLOW.
Expand Down Expand Up @@ -1068,7 +1076,7 @@ fn check_matcher_core<'tt>(
token::CloseDelim(d.delim),
span.close,
));
check_matcher_core(sess, def, first_sets, &d.tts, &my_suffix);
check_matcher_core(sess, def, first_sets, &d.tts, &my_suffix)?;
// don't track non NT tokens
last.replace_with_irrelevant();

Expand Down Expand Up @@ -1100,7 +1108,7 @@ fn check_matcher_core<'tt>(
// At this point, `suffix_first` is built, and
// `my_suffix` is some TokenSet that we can use
// for checking the interior of `seq_rep`.
let next = check_matcher_core(sess, def, first_sets, &seq_rep.tts, my_suffix);
let next = check_matcher_core(sess, def, first_sets, &seq_rep.tts, my_suffix)?;
if next.maybe_empty {
last.add_all(&next);
} else {
Expand Down Expand Up @@ -1206,14 +1214,15 @@ fn check_matcher_core<'tt>(
));
}
}
err.emit();
errored = Err(err.emit());
}
}
}
}
}
}
last
errored?;
Ok(last)
}

fn token_can_be_followed_by_any(tok: &mbe::TokenTree) -> bool {
Expand Down