-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Make positional argument error in format! clearer #45807
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
Changes from 3 commits
82d5ea4
d5cb474
13a4162
b577b9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,8 @@ struct Context<'a, 'b: 'a> { | |
/// still existed in this phase of processing. | ||
/// Used only for `all_pieces_simple` tracking in `trans_piece`. | ||
curarg: usize, | ||
/// Keep track of invalid references to positional arguments | ||
invalid_refs: Vec<usize>, | ||
} | ||
|
||
/// Parses the arguments from the given list of tokens, returning None | ||
|
@@ -251,23 +253,49 @@ impl<'a, 'b> Context<'a, 'b> { | |
|
||
fn describe_num_args(&self) -> String { | ||
match self.args.len() { | ||
0 => "no arguments given".to_string(), | ||
1 => "there is 1 argument".to_string(), | ||
x => format!("there are {} arguments", x), | ||
0 => "no arguments were given".to_string(), | ||
1 => "there is only 1 argument".to_string(), | ||
x => format!("there are only {} arguments", x), | ||
} | ||
} | ||
|
||
/// Handle invalid references to positional arguments. Output different | ||
/// errors for the case where all arguments are positional and for when | ||
/// there are named arguments in the format string. | ||
fn report_invalid_references(&self) { | ||
let mut refs: Vec<String> = self.invalid_refs | ||
.iter() | ||
.map(|r| r.to_string()) | ||
.collect(); | ||
|
||
let msg = if self.names.is_empty() { | ||
format!("{} positional argument{} in format string, but {}", | ||
self.pieces.len(), | ||
if self.pieces.len() > 1 { "s" } else { "" }, | ||
self.describe_num_args()) | ||
} else { | ||
let arg_list = match refs.len() { | ||
1 => format!("argument {}", refs.pop().unwrap()), | ||
_ => format!("arguments {head} and {tail}", | ||
tail=refs.pop().unwrap(), | ||
head=refs.join(", ")) | ||
}; | ||
|
||
format!("invalid reference to positional {} ({})", | ||
arg_list, | ||
self.describe_num_args()) | ||
}; | ||
|
||
self.ecx.span_err(self.fmtsp, &msg[..]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add a |
||
} | ||
|
||
/// Actually verifies and tracks a given format placeholder | ||
/// (a.k.a. argument). | ||
fn verify_arg_type(&mut self, arg: Position, ty: ArgumentType) { | ||
match arg { | ||
Exact(arg) => { | ||
if self.args.len() <= arg { | ||
let msg = format!("invalid reference to argument `{}` ({})", | ||
arg, | ||
self.describe_num_args()); | ||
|
||
self.ecx.span_err(self.fmtsp, &msg[..]); | ||
self.invalid_refs.push(arg); | ||
return; | ||
} | ||
match ty { | ||
|
@@ -691,6 +719,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, | |
all_pieces_simple: true, | ||
macsp, | ||
fmtsp: fmt.span, | ||
invalid_refs: Vec::new(), | ||
}; | ||
|
||
let fmt_str = &*fmt.node.0.as_str(); | ||
|
@@ -736,6 +765,10 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, | |
cx.str_pieces.push(s); | ||
} | ||
|
||
if cx.invalid_refs.len() >= 1 { | ||
cx.report_invalid_references(); | ||
} | ||
|
||
// Make sure that all arguments were used and all arguments have types. | ||
let num_pos_args = cx.args.len() - cx.names.len(); | ||
let mut errs = vec![]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,36 +11,39 @@ | |
fn main() { | ||
// bad arguments to the format! call | ||
|
||
format!("{}"); //~ ERROR: invalid reference to argument | ||
// bad number of arguments, see #44954 (originally #15780) | ||
|
||
format!("{1}", 1); //~ ERROR: invalid reference to argument `1` | ||
//~^ ERROR: argument never used | ||
format!("{foo}"); //~ ERROR: no argument named `foo` | ||
format!("{}"); | ||
//~^ ERROR: 1 positional argument in format string, but no arguments were given | ||
|
||
format!("", 1, 2); //~ ERROR: multiple unused formatting arguments | ||
format!("{}", 1, 2); //~ ERROR: argument never used | ||
format!("{1}", 1, 2); //~ ERROR: argument never used | ||
format!("{}", 1, foo=2); //~ ERROR: named argument never used | ||
format!("{foo}", 1, foo=2); //~ ERROR: argument never used | ||
format!("", foo=2); //~ ERROR: named argument never used | ||
format!("{1}", 1); | ||
//~^ ERROR: 1 positional argument in format string, but there is only 1 argument | ||
//~^^ ERROR: argument never used | ||
|
||
format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument | ||
format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow | ||
|
||
// bad number of arguments, see #15780 | ||
|
||
format!("{0}"); | ||
//~^ ERROR invalid reference to argument `0` (no arguments given) | ||
format!("{} {}"); | ||
//~^ ERROR: 2 positional arguments in format string, but no arguments were given | ||
|
||
format!("{0} {1}", 1); | ||
//~^ ERROR invalid reference to argument `1` (there is 1 argument) | ||
//~^ ERROR: 2 positional arguments in format string, but there is only 1 argument | ||
|
||
format!("{0} {1} {2}", 1, 2); | ||
//~^ ERROR invalid reference to argument `2` (there are 2 arguments) | ||
|
||
format!("{0} {1}"); | ||
//~^ ERROR invalid reference to argument `0` (no arguments given) | ||
//~^^ ERROR invalid reference to argument `1` (no arguments given) | ||
//~^ ERROR: 3 positional arguments in format string, but there are only 2 arguments | ||
|
||
format!("{} {value} {} {}", 1, value=2); | ||
//~^ ERROR: invalid reference to positional argument 2 (there are only 2 arguments) | ||
format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a case with multiple named arguments that are not used, mixed with positional arguments? I'm curious about the new output. |
||
//~^ ERROR: invalid reference to positional arguments 3, 4 and 5 (there are only 3 arguments) | ||
|
||
format!("{foo}"); //~ ERROR: no argument named `foo` | ||
format!("", 1, 2); //~ ERROR: multiple unused formatting arguments | ||
format!("{}", 1, 2); //~ ERROR: argument never used | ||
format!("{1}", 1, 2); //~ ERROR: argument never used | ||
format!("{}", 1, foo=2); //~ ERROR: named argument never used | ||
format!("{foo}", 1, foo=2); //~ ERROR: argument never used | ||
format!("", foo=2); //~ ERROR: named argument never used | ||
|
||
format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument | ||
format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow | ||
|
||
// bad named arguments, #35082 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add another check for
self.args.iter().filter_map(|arg| /* Some(arg) if numbered positional argument */).count() == 0
here, so that if there are arguments like{1}
, it goes to the else branch and it is explicit about the arguments that are wrong.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the position of numbered and implicit placeholder is already resolved in the parser, should we change it so we still have that information here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you prefer, that's also a reasonable change.