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

Changes from 1 commit
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
Loading