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.

Sign up for GitHub

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

Jump to bottom

Remove various has_errors or err_count uses #120342

Merged
merged 6 commits into from
Jan 30, 2024
Merged

Remove various has_errors or err_count uses #120342

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f68741b
Stop checking `err_count` in macro_rules validity checking
oli-obk Jan 25, 2024
f6f0e04
Remove an unused error count check
oli-obk Jan 25, 2024
646c8fc
Statically ensure an error is reported in report_arg_errors
oli-obk Jan 25, 2024
2b60e56
Statically ensure report_selection_error actually reports an error
oli-obk Jan 25, 2024
3042da0
Remove has_errors check in builtin macro parsing
oli-obk Jan 25, 2024
054e1e3
Track ErrorGuaranteed instead of conjuring it from thin air
oli-obk Jan 25, 2024
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
12 changes: 8 additions & 4 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,15 @@ fn parse_args<'a>(ecx: &mut ExtCtxt<'a>, sp: Span, tts: TokenStream) -> PResult<
_ => {
let expr = p.parse_expr()?;
if !args.named_args().is_empty() {
ecx.dcx().emit_err(errors::PositionalAfterNamed {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unconnected with the second half of this commit. Is it necessary?

Also, if necessary, it seems incomplete, because there are two other emit calls in this function. Seems like they should be converted to return created errors as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The others return. this one doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

I see one err.emit() and two emit_err() calls in parse_args. Prior to this change, none of them returned. After this change, one of them returns an Err without emitting, and the other two are unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh hmm. Need to rethink this one

Copy link
Contributor Author

@oli-obk oli-obk Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The reason I did this is so that expand_format_args_impl (which is the only parse_args caller), doesn't cause follow-up warnings like

warning: named argument `foo` is not used by name
  --> $DIR/format-parse-errors.rs:9:9
   |
LL |         "s {foo} {} {}",
   |                  -- this formatting argument uses named argument `foo` by position
LL |         foo = foo,
   |         ^^^ this named argument is referred to by position in formatting string
   |
   = note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
   |
LL |         "s {foo} {foo} {}",
   |                   +++

which are a bit nonsensical considering we already got a

error: positional arguments cannot follow named arguments
  --> $DIR/format-parse-errors.rs:10:9
   |
LL |         foo = foo,
   |         --------- named argument
LL |         bar,
   |         ^^^ positional arguments must be before named arguments

and the fact that the diagnostic is likely always wrong. So I opted to just make the entire macro expansion go to a DummyResult::any if there were errors during it. This is already done for other errors, so...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm now not clear if you're intending to do more work here ("need to rethink this one") or if you are waiting for a response from me. Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no changes needed. this early return ensures that the macro expansion is poisoned instead of expanding to nonsense that may cause follow up nonsensical diagnostics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically I think we should merge it as is, but I can also remove the entire commit and file it as a separate PR if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll accept that explanation, thanks.

return Err(ecx.dcx().create_err(errors::PositionalAfterNamed {
span: expr.span,
args: args
.named_args()
.iter()
.filter_map(|a| a.kind.ident().map(|ident| (a, ident)))
.map(|(arg, n)| n.span.to(arg.expr.span))
.collect(),
});
}));
}
args.add(FormatArgument { kind: FormatArgumentKind::Normal, expr });
}
Expand Down Expand Up @@ -313,6 +313,8 @@ fn make_format_args(
}
use ArgRef::*;

let mut unnamed_arg_after_named_arg = false;

let mut lookup_arg = |arg: ArgRef<'_>,
span: Option<Span>,
used_as: PositionUsedAs,
Expand Down Expand Up @@ -352,6 +354,7 @@ fn make_format_args(
// For the moment capturing variables from format strings expanded from macros is
// disabled (see RFC #2795)
ecx.dcx().emit_err(errors::FormatNoArgNamed { span, name });
unnamed_arg_after_named_arg = true;
DummyResult::raw_expr(span, true)
};
Ok(args.add(FormatArgument { kind: FormatArgumentKind::Captured(ident), expr }))
Expand Down Expand Up @@ -510,7 +513,8 @@ fn make_format_args(
})
.collect::<Vec<_>>();

