Skip to content

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.

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

Merged
merged 4 commits into from
Nov 11, 2017
Merged

Make positional argument error in format! clearer #45807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82d5ea4
Make format! positional argument errors clear
tommyip Nov 6, 2017
d5cb474
Add tests for new format! error message
tommyip Nov 6, 2017
13a4162
Update tests for new format! error message
tommyip Nov 6, 2017
b577b9a
Retain information on whether a format argument has explicit position
tommyip Nov 9, 2017
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
22 changes: 12 additions & 10 deletions src/libfmt_macros/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ pub struct FormatSpec<'a> {
/// Enum describing where an argument for a format can be located.
#[derive(Copy, Clone, PartialEq)]
pub enum Position<'a> {
/// The argument is located at a specific index.
/// The arugment is implied to be located at an index
ArgumentImplicitlyIs(usize),
/// The argument is located at a specific index given in the format
ArgumentIs(usize),
/// The argument has a name.
ArgumentNamed(&'a str),
Expand Down Expand Up @@ -275,7 +277,7 @@ impl<'a> Parser<'a> {
None => {
let i = self.curarg;
self.curarg += 1;
ArgumentIs(i)
ArgumentImplicitlyIs(i)
}
};

Expand Down Expand Up @@ -517,7 +519,7 @@ mod tests {
fn format_nothing() {
same("{}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: fmtdflt(),
})]);
}
Expand Down Expand Up @@ -595,7 +597,7 @@ mod tests {
fn format_counts() {
same("{:10s}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -607,7 +609,7 @@ mod tests {
})]);
same("{:10$.10s}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -619,7 +621,7 @@ mod tests {
})]);
same("{:.*s}",
&[NextArgument(Argument {
position: ArgumentIs(1),
position: ArgumentImplicitlyIs(1),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -631,7 +633,7 @@ mod tests {
})]);
same("{:.10$s}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -643,7 +645,7 @@ mod tests {
})]);
same("{:a$.b$s}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -658,7 +660,7 @@ mod tests {
fn format_flags() {
same("{:-}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -670,7 +672,7 @@ mod tests {
})]);
same("{:+#}",
&[NextArgument(Argument {
position: ArgumentIs(0),
position: ArgumentImplicitlyIs(0),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/on_unimplemented.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
}
},
// `{:1}` and `{}` are not to be used
Position::ArgumentIs(_) => {
Position::ArgumentIs(_) | Position::ArgumentImplicitlyIs(_) => {
span_err!(tcx.sess, span, E0231,
"only named substitution \
parameters are allowed");
Expand Down
67 changes: 59 additions & 8 deletions src/libsyntax_ext/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -226,7 +228,7 @@ impl<'a, 'b> Context<'a, 'b> {
// argument second, if it's an implicit positional parameter
// it's written second, so it should come after width/precision.
let pos = match arg.position {
parse::ArgumentIs(i) => Exact(i),
parse::ArgumentIs(i) | parse::ArgumentImplicitlyIs(i) => Exact(i),
parse::ArgumentNamed(s) => Named(s.to_string()),
};

Expand All @@ -251,23 +253,54 @@ impl<'a, 'b> Context<'a, 'b> {

fn describe_num_args(&self) -> String {
match self.args.len() {
0 => "no arguments given".to_string(),
0 => "no arguments were given".to_string(),
1 => "there is 1 argument".to_string(),
x => format!("there are {} 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 or numbered positional arguments in the
/// format string.
fn report_invalid_references(&self, numbered_position_args: bool) {
let mut e;
let mut refs: Vec<String> = self.invalid_refs
.iter()
.map(|r| r.to_string())
.collect();

if self.names.is_empty() && !numbered_position_args {
e = self.ecx.mut_span_err(self.fmtsp,
&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(", "))
};

e = self.ecx.mut_span_err(self.fmtsp,
&format!("invalid reference to positional {} ({})",
arg_list,
self.describe_num_args()));
e.note("positional arguments are zero-based");
};

e.emit();
}

/// 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 {
Expand Down Expand Up @@ -403,7 +436,8 @@ impl<'a, 'b> Context<'a, 'b> {
}
};
match arg.position {
parse::ArgumentIs(i) => {
parse::ArgumentIs(i)
| parse::ArgumentImplicitlyIs(i) => {
// Map to index in final generated argument array
// in case of multiple types specified
let arg_idx = match arg_index_consumed.get_mut(i) {
Expand Down Expand Up @@ -691,6 +725,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();
Expand All @@ -711,6 +746,18 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
}
}

let numbered_position_args = pieces.iter().any(|arg: &parse::Piece| {
match *arg {
parse::String(_) => false,
parse::NextArgument(arg) => {
match arg.position {
parse::Position::ArgumentIs(_) => true,
_ => false,
}
}
}
});

cx.build_index_map();

let mut arg_index_consumed = vec![0usize; cx.arg_index_map.len()];
Expand All @@ -736,6 +783,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(numbered_position_args);
}

// 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![];
Expand Down
54 changes: 31 additions & 23 deletions src/test/compile-fail/ifmt-bad-arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,44 @@
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: invalid reference to positional argument 1 (there is 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: invalid reference to positional argument 1 (there is 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: invalid reference to positional argument 2 (there are 2 arguments)

format!("{} {value} {} {}", 1, value=2);
//~^ ERROR: invalid reference to positional argument 2 (there are 2 arguments)
format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2);
//~^ ERROR: invalid reference to positional arguments 3, 4 and 5 (there are 3 arguments)

format!("{} {foo} {} {bar} {}", 1, 2, 3);
//~^ ERROR: there is no argument named `foo`
//~^^ ERROR: there is no argument named `bar`

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!("{} {}", 1, 2, foo=1, bar=2); //~ ERROR: multiple unused formatting arguments

format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument
format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow

// bad named arguments, #35082

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/cross-crate-macro-backtrace/main.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: invalid reference to argument `0` (no arguments given)
error: 1 positional argument in format string, but no arguments were given
--> $DIR/main.rs:16:5
|
16 | myprintln!("{}"); //~ ERROR in this macro
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/macros/macro-backtrace-println.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: invalid reference to argument `0` (no arguments given)
error: 1 positional argument in format string, but no arguments were given
--> $DIR/macro-backtrace-println.rs:24:30
|
24 | ($fmt:expr) => (myprint!(concat!($fmt, "/n")));
Expand Down