Skip to content

On format!() arg count mismatch provide extra info #63121

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 5 commits into from
Aug 3, 2019
Merged

On format!() arg count mismatch provide extra info #63121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
159dcb2
On `format!()` arg count mismatch provide extra info
estebank Jul 30, 2019
86f4f68
Improve handling of invalid references in `format!()`
estebank Jul 30, 2019
762f645
review comments
estebank Jul 30, 2019
9e59d74
fix tests
estebank Jul 30, 2019
22ea38d
fix dedup
estebank Jul 31, 2019
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
76 changes: 52 additions & 24 deletions src/libfmt_macros/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,20 @@ pub struct Argument<'a> {
/// Specification for the formatting of an argument in the format string.
#[derive(Copy, Clone, PartialEq)]
pub struct FormatSpec<'a> {
/// Optionally specified character to fill alignment with
/// Optionally specified character to fill alignment with.
pub fill: Option<char>,
/// Optionally specified alignment
/// Optionally specified alignment.
pub align: Alignment,
/// Packed version of various flags provided
/// Packed version of various flags provided.
pub flags: u32,
/// The integer precision to use
/// The integer precision to use.
pub precision: Count,
/// The string width requested for the resulting format
/// The span of the precision formatting flag (for diagnostics).
pub precision_span: Option<InnerSpan>,
/// The string width requested for the resulting format.
pub width: Count,
/// The span of the width formatting flag (for diagnostics).
pub width_span: Option<InnerSpan>,
/// The descriptor string representing the name of the format desired for
/// this argument, this can be empty or any number of characters, although
/// it is required to be one word.
Expand Down Expand Up @@ -285,19 +289,24 @@ impl<'a> Parser<'a> {
}

