Skip to content

Commit bd7ce71

Browse files
borsbors
committed
Auto merge of #45807 - tommyip:format_err, r=estebank
Make positional argument error in format! clearer r? @estebank Fixes #44954
2 parents c703ff2 + b577b9a commit bd7ce71

File tree

6 files changed

+105
-44
lines changed

6 files changed

+105
-44
lines changed

‎src/libfmt_macros/lib.rs‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ pub struct FormatSpec<'a> {
7373
/// Enum describing where an argument for a format can be located.
7474
#[derive(Copy, Clone, PartialEq)]
7575
pub enum Position<'a> {
76-
/// The argument is located at a specific index.
76+
/// The arugment is implied to be located at an index
77+
ArgumentImplicitlyIs(usize),
78+
/// The argument is located at a specific index given in the format
7779
ArgumentIs(usize),
7880
/// The argument has a name.
7981
ArgumentNamed(&'a str),
@@ -275,7 +277,7 @@ impl<'a> Parser<'a> {
275277
None => {
276278
let i = self.curarg;
277279
self.curarg += 1;
278-
ArgumentIs(i)
280+
ArgumentImplicitlyIs(i)
279281
}
280282
};
281283

@@ -517,7 +519,7 @@ mod tests {
517519
fn format_nothing() {
518520
same("{}",
519521
&[NextArgument(Argument {
520-
position: ArgumentIs(0),
522+
position: ArgumentImplicitlyIs(0),
521523
format: fmtdflt(),
522524
})]);
523525
}
@@ -595,7 +597,7 @@ mod tests {
595597
fn format_counts() {
596598
same("{:10s}",
597599
&[NextArgument(Argument {
598-
position: ArgumentIs(0),
600+
position: ArgumentImplicitlyIs(0),
599601
format: FormatSpec {
600602
fill: None,
601603
align: AlignUnknown,
@@ -607,7 +609,7 @@ mod tests {
607609
})]);
608610
same("{:10$.10s}",
609611
&[NextArgument(Argument {
610-
position: ArgumentIs(0),
612+
position: ArgumentImplicitlyIs(0),
611613
format: FormatSpec {
612614
fill: None,
613615
align: AlignUnknown,
@@ -619,7 +621,7 @@ mod tests {
619621
})]);
620622
same("{:.*s}",
621623
&[NextArgument(Argument {
622-
position: ArgumentIs(1),
624+
position: ArgumentImplicitlyIs(1),
623625
format: FormatSpec {
624626
fill: None,
625627
align: AlignUnknown,
@@ -631,7 +633,7 @@ mod tests {
631633
})]);
632634
same("{:.10$s}",
633635
&[NextArgument(Argument {
634-
position: ArgumentIs(0),
636+
position: ArgumentImplicitlyIs(0),
635637
format: FormatSpec {
636638
fill: None,
637639
align: AlignUnknown,
@@ -643,7 +645,7 @@ mod tests {
643645
})]);
644646
same("{:a$.b$s}",
645647
&[NextArgument(Argument {
646-
position: ArgumentIs(0),
648+
position: ArgumentImplicitlyIs(0),
647649
format: FormatSpec {
648650
fill: None,
649651
align: AlignUnknown,
@@ -658,7 +660,7 @@ mod tests {
658660
fn format_flags() {
659661
same("{:-}",
660662
&[NextArgument(Argument {
661-
position: ArgumentIs(0),
663+
position: ArgumentImplicitlyIs(0),
662664
format: FormatSpec {
663665
fill: None,
664666
align: AlignUnknown,
@@ -670,7 +672,7 @@ mod tests {
670672
})]);
671673
same("{:+#}",
672674
&[NextArgument(Argument {
673-
position: ArgumentIs(0),
675+
position: ArgumentImplicitlyIs(0),
674676
format: FormatSpec {
675677
fill: None,
676678
align: AlignUnknown,

‎src/librustc/traits/on_unimplemented.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
254254
}
255255
},
256256
// `{:1}` and `{}` are not to be used
257-
Position::ArgumentIs(_) => {
257+
Position::ArgumentIs(_) | Position::ArgumentImplicitlyIs(_) => {
258258
span_err!(tcx.sess, span, E0231,
259259
"only named substitution \
260260
parameters are allowed");

‎src/libsyntax_ext/format.rs‎

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,8 @@ struct Context<'a, 'b: 'a> {
110110
/// still existed in this phase of processing.
111111
/// Used only for `all_pieces_simple` tracking in `trans_piece`.
112112
curarg: usize,
113+
/// Keep track of invalid references to positional arguments
114+
invalid_refs: Vec<usize>,
113115
}
114116

115117
/// Parses the arguments from the given list of tokens, returning None
@@ -226,7 +228,7 @@ impl<'a, 'b> Context<'a, 'b> {
226228
// argument second, if it's an implicit positional parameter
227229
// it's written second, so it should come after width/precision.
228230
let pos = match arg.position {
229-
parse::ArgumentIs(i) => Exact(i),
231+
parse::ArgumentIs(i) | parse::ArgumentImplicitlyIs(i) => Exact(i),
230232
parse::ArgumentNamed(s) => Named(s.to_string()),
231233
};
232234

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

252254
fn describe_num_args(&self) -> String {
253255
match self.args.len() {
254-
0 => "no arguments given".to_string(),
256+
0 => "no arguments were given".to_string(),
255257
1 => "there is 1 argument".to_string(),
256258
x => format!("there are {} arguments", x),
257259
}
258260
}
259261

262+
/// Handle invalid references to positional arguments. Output different
263+
/// errors for the case where all arguments are positional and for when
264+
/// there are named arguments or numbered positional arguments in the
265+
/// format string.
266+
fn report_invalid_references(&self, numbered_position_args: bool) {
267+
let mut e;
268+
let mut refs: Vec<String> = self.invalid_refs
269+
.iter()
270+
.map(|r| r.to_string())
271+
.collect();
272+
273+
if self.names.is_empty() && !numbered_position_args {
274+
e = self.ecx.mut_span_err(self.fmtsp,
275+
&format!("{} positional argument{} in format string, but {}",
276+
self.pieces.len(),
277+
if self.pieces.len() > 1 { "s" } else { "" },
278+
self.describe_num_args()));
279+
} else {
280+
let arg_list = match refs.len() {
281+
1 => format!("argument {}", refs.pop().unwrap()),
282+
_ => format!("arguments {head} and {tail}",
283+
tail=refs.pop().unwrap(),
284+
head=refs.join(", "))
285+
};
286+
287+
e = self.ecx.mut_span_err(self.fmtsp,
288+
&format!("invalid reference to positional {} ({})",
289+
arg_list,
290+
self.describe_num_args()));
291+
e.note("positional arguments are zero-based");
292+
};
293+
294+
e.emit();
295+
}
296+
260297
/// Actually verifies and tracks a given format placeholder
261298
/// (a.k.a. argument).
262299
fn verify_arg_type(&mut self, arg: Position, ty: ArgumentType) {
263300
match arg {
264301
Exact(arg) => {
265302
if self.args.len() <= arg {
266-
let msg = format!("invalid reference to argument `{}` ({})",
267-
arg,
268-
self.describe_num_args());
269-
270-
self.ecx.span_err(self.fmtsp, &msg[..]);
303+
self.invalid_refs.push(arg);
271304
return;
272305
}
273306
match ty {
@@ -403,7 +436,8 @@ impl<'a, 'b> Context<'a, 'b> {
403436
}
404437
};
405438
match arg.position {
406-
parse::ArgumentIs(i) => {
439+
parse::ArgumentIs(i)
440+
| parse::ArgumentImplicitlyIs(i) => {
407441
// Map to index in final generated argument array
408442
// in case of multiple types specified
409443
let arg_idx = match arg_index_consumed.get_mut(i) {
@@ -691,6 +725,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
691725
all_pieces_simple: true,
692726
macsp,
693727
fmtsp: fmt.span,
728+
invalid_refs: Vec::new(),
694729
};
695730

696731
let fmt_str = &*fmt.node.0.as_str();
@@ -711,6 +746,18 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
711746
}
712747
}
713748

749+
let numbered_position_args = pieces.iter().any(|arg: &parse::Piece| {
750+
match *arg {
751+
parse::String(_) => false,
752+
parse::NextArgument(arg) => {
753+
match arg.position {
754+
parse::Position::ArgumentIs(_) => true,
755+
_ => false,
756+
}
757+
}
758+
}
759+
});
760+
714761
cx.build_index_map();
715762

716763
let mut arg_index_consumed = vec![0usize; cx.arg_index_map.len()];
@@ -736,6 +783,10 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
736783
cx.str_pieces.push(s);
737784
}
738785

786+
if cx.invalid_refs.len() >= 1 {
787+
cx.report_invalid_references(numbered_position_args);
788+
}
789+
739790
// Make sure that all arguments were used and all arguments have types.
740791
let num_pos_args = cx.args.len() - cx.names.len();
741792
let mut errs = vec![];

‎src/test/compile-fail/ifmt-bad-arg.rs‎

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -11,36 +11,44 @@
1111
fn main() {
1212
// bad arguments to the format! call
1313

14-
format!("{}"); //~ ERROR: invalid reference to argument
14+
// bad number of arguments, see #44954 (originally #15780)
1515

16-
format!("{1}", 1); //~ ERROR: invalid reference to argument `1`
17-
//~^ ERROR: argument never used
18-
format!("{foo}"); //~ ERROR: no argument named `foo`
16+
format!("{}");
17+
//~^ ERROR: 1 positional argument in format string, but no arguments were given
1918

20-
format!("", 1, 2); //~ ERROR: multiple unused formatting arguments
21-
format!("{}", 1, 2); //~ ERROR: argument never used
22-
format!("{1}", 1, 2); //~ ERROR: argument never used
23-
format!("{}", 1, foo=2); //~ ERROR: named argument never used
24-
format!("{foo}", 1, foo=2); //~ ERROR: argument never used
25-
format!("", foo=2); //~ ERROR: named argument never used
19+
format!("{1}", 1);
20+
//~^ ERROR: invalid reference to positional argument 1 (there is 1 argument)
21+
//~^^ ERROR: argument never used
2622

27-
format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument
28-
format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow
29-
30-
// bad number of arguments, see #15780
31-
32-
format!("{0}");
33-
//~^ ERROR invalid reference to argument `0` (no arguments given)
23+
format!("{} {}");
24+
//~^ ERROR: 2 positional arguments in format string, but no arguments were given
3425

3526
format!("{0} {1}", 1);
36-
//~^ ERROR invalid reference to argument `1` (there is 1 argument)
27+
//~^ ERROR: invalid reference to positional argument 1 (there is 1 argument)
3728

3829
format!("{0} {1} {2}", 1, 2);
39-
//~^ ERROR invalid reference to argument `2` (there are 2 arguments)
40-
41-
format!("{0} {1}");
42-
//~^ ERROR invalid reference to argument `0` (no arguments given)
43-
//~^^ ERROR invalid reference to argument `1` (no arguments given)
30+
//~^ ERROR: invalid reference to positional argument 2 (there are 2 arguments)
31+
32+
format!("{} {value} {} {}", 1, value=2);
33+
//~^ ERROR: invalid reference to positional argument 2 (there are 2 arguments)
34+
format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2);
35+
//~^ ERROR: invalid reference to positional arguments 3, 4 and 5 (there are 3 arguments)
36+
37+
format!("{} {foo} {} {bar} {}", 1, 2, 3);
38+
//~^ ERROR: there is no argument named `foo`
39+
//~^^ ERROR: there is no argument named `bar`
40+
41+
format!("{foo}"); //~ ERROR: no argument named `foo`
42+
format!("", 1, 2); //~ ERROR: multiple unused formatting arguments
43+
format!("{}", 1, 2); //~ ERROR: argument never used
44+
format!("{1}", 1, 2); //~ ERROR: argument never used
45+
format!("{}", 1, foo=2); //~ ERROR: named argument never used
46+
format!("{foo}", 1, foo=2); //~ ERROR: argument never used
47+
format!("", foo=2); //~ ERROR: named argument never used
48+
format!("{} {}", 1, 2, foo=1, bar=2); //~ ERROR: multiple unused formatting arguments
49+
50+
format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument
51+
format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow
4452

4553
// bad named arguments, #35082
4654

‎src/test/ui/cross-crate-macro-backtrace/main.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
error: invalid reference to argument `0` (no arguments given)
1+
error: 1 positional argument in format string, but no arguments were given
22
--> $DIR/main.rs:16:5
33
|
44
16 | myprintln!("{}"); //~ ERROR in this macro

‎src/test/ui/macros/macro-backtrace-println.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
error: invalid reference to argument `0` (no arguments given)
1+
error: 1 positional argument in format string, but no arguments were given
22
--> $DIR/macro-backtrace-println.rs:24:30
33
|
44
24 | ($fmt:expr) => (myprint!(concat!($fmt, "/n")));

0 commit comments

Comments
 (0)