if !unused.is_empty() {
let has_unused = !unused.is_empty();
if has_unused {
// If there's a lot of unused arguments,
// let's check if this format arguments looks like another syntax (printf / shell).
let detect_foreign_fmt = unused.len() > args.explicit_args().len() / 2;
Expand All @@ -529,7 +533,7 @@ fn make_format_args(

// Only check for unused named argument names if there are no other errors to avoid causing
// too much noise in output errors, such as when a named argument is entirely unused.
if invalid_refs.is_empty() && ecx.dcx().has_errors().is_none() {
if invalid_refs.is_empty() && !has_unused && !unnamed_arg_after_named_arg {
for &(index, span, used_as) in &numeric_refences_to_named_arg {
let (position_sp_to_replace, position_sp_for_msg) = match used_as {
Placeholder(pspan) => (span, pspan),
Expand Down
39 changes: 24 additions & 15 deletions compiler/rustc_expand/src/mbe/macro_rules.rs
View file
Open in desktop
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
30 changes: 13 additions & 17 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.map(|vars| self.resolve_vars_if_possible(vars)),
);

self.report_arg_errors(
self.set_tainted_by_errors(self.report_arg_errors(
compatibility_diagonal,
formal_and_expected_inputs,
provided_args,
Expand All @@ -433,7 +433,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn_def_id,
call_span,
call_expr,
);
));
}
}

Expand All @@ -447,7 +447,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn_def_id: Option<DefId>,
call_span: Span,
call_expr: &'tcx hir::Expr<'tcx>,
) {
) -> ErrorGuaranteed {
// Next, let's construct the error
let (error_span, full_call_span, call_name, is_method) = match &call_expr.kind {
hir::ExprKind::Call(
Expand Down Expand Up @@ -486,10 +486,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

let tcx = self.tcx;
// FIXME: taint after emitting errors and pass through an `ErrorGuaranteed`
self.set_tainted_by_errors(
tcx.dcx().span_delayed_bug(call_span, "no errors reported for args"),
);

// Get the argument span in the context of the call span so that
// suggestions and labels are (more) correct when an arg is a
Expand Down Expand Up @@ -696,8 +692,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(mismatch_idx),
is_method,
);
err.emit();
return;
return err.emit();
}
}
}
Expand All @@ -721,11 +716,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if cfg!(debug_assertions) {
span_bug!(error_span, "expected errors from argument matrix");
} else {
tcx.dcx().emit_err(errors::ArgMismatchIndeterminate { span: error_span });
return tcx.dcx().emit_err(errors::ArgMismatchIndeterminate { span: error_span });
}
return;
}

let mut reported = None;
errors.retain(|error| {
let Error::Invalid(provided_idx, expected_idx, Compatibility::Incompatible(Some(e))) =
error
Expand All @@ -736,16 +731,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let trace =
mk_trace(provided_span, formal_and_expected_inputs[*expected_idx], provided_ty);
if !matches!(trace.cause.as_failure_code(*e), FailureCode::Error0308) {
self.err_ctxt().report_and_explain_type_error(trace, *e).emit();
reported = Some(self.err_ctxt().report_and_explain_type_error(trace, *e).emit());
return false;
}
true
});

// We're done if we found errors, but we already emitted them.
if errors.is_empty() {
return;
if let Some(reported) = reported {
assert!(errors.is_empty());
return reported;
}
assert!(!errors.is_empty());

// Okay, now that we've emitted the special errors separately, we
// are only left missing/extra/swapped and mismatched arguments, both
Expand Down Expand Up @@ -802,8 +799,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(expected_idx.as_usize()),
is_method,
);
err.emit();
return;
return err.emit();
}

let mut err = if formal_and_expected_inputs.len() == provided_args.len() {
Expand Down Expand Up @@ -1251,7 +1247,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
}

err.emit();
err.emit()
}

fn suggest_ptr_null_mut(
Expand Down
13 changes: 0 additions & 13 deletions compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,6 @@ pub struct FnCtxt<'a, 'tcx> {
/// eventually).
pub(super) param_env: ty::ParamEnv<'tcx>,

/// Number of errors that had been reported when we started
/// checking this function. On exit, if we find that *more* errors
/// have been reported, we will skip regionck and other work that
/// expects the types within the function to be consistent.
// FIXME(matthewjasper) This should not exist, and it's not correct
// if type checking is run in parallel.
err_count_on_creation: usize,

/// If `Some`, this stores coercion information for returned
/// expressions. If `None`, this is in a context where return is
/// inappropriate, such as a const expression.
Expand Down Expand Up @@ -126,7 +118,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
FnCtxt {
body_id,
param_env,
err_count_on_creation: inh.tcx.dcx().err_count(),
ret_coercion: None,
ret_coercion_span: Cell::new(None),
coroutine_types: None,
Expand Down Expand Up @@ -195,10 +186,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}),
}
}

pub fn errors_reported_since_creation(&self) -> bool {
self.dcx().err_count() > self.err_count_on_creation
}
}

impl<'a, 'tcx> Deref for FnCtxt<'a, 'tcx> {
Expand Down
Loading
Loading