/// Optionally consumes the specified character. If the character is not at
/// the current position, then the current iterator isn't moved and false is
/// returned, otherwise the character is consumed and true is returned.
/// the current position, then the current iterator isn't moved and `false` is
/// returned, otherwise the character is consumed and `true` is returned.
fn consume(&mut self, c: char) -> bool {
if let Some(&(_, maybe)) = self.cur.peek() {
self.consume_pos(c).is_some()
}

/// Optionally consumes the specified character. If the character is not at
/// the current position, then the current iterator isn't moved and `None` is
/// returned, otherwise the character is consumed and the current position is
/// returned.
fn consume_pos(&mut self, c: char) -> Option<usize> {
if let Some(&(pos, maybe)) = self.cur.peek() {
if c == maybe {
self.cur.next();
true
} else {
false
return Some(pos);
}
} else {
false
}
None
}

fn to_span_index(&self, pos: usize) -> InnerOffset {
Expand Down Expand Up @@ -465,7 +474,9 @@ impl<'a> Parser<'a> {
align: AlignUnknown,
flags: 0,
precision: CountImplied,
precision_span: None,
width: CountImplied,
width_span: None,
ty: &self.input[..0],
};
if !self.consume(':') {
Expand Down Expand Up @@ -502,6 +513,11 @@ impl<'a> Parser<'a> {
}
// Width and precision
let mut havewidth = false;

let mut width_span_start = 0;
if let Some((pos, '0')) = self.cur.peek() {
width_span_start = *pos;
}
if self.consume('0') {
// small ambiguity with '0$' as a format string. In theory this is a
// '0' flag and then an ill-formatted format string with just a '$'
Expand All @@ -515,17 +531,28 @@ impl<'a> Parser<'a> {
}
}
if !havewidth {
spec.width = self.count();
if width_span_start == 0 {
if let Some((pos, _)) = self.cur.peek() {
width_span_start = *pos;
}
}
let (w, sp) = self.count(width_span_start);
spec.width = w;
spec.width_span = sp;
}
if self.consume('.') {
if self.consume('*') {
if let Some(start) = self.consume_pos('.') {
if let Some(end) = self.consume_pos('*') {
// Resolve `CountIsNextParam`.
// We can do this immediately as `position` is resolved later.
let i = self.curarg;
self.curarg += 1;
spec.precision = CountIsParam(i);
spec.precision_span =
Some(self.to_span_index(start).to(self.to_span_index(end + 1)));
} else {
spec.precision = self.count();
let (p, sp) = self.count(start);
spec.precision = p;
spec.precision_span = sp;
}
}
// Optional radix followed by the actual format specifier
Expand Down Expand Up @@ -554,24 +581,25 @@ impl<'a> Parser<'a> {
/// Parses a Count parameter at the current position. This does not check
/// for 'CountIsNextParam' because that is only used in precision, not
/// width.
fn count(&mut self) -> Count {
fn count(&mut self, start: usize) -> (Count, Option<InnerSpan>) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it's pretty tailored in the implementation, but perhaps this could be somewhat more generalized? For example instead of modifying existing methods and having some which return a span and some which don't (in addition to passing around start spans and such) could one new function be added liek:

fn span<R>(&mut self, f: impl FnOnce(&mut Self) -> R) -> (R, Span) {
    // ...
}

and that's called like let (count, span) = self.span(|me| me.count()); or something like that?

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 thing is that the actual span for the thing being composed requires some passing of location between different methods. count in particular is only used in two places and both care about the span. These methods are meant to only be used from a single module, which is why I didn't bother with an externally pleasing api surface for them.

if let Some(i) = self.integer() {
if self.consume('$') {
CountIsParam(i)
if let Some(end) = self.consume_pos('$') {
let span = self.to_span_index(start).to(self.to_span_index(end + 1));
(CountIsParam(i), Some(span))
} else {
CountIs(i)
(CountIs(i), None)
}
} else {
let tmp = self.cur.clone();
let word = self.word();
if word.is_empty() {
self.cur = tmp;
CountImplied
(CountImplied, None)
} else if self.consume('$') {
CountIsName(Symbol::intern(word))
(CountIsName(Symbol::intern(word)), None)
} else {
self.cur = tmp;
CountImplied
(CountImplied, None)
}
}
}
Expand Down
107 changes: 92 additions & 15 deletions src/libsyntax_ext/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ struct Context<'a, 'b> {
invalid_refs: Vec<(usize, usize)>,
/// Spans of all the formatting arguments, in order.
arg_spans: Vec<Span>,
/// All the formatting arguments that have formatting flags set, in order for diagnostics.
arg_with_formatting: Vec<parse::FormatSpec<'a>>,
/// Whether this formatting string is a literal or it comes from a macro.
is_literal: bool,
}
Expand Down Expand Up @@ -279,14 +281,20 @@ impl<'a, 'b> Context<'a, 'b> {
.iter()
.map(|(r, pos)| (r.to_string(), self.arg_spans.get(*pos)));

let mut zero_based_note = false;

if self.names.is_empty() && !numbered_position_args {
let count = self.pieces.len() + self.arg_with_formatting
.iter()
.filter(|fmt| fmt.precision_span.is_some())
.count();
e = self.ecx.mut_span_err(
sp,
&format!(
"{} positional argument{} in format string, but {}",
self.pieces.len(),
if self.pieces.len() > 1 { "s" } else { "" },
self.describe_num_args()
count,
if count > 1 { "s" } else { "" },
self.describe_num_args(),
),
);
} else {
Expand Down Expand Up @@ -317,9 +325,70 @@ impl<'a, 'b> Context<'a, 'b> {
&format!("invalid reference to positional {} ({})",
arg_list,
self.describe_num_args()));
e.note("positional arguments are zero-based");
zero_based_note = true;
};

for fmt in &self.arg_with_formatting {
if let Some(span) = fmt.precision_span {
let span = self.fmtsp.from_inner(span);
match fmt.precision {
parse::CountIsParam(pos) if pos > self.args.len() => {
e.span_label(span, &format!(
"this precision flag expects an `usize` argument at position {}, \
but {}",
pos,
self.describe_num_args(),
));
zero_based_note = true;
}
parse::CountIsParam(pos) => {
let count = self.pieces.len() + self.arg_with_formatting
.iter()
.filter(|fmt| fmt.precision_span.is_some())
.count();
e.span_label(span, &format!(
"this precision flag adds an extra required argument at position {}, \
which is why there {} expected",
pos,
if count == 1 {
"is 1 argument".to_string()
} else {
format!("are {} arguments", count)
},
));
e.span_label(
self.args[pos].span,
"this parameter corresponds to the precision flag",
);
zero_based_note = true;
}
_ => {}
}
}
if let Some(span) = fmt.width_span {
let span = self.fmtsp.from_inner(span);
match fmt.width {
parse::CountIsParam(pos) if pos > self.args.len() => {
e.span_label(span, &format!(
"this width flag expects an `usize` argument at position {}, \
but {}",
pos,
self.describe_num_args(),
));
zero_based_note = true;
}
_ => {}
}
}
}
if zero_based_note {
e.note("positional arguments are zero-based");
}
if !self.arg_with_formatting.is_empty() {
e.note("for information about formatting flags, visit \
https://doc.rust-lang.org/std/fmt/index.html");
}

e.emit();
}

Expand Down Expand Up @@ -435,10 +504,11 @@ impl<'a, 'b> Context<'a, 'b> {

/// Builds a static `rt::Argument` from a `parse::Piece` or append
/// to the `literal` string.
fn build_piece(&mut self,
piece: &parse::Piece<'_>,
arg_index_consumed: &mut Vec<usize>)
-> Option<P<ast::Expr>> {
fn build_piece(
&mut self,
piece: &parse::Piece<'a>,
arg_index_consumed: &mut Vec<usize>,
) -> Option<P<ast::Expr>> {
let sp = self.macsp;
match *piece {
parse::String(s) => {
Expand Down Expand Up @@ -496,7 +566,9 @@ impl<'a, 'b> Context<'a, 'b> {
align: parse::AlignUnknown,
flags: 0,
precision: parse::CountImplied,
precision_span: None,
width: parse::CountImplied,
width_span: None,
ty: arg.format.ty,
},
};
Expand All @@ -506,6 +578,9 @@ impl<'a, 'b> Context<'a, 'b> {
let pos_simple =
arg.position.index() == simple_arg.position.index();

if arg.format.precision_span.is_some() || arg.format.width_span.is_some() {
self.arg_with_formatting.push(arg.format); //'liself.fmtsp.from_inner(span));
}
if !pos_simple || arg.format != simple_arg.format || fill != ' ' {
self.all_pieces_simple = false;
}
Expand All @@ -530,7 +605,7 @@ impl<'a, 'b> Context<'a, 'b> {
let path = self.ecx.path_global(sp, Context::rtpath(self.ecx, "FormatSpec"));
let fmt = self.ecx.expr_struct(
sp,
path,
path,
vec![
self.ecx.field_imm(sp, self.ecx.ident_of("fill"), fill),
self.ecx.field_imm(sp, self.ecx.ident_of("align"), align),
Expand Down Expand Up @@ -657,12 +732,13 @@ impl<'a, 'b> Context<'a, 'b> {
self.ecx.expr_call_global(self.macsp, path, fn_args)
}

fn format_arg(ecx: &ExtCtxt<'_>,
macsp: Span,
mut sp: Span,
ty: &ArgumentType,
arg: ast::Ident)
-> P<ast::Expr> {
fn format_arg(
ecx: &ExtCtxt<'_>,
macsp: Span,
mut sp: Span,
ty: &ArgumentType,
arg: ast::Ident,
) -> P<ast::Expr> {
sp = sp.apply_mark(ecx.current_expansion.id);
let arg = ecx.expr_ident(sp, arg);
let trait_ = match *ty {
Expand Down Expand Up @@ -941,6 +1017,7 @@ pub fn expand_preparsed_format_args(
fmtsp: fmt.span,
invalid_refs: Vec::new(),
arg_spans,
arg_with_formatting: Vec::new(),
is_literal,
};

Expand Down
8 changes: 8 additions & 0 deletions src/test/ui/if/ifmt-bad-arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,12 @@ ninth number: {
tenth number: {}",
1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
//~^^ ERROR: invalid format string
println!("{} {:.*} {}", 1, 3.2, 4);
//~^ ERROR 4 positional arguments in format string, but there are 3 arguments
//~| ERROR mismatched types
println!("{} {:07$.*} {}", 1, 3.2, 4);
//~^ ERROR 4 positional arguments in format string, but there are 3 arguments
//~| ERROR mismatched types
println!("{} {:07$} {}", 1, 3.2, 4);
//~^ ERROR 3 positional arguments in format string, but there are 3 arguments
}
55 changes: 54 additions & 1 deletion src/test/ui/if/ifmt-bad-arg.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,5 +220,58 @@ LL | tenth number: {}",
|
= note: if you intended to print `{`, you can escape it using `{{`

error: aborting due to 28 previous errors
error: 4 positional arguments in format string, but there are 3 arguments
--> $DIR/ifmt-bad-arg.rs:78:15
|
LL | println!("{} {:.*} {}", 1, 3.2, 4);
| ^^ ^^--^ ^^ --- this parameter corresponds to the precision flag
| |
| this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
|
= note: positional arguments are zero-based
= note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 4 positional arguments in format string, but there are 3 arguments
--> $DIR/ifmt-bad-arg.rs:81:15
|
LL | println!("{} {:07$.*} {}", 1, 3.2, 4);
| ^^ ^^-----^ ^^ --- this parameter corresponds to the precision flag
| | |
| | this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
| this width flag expects an `usize` argument at position 7, but there are 3 arguments
|
= note: positional arguments are zero-based
= note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 3 positional arguments in format string, but there are 3 arguments
--> $DIR/ifmt-bad-arg.rs:84:15
|
LL | println!("{} {:07$} {}", 1, 3.2, 4);
| ^^ ^^---^ ^^
| |
| this width flag expects an `usize` argument at position 7, but there are 3 arguments
|
= note: positional arguments are zero-based
= note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error[E0308]: mismatched types
--> $DIR/ifmt-bad-arg.rs:78:32
|
LL | println!("{} {:.*} {}", 1, 3.2, 4);
| ^^^ expected usize, found floating-point number
|
= note: expected type `&usize`
found type `&{float}`

error[E0308]: mismatched types
--> $DIR/ifmt-bad-arg.rs:81:35
|
LL | println!("{} {:07$.*} {}", 1, 3.2, 4);
| ^^^ expected usize, found floating-point number
|
= note: expected type `&usize`
found type `&{float}`

error: aborting due to 33 previous errors

For more information about this error, try `rustc --explain E0308`.