From 38abca8c2d7de08861cd61bc439efdb7cf4de398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 22 Jul 2018 22:40:24 -0700 Subject: [PATCH 01/11] Point at internal span in format string --- src/libfmt_macros/lib.rs | 17 +- src/libsyntax_ext/format.rs | 34 ++-- src/test/{compile-fail => ui}/ifmt-bad-arg.rs | 0 src/test/ui/ifmt-bad-arg.stderr | 187 ++++++++++++++++++ .../ui/macros/macro-backtrace-println.stderr | 4 +- 5 files changed, 224 insertions(+), 18 deletions(-) rename src/test/{compile-fail => ui}/ifmt-bad-arg.rs (100%) create mode 100644 src/test/ui/ifmt-bad-arg.stderr diff --git a/src/libfmt_macros/lib.rs b/src/libfmt_macros/lib.rs index 9952e5f64d6ab..f6dcebf8c50f5 100644 --- a/src/libfmt_macros/lib.rs +++ b/src/libfmt_macros/lib.rs @@ -154,6 +154,7 @@ pub struct Parser<'a> { style: Option, /// How many newlines have been seen in the string so far, to adjust the error spans seen_newlines: usize, + pub arg_places: Vec<(usize, usize)>, } impl<'a> Iterator for Parser<'a> { @@ -168,9 +169,13 @@ impl<'a> Iterator for Parser<'a> { if self.consume('{') { Some(String(self.string(pos + 1))) } else { - let ret = Some(NextArgument(self.argument())); - self.must_consume('}'); - ret + let mut arg = self.argument(); + if let Some(arg_pos) = self.must_consume('}').map(|end| { + (pos + raw + 1, end + raw + 2) + }) { + self.arg_places.push(arg_pos); + } + Some(NextArgument(arg)) } } '}' => { @@ -211,6 +216,7 @@ impl<'a> Parser<'a> { curarg: 0, style, seen_newlines: 0, + arg_places: vec![], } } @@ -271,7 +277,7 @@ impl<'a> Parser<'a> { /// Forces consumption of the specified character. If the character is not /// found, an error is emitted. - fn must_consume(&mut self, c: char) { + fn must_consume(&mut self, c: char) -> Option { self.ws(); let raw = self.style.unwrap_or(0); @@ -279,12 +285,14 @@ impl<'a> Parser<'a> { if let Some(&(pos, maybe)) = self.cur.peek() { if c == maybe { self.cur.next(); + Some(pos) } else { let pos = pos + padding + 1; self.err(format!("expected `{:?}`, found `{:?}`", c, maybe), format!("expected `{}`", c), pos, pos); + None } } else { let msg = format!("expected `{:?}` but string was terminated", c); @@ -302,6 +310,7 @@ impl<'a> Parser<'a> { } else { self.err(msg, format!("expected `{:?}`", c), pos, pos); } + None } } diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index 755d2b476b716..4700f814e5853 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -21,7 +21,7 @@ use syntax::feature_gate; use syntax::parse::token; use syntax::ptr::P; use syntax::symbol::Symbol; -use syntax_pos::{Span, DUMMY_SP}; +use syntax_pos::{Span, MultiSpan, DUMMY_SP}; use syntax::tokenstream; use std::collections::{HashMap, HashSet}; @@ -264,28 +264,38 @@ impl<'a, 'b> Context<'a, 'b> { /// 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) { + fn report_invalid_references(&self, numbered_position_args: bool, arg_places: &[(usize, usize)]) { let mut e; - let mut refs: Vec = self.invalid_refs - .iter() - .map(|r| r.to_string()) - .collect(); + let sps = arg_places.iter() + .map(|&(start, end)| self.fmtsp.from_inner_byte_pos(start, end)) + .collect::>(); + let sp = MultiSpan::from_spans(sps); + let mut refs: Vec<_> = 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, + 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())); } else { let arg_list = match refs.len() { - 1 => format!("argument {}", refs.pop().unwrap()), - _ => format!("arguments {head} and {tail}", - tail=refs.pop().unwrap(), + 1 => { + let reg = refs.pop().unwrap(); + format!("argument {}", reg) + }, + _ => { + let reg = refs.pop().unwrap(); + format!("arguments {head} and {tail}", + tail=reg, head=refs.join(", ")) + } }; - e = self.ecx.mut_span_err(self.fmtsp, + e = self.ecx.mut_span_err(sp, &format!("invalid reference to positional {} ({})", arg_list, self.describe_num_args())); @@ -835,7 +845,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } if cx.invalid_refs.len() >= 1 { - cx.report_invalid_references(numbered_position_args); + cx.report_invalid_references(numbered_position_args, &parser.arg_places); } // Make sure that all arguments were used and all arguments have types. diff --git a/src/test/compile-fail/ifmt-bad-arg.rs b/src/test/ui/ifmt-bad-arg.rs similarity index 100% rename from src/test/compile-fail/ifmt-bad-arg.rs rename to src/test/ui/ifmt-bad-arg.rs diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr new file mode 100644 index 0000000000000..4ad3c2b655047 --- /dev/null +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -0,0 +1,187 @@ +error: 1 positional argument in format string, but no arguments were given + --> $DIR/ifmt-bad-arg.rs:16:14 + | +LL | format!("{}"); + | ^^ + +error: invalid reference to positional argument 1 (there is 1 argument) + --> $DIR/ifmt-bad-arg.rs:19:14 + | +LL | format!("{1}", 1); + | ^^^ + | + = note: positional arguments are zero-based + +error: argument never used + --> $DIR/ifmt-bad-arg.rs:19:20 + | +LL | format!("{1}", 1); + | ^ + +error: 2 positional arguments in format string, but no arguments were given + --> $DIR/ifmt-bad-arg.rs:23:14 + | +LL | format!("{} {}"); + | ^^ ^^ + +error: invalid reference to positional argument 1 (there is 1 argument) + --> $DIR/ifmt-bad-arg.rs:26:14 + | +LL | format!("{0} {1}", 1); + | ^^^ ^^^ + | + = note: positional arguments are zero-based + +error: invalid reference to positional argument 2 (there are 2 arguments) + --> $DIR/ifmt-bad-arg.rs:29:14 + | +LL | format!("{0} {1} {2}", 1, 2); + | ^^^ ^^^ ^^^ + | + = note: positional arguments are zero-based + +error: invalid reference to positional argument 2 (there are 2 arguments) + --> $DIR/ifmt-bad-arg.rs:32:14 + | +LL | format!("{} {value} {} {}", 1, value=2); + | ^^ ^^^^^^^ ^^ ^^ + | + = note: positional arguments are zero-based + +error: invalid reference to positional arguments 3, 4 and 5 (there are 3 arguments) + --> $DIR/ifmt-bad-arg.rs:34:14 + | +LL | format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2); + | ^^^^^^ ^^^^^^^ ^^ ^^ ^^ ^^ ^^ ^^ + | + = note: positional arguments are zero-based + +error: there is no argument named `foo` + --> $DIR/ifmt-bad-arg.rs:37:13 + | +LL | format!("{} {foo} {} {bar} {}", 1, 2, 3); + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: there is no argument named `bar` + --> $DIR/ifmt-bad-arg.rs:37:13 + | +LL | format!("{} {foo} {} {bar} {}", 1, 2, 3); + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: there is no argument named `foo` + --> $DIR/ifmt-bad-arg.rs:41:13 + | +LL | format!("{foo}"); //~ ERROR: no argument named `foo` + | ^^^^^^^ + +error: multiple unused formatting arguments + --> $DIR/ifmt-bad-arg.rs:42:17 + | +LL | format!("", 1, 2); //~ ERROR: multiple unused formatting arguments + | -- ^ ^ + | | + | multiple missing formatting arguments + +error: argument never used + --> $DIR/ifmt-bad-arg.rs:43:22 + | +LL | format!("{}", 1, 2); //~ ERROR: argument never used + | ^ + +error: argument never used + --> $DIR/ifmt-bad-arg.rs:44:20 + | +LL | format!("{1}", 1, 2); //~ ERROR: argument never used + | ^ + +error: named argument never used + --> $DIR/ifmt-bad-arg.rs:45:26 + | +LL | format!("{}", 1, foo=2); //~ ERROR: named argument never used + | ^ + +error: argument never used + --> $DIR/ifmt-bad-arg.rs:46:22 + | +LL | format!("{foo}", 1, foo=2); //~ ERROR: argument never used + | ^ + +error: named argument never used + --> $DIR/ifmt-bad-arg.rs:47:21 + | +LL | format!("", foo=2); //~ ERROR: named argument never used + | ^ + +error: multiple unused formatting arguments + --> $DIR/ifmt-bad-arg.rs:48:32 + | +LL | format!("{} {}", 1, 2, foo=1, bar=2); //~ ERROR: multiple unused formatting arguments + | ------- ^ ^ + | | + | multiple missing formatting arguments + +error: duplicate argument named `foo` + --> $DIR/ifmt-bad-arg.rs:50:33 + | +LL | format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument + | ^ + | +note: previously here + --> $DIR/ifmt-bad-arg.rs:50:26 + | +LL | format!("{foo}", foo=1, foo=2); //~ ERROR: duplicate argument + | ^ + +error: expected ident, positional arguments cannot follow named arguments + --> $DIR/ifmt-bad-arg.rs:51:24 + | +LL | format!("", foo=1, 2); //~ ERROR: positional arguments cannot follow + | ^ + +error: there is no argument named `valueb` + --> $DIR/ifmt-bad-arg.rs:55:13 + | +LL | format!("{valuea} {valueb}", valuea=5, valuec=7); + | ^^^^^^^^^^^^^^^^^^^ + +error: named argument never used + --> $DIR/ifmt-bad-arg.rs:55:51 + | +LL | format!("{valuea} {valueb}", valuea=5, valuec=7); + | ^ + +error: invalid format string: expected `'}'` but string was terminated + --> $DIR/ifmt-bad-arg.rs:61:15 + | +LL | format!("{"); //~ ERROR: expected `'}'` but string was terminated + | ^ expected `'}'` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: unmatched `}` found + --> $DIR/ifmt-bad-arg.rs:63:18 + | +LL | format!("foo } bar"); //~ ERROR: unmatched `}` found + | ^ unmatched `}` in format string + | + = note: if you intended to print `}`, you can escape it using `}}` + +error: invalid format string: unmatched `}` found + --> $DIR/ifmt-bad-arg.rs:64:18 + | +LL | format!("foo }"); //~ ERROR: unmatched `}` found + | ^ unmatched `}` in format string + | + = note: if you intended to print `}`, you can escape it using `}}` + +error: argument never used + --> $DIR/ifmt-bad-arg.rs:66:27 + | +LL | format!("foo %s baz", "bar"); //~ ERROR: argument never used + | ^^^^^ + | + = help: `%s` should be written as `{}` + = note: printf formatting not supported; see the documentation for `std::fmt` + +error: aborting due to 26 previous errors + diff --git a/src/test/ui/macros/macro-backtrace-println.stderr b/src/test/ui/macros/macro-backtrace-println.stderr index 8f2eb0173a499..f0ca576f652eb 100644 --- a/src/test/ui/macros/macro-backtrace-println.stderr +++ b/src/test/ui/macros/macro-backtrace-println.stderr @@ -1,8 +1,8 @@ error: 1 positional argument in format string, but no arguments were given - --> $DIR/macro-backtrace-println.rs:24:30 + --> $DIR/macro-backtrace-println.rs:24:31 | LL | ($fmt:expr) => (myprint!(concat!($fmt, "/n"))); //~ ERROR no arguments were given - | ^^^^^^^^^^^^^^^^^^^ + | ^^ ... LL | myprintln!("{}"); | ----------------- in this macro invocation From 42306591b9c0a280da363c83df16b47ad8b04024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 23 Jul 2018 00:01:17 -0700 Subject: [PATCH 02/11] Point at incorrect named arg in format string --- src/libsyntax_ext/format.rs | 21 +++++++++++++++++++-- src/test/ui/ifmt-bad-arg.stderr | 16 ++++++++-------- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index 4700f814e5853..215e4f5a83523 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -111,8 +111,10 @@ struct Context<'a, 'b: 'a> { /// still existed in this phase of processing. /// Used only for `all_pieces_simple` tracking in `build_piece`. curarg: usize, + curpiece: usize, /// Keep track of invalid references to positional arguments invalid_refs: Vec, + arg_spans: Vec, } /// Parses the arguments from the given list of tokens, returning None @@ -235,6 +237,7 @@ impl<'a, 'b> Context<'a, 'b> { let ty = Placeholder(arg.format.ty.to_string()); self.verify_arg_type(pos, ty); + self.curpiece += 1; } } } @@ -347,7 +350,9 @@ impl<'a, 'b> Context<'a, 'b> { Some(e) => *e, None => { let msg = format!("there is no argument named `{}`", name); - self.ecx.span_err(self.fmtsp, &msg[..]); + let sp = *self.arg_spans.get(self.curpiece).unwrap_or(&self.fmtsp); + let mut err = self.ecx.struct_span_err(sp, &msg[..]); + err.emit(); return; } }; @@ -773,6 +778,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, arg_unique_types, names, curarg: 0, + curpiece: 0, arg_index_map: Vec::new(), count_args: Vec::new(), count_positions: HashMap::new(), @@ -785,6 +791,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, macsp, fmtsp: fmt.span, invalid_refs: Vec::new(), + arg_spans: Vec::new(), }; let fmt_str = &*fmt.node.0.as_str(); @@ -793,12 +800,22 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, ast::StrStyle::Raw(raw) => Some(raw as usize), }; let mut parser = parse::Parser::new(fmt_str, str_style); + let mut unverified_pieces = vec![]; let mut pieces = vec![]; - while let Some(mut piece) = parser.next() { + while let Some(piece) = parser.next() { if !parser.errors.is_empty() { break; } + unverified_pieces.push(piece); + } + + cx.arg_spans = parser.arg_places.iter() + .map(|&(start, end)| fmt.span.from_inner_byte_pos(start, end)) + .collect(); + + // This needs to happen *after* the Parser has consumed all pieces to create all the spans + for mut piece in unverified_pieces { cx.verify_piece(&piece); cx.resolve_name_inplace(&mut piece); pieces.push(piece); diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index 4ad3c2b655047..2d49c36c06de1 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -57,22 +57,22 @@ LL | format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2); = note: positional arguments are zero-based error: there is no argument named `foo` - --> $DIR/ifmt-bad-arg.rs:37:13 + --> $DIR/ifmt-bad-arg.rs:37:17 | LL | format!("{} {foo} {} {bar} {}", 1, 2, 3); - | ^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^ error: there is no argument named `bar` - --> $DIR/ifmt-bad-arg.rs:37:13 + --> $DIR/ifmt-bad-arg.rs:37:26 | LL | format!("{} {foo} {} {bar} {}", 1, 2, 3); - | ^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^ error: there is no argument named `foo` - --> $DIR/ifmt-bad-arg.rs:41:13 + --> $DIR/ifmt-bad-arg.rs:41:14 | LL | format!("{foo}"); //~ ERROR: no argument named `foo` - | ^^^^^^^ + | ^^^^^ error: multiple unused formatting arguments --> $DIR/ifmt-bad-arg.rs:42:17 @@ -139,10 +139,10 @@ LL | format!("", foo=1, 2); //~ ERROR: positional arguments cannot | ^ error: there is no argument named `valueb` - --> $DIR/ifmt-bad-arg.rs:55:13 + --> $DIR/ifmt-bad-arg.rs:55:23 | LL | format!("{valuea} {valueb}", valuea=5, valuec=7); - | ^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^ error: named argument never used --> $DIR/ifmt-bad-arg.rs:55:51 From 6bcf8777fe632fb2c506e31d12fbfe20712ecfe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 23 Jul 2018 15:09:00 -0700 Subject: [PATCH 03/11] Point only at invalid positional arguments --- src/libsyntax_ext/format.rs | 138 ++++++++++++++++++-------------- src/test/ui/ifmt-bad-arg.stderr | 16 ++-- 2 files changed, 84 insertions(+), 70 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index 215e4f5a83523..a320b52fb7bd9 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -14,18 +14,18 @@ use self::Position::*; use fmt_macros as parse; use syntax::ast; -use syntax::ext::base::*; use syntax::ext::base; +use syntax::ext::base::*; use syntax::ext::build::AstBuilder; use syntax::feature_gate; use syntax::parse::token; use syntax::ptr::P; use syntax::symbol::Symbol; -use syntax_pos::{Span, MultiSpan, DUMMY_SP}; use syntax::tokenstream; +use syntax_pos::{MultiSpan, Span, DUMMY_SP}; -use std::collections::{HashMap, HashSet}; use std::collections::hash_map::Entry; +use std::collections::{HashMap, HashSet}; #[derive(PartialEq)] enum ArgumentType { @@ -111,9 +111,11 @@ struct Context<'a, 'b: 'a> { /// still existed in this phase of processing. /// Used only for `all_pieces_simple` tracking in `build_piece`. curarg: usize, + /// Current piece being evaluated, used for error reporting. curpiece: usize, - /// Keep track of invalid references to positional arguments - invalid_refs: Vec, + /// Keep track of invalid references to positional arguments. + invalid_refs: Vec<(usize, usize)>, + /// Spans of all the formatting arguments, in order. arg_spans: Vec, } @@ -157,15 +159,20 @@ fn parse_args(ecx: &mut ExtCtxt, i } _ if named => { - ecx.span_err(p.span, - "expected ident, positional arguments \ - cannot follow named arguments"); + ecx.span_err( + p.span, + "expected ident, positional arguments cannot follow named arguments", + ); return None; } _ => { - ecx.span_err(p.span, - &format!("expected ident for named argument, found `{}`", - p.this_token_to_string())); + ecx.span_err( + p.span, + &format!( + "expected ident for named argument, found `{}`", + p.this_token_to_string() + ), + ); return None; } }; @@ -267,34 +274,47 @@ impl<'a, 'b> Context<'a, 'b> { /// 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, arg_places: &[(usize, usize)]) { + fn report_invalid_references(&self, numbered_position_args: bool) { let mut e; - let sps = arg_places.iter() - .map(|&(start, end)| self.fmtsp.from_inner_byte_pos(start, end)) - .collect::>(); - let sp = MultiSpan::from_spans(sps); - let mut refs: Vec<_> = self.invalid_refs + let sp = MultiSpan::from_spans(self.arg_spans.clone()); + let mut refs: Vec<_> = self + .invalid_refs .iter() - .map(|r| r.to_string()) + .map(|(r, pos)| (r.to_string(), self.arg_spans.get(*pos))) .collect(); if self.names.is_empty() && !numbered_position_args { - e = self.ecx.mut_span_err(sp, - &format!("{} positional argument{} in format string, but {}", + 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())); + self.describe_num_args() + ), + ); } else { - let arg_list = match refs.len() { + let (arg_list, sp) = match refs.len() { 1 => { - let reg = refs.pop().unwrap(); - format!("argument {}", reg) - }, + let (reg, pos) = refs.pop().unwrap(); + ( + format!("argument {}", reg), + MultiSpan::from_span(*pos.unwrap_or(&self.fmtsp)), + ) + } _ => { + let pos = + MultiSpan::from_spans(refs.iter().map(|(_, p)| *p.unwrap()).collect()); + let mut refs: Vec = refs.iter().map(|(s, _)| s.to_owned()).collect(); let reg = refs.pop().unwrap(); - format!("arguments {head} and {tail}", - tail=reg, - head=refs.join(", ")) + ( + format!( + "arguments {head} and {tail}", + tail = reg, + head = refs.join(", ") + ), + pos, + ) } }; @@ -314,7 +334,7 @@ impl<'a, 'b> Context<'a, 'b> { match arg { Exact(arg) => { if self.args.len() <= arg { - self.invalid_refs.push(arg); + self.invalid_refs.push((arg, self.curpiece)); return; } match ty { @@ -520,33 +540,27 @@ impl<'a, 'b> Context<'a, 'b> { let prec = self.build_count(arg.format.precision); let width = self.build_count(arg.format.width); let path = self.ecx.path_global(sp, Context::rtpath(self.ecx, "FormatSpec")); - let fmt = - self.ecx.expr_struct(sp, + let fmt = self.ecx.expr_struct( + sp, path, - vec![self.ecx - .field_imm(sp, self.ecx.ident_of("fill"), fill), - self.ecx.field_imm(sp, - self.ecx.ident_of("align"), - align), - self.ecx.field_imm(sp, - self.ecx.ident_of("flags"), - flags), - self.ecx.field_imm(sp, - self.ecx.ident_of("precision"), - prec), - self.ecx.field_imm(sp, - self.ecx.ident_of("width"), - width)]); + vec![ + self.ecx.field_imm(sp, self.ecx.ident_of("fill"), fill), + self.ecx.field_imm(sp, self.ecx.ident_of("align"), align), + self.ecx.field_imm(sp, self.ecx.ident_of("flags"), flags), + self.ecx.field_imm(sp, self.ecx.ident_of("precision"), prec), + self.ecx.field_imm(sp, self.ecx.ident_of("width"), width), + ], + ); let path = self.ecx.path_global(sp, Context::rtpath(self.ecx, "Argument")); - Some(self.ecx.expr_struct(sp, + Some(self.ecx.expr_struct( + sp, path, - vec![self.ecx.field_imm(sp, - self.ecx.ident_of("position"), - pos), - self.ecx.field_imm(sp, - self.ecx.ident_of("format"), - fmt)])) + vec![ + self.ecx.field_imm(sp, self.ecx.ident_of("position"), pos), + self.ecx.field_imm(sp, self.ecx.ident_of("format"), fmt), + ], + )) } } } @@ -559,9 +573,9 @@ impl<'a, 'b> Context<'a, 'b> { let mut pats = Vec::new(); let mut heads = Vec::new(); - let names_pos: Vec<_> = (0..self.args.len()).map(|i| { - self.ecx.ident_of(&format!("arg{}", i)).gensym() - }).collect(); + let names_pos: Vec<_> = (0..self.args.len()) + .map(|i| self.ecx.ident_of(&format!("arg{}", i)).gensym()) + .collect(); // First, build up the static array which will become our precompiled // format "string" @@ -705,10 +719,11 @@ pub fn expand_format_args<'cx>(ecx: &'cx mut ExtCtxt, } } -pub fn expand_format_args_nl<'cx>(ecx: &'cx mut ExtCtxt, +pub fn expand_format_args_nl<'cx>( + ecx: &'cx mut ExtCtxt, mut sp: Span, - tts: &[tokenstream::TokenTree]) - -> Box { + tts: &[tokenstream::TokenTree], +) -> Box { //if !ecx.ecfg.enable_allow_internal_unstable() { // For some reason, the only one that actually works for `println` is the first check @@ -759,7 +774,6 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, let sugg_fmt = match args.len() { 0 => "{}".to_string(), _ => format!("{}{{}}", "{} ".repeat(args.len())), - }; err.span_suggestion( fmt_sp.shrink_to_lo(), @@ -768,7 +782,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, ); err.emit(); return DummyResult::raw_expr(sp); - }, + } }; let mut cx = Context { @@ -862,7 +876,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } if cx.invalid_refs.len() >= 1 { - cx.report_invalid_references(numbered_position_args, &parser.arg_places); + cx.report_invalid_references(numbered_position_args); } // Make sure that all arguments were used and all arguments have types. @@ -894,7 +908,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } else { let mut diag = cx.ecx.struct_span_err( errs.iter().map(|&(sp, _)| sp).collect::>(), - "multiple unused formatting arguments" + "multiple unused formatting arguments", ); diag.span_label(cx.fmtsp, "multiple missing formatting arguments"); diag diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index 2d49c36c06de1..4f5f37132e821 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -25,34 +25,34 @@ LL | format!("{} {}"); | ^^ ^^ error: invalid reference to positional argument 1 (there is 1 argument) - --> $DIR/ifmt-bad-arg.rs:26:14 + --> $DIR/ifmt-bad-arg.rs:26:18 | LL | format!("{0} {1}", 1); - | ^^^ ^^^ + | ^^^ | = note: positional arguments are zero-based error: invalid reference to positional argument 2 (there are 2 arguments) - --> $DIR/ifmt-bad-arg.rs:29:14 + --> $DIR/ifmt-bad-arg.rs:29:22 | LL | format!("{0} {1} {2}", 1, 2); - | ^^^ ^^^ ^^^ + | ^^^ | = note: positional arguments are zero-based error: invalid reference to positional argument 2 (there are 2 arguments) - --> $DIR/ifmt-bad-arg.rs:32:14 + --> $DIR/ifmt-bad-arg.rs:32:28 | LL | format!("{} {value} {} {}", 1, value=2); - | ^^ ^^^^^^^ ^^ ^^ + | ^^ | = note: positional arguments are zero-based error: invalid reference to positional arguments 3, 4 and 5 (there are 3 arguments) - --> $DIR/ifmt-bad-arg.rs:34:14 + --> $DIR/ifmt-bad-arg.rs:34:38 | LL | format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2); - | ^^^^^^ ^^^^^^^ ^^ ^^ ^^ ^^ ^^ ^^ + | ^^ ^^ ^^ | = note: positional arguments are zero-based From c55a698943c515c8028522360e42b12bbf4fb274 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 23 Jul 2018 15:33:36 -0700 Subject: [PATCH 04/11] Only point at inside of string literals if they're actually string literals --- src/libsyntax_ext/format.rs | 30 +++++++++++++++---- .../ui/macros/macro-backtrace-println.stderr | 4 +-- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index a320b52fb7bd9..3d22178eab05b 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -117,6 +117,8 @@ struct Context<'a, 'b: 'a> { invalid_refs: Vec<(usize, usize)>, /// Spans of all the formatting arguments, in order. arg_spans: Vec, + /// Wether this formatting string is a literal or it comes from a macro. + is_literal: bool, } /// Parses the arguments from the given list of tokens, returning None @@ -276,7 +278,11 @@ impl<'a, 'b> Context<'a, 'b> { /// format string. fn report_invalid_references(&self, numbered_position_args: bool) { let mut e; - let sp = MultiSpan::from_spans(self.arg_spans.clone()); + let sp = if self.is_literal { + MultiSpan::from_spans(self.arg_spans.clone()) + } else { + MultiSpan::from_span(self.fmtsp) + }; let mut refs: Vec<_> = self .invalid_refs .iter() @@ -294,7 +300,7 @@ impl<'a, 'b> Context<'a, 'b> { ), ); } else { - let (arg_list, sp) = match refs.len() { + let (arg_list, mut sp) = match refs.len() { 1 => { let (reg, pos) = refs.pop().unwrap(); ( @@ -317,11 +323,14 @@ impl<'a, 'b> Context<'a, 'b> { ) } }; + if !self.is_literal { + sp = MultiSpan::from_span(self.fmtsp); + } e = self.ecx.mut_span_err(sp, &format!("invalid reference to positional {} ({})", - arg_list, - self.describe_num_args())); + arg_list, + self.describe_num_args())); e.note("positional arguments are zero-based"); }; @@ -370,7 +379,11 @@ impl<'a, 'b> Context<'a, 'b> { Some(e) => *e, None => { let msg = format!("there is no argument named `{}`", name); - let sp = *self.arg_spans.get(self.curpiece).unwrap_or(&self.fmtsp); + let sp = if self.is_literal { + *self.arg_spans.get(self.curpiece).unwrap_or(&self.fmtsp) + } else { + self.fmtsp + }; let mut err = self.ecx.struct_span_err(sp, &msg[..]); err.emit(); return; @@ -721,7 +734,7 @@ pub fn expand_format_args<'cx>(ecx: &'cx mut ExtCtxt, pub fn expand_format_args_nl<'cx>( ecx: &'cx mut ExtCtxt, - mut sp: Span, + mut sp: Span, tts: &[tokenstream::TokenTree], ) -> Box { //if !ecx.ecfg.enable_allow_internal_unstable() { @@ -784,6 +797,10 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, return DummyResult::raw_expr(sp); } }; + let is_literal = match ecx.codemap().span_to_snippet(fmt_sp) { + Ok(ref s) if s.starts_with("\"") || s.starts_with("r#") => true, + _ => false, + }; let mut cx = Context { ecx, @@ -806,6 +823,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, fmtsp: fmt.span, invalid_refs: Vec::new(), arg_spans: Vec::new(), + is_literal, }; let fmt_str = &*fmt.node.0.as_str(); diff --git a/src/test/ui/macros/macro-backtrace-println.stderr b/src/test/ui/macros/macro-backtrace-println.stderr index f0ca576f652eb..8f2eb0173a499 100644 --- a/src/test/ui/macros/macro-backtrace-println.stderr +++ b/src/test/ui/macros/macro-backtrace-println.stderr @@ -1,8 +1,8 @@ error: 1 positional argument in format string, but no arguments were given - --> $DIR/macro-backtrace-println.rs:24:31 + --> $DIR/macro-backtrace-println.rs:24:30 | LL | ($fmt:expr) => (myprint!(concat!($fmt, "/n"))); //~ ERROR no arguments were given - | ^^ + | ^^^^^^^^^^^^^^^^^^^ ... LL | myprintln!("{}"); | ----------------- in this macro invocation From bde2be0b1c91489bc590650613fac63854568176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 23 Jul 2018 15:43:34 -0700 Subject: [PATCH 05/11] Add test for raw string --- src/test/ui/ifmt-bad-arg.rs | 7 +++++++ src/test/ui/ifmt-bad-arg.stderr | 8 +++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/test/ui/ifmt-bad-arg.rs b/src/test/ui/ifmt-bad-arg.rs index afe9bc152a36c..bbd75f30d6cb1 100644 --- a/src/test/ui/ifmt-bad-arg.rs +++ b/src/test/ui/ifmt-bad-arg.rs @@ -64,4 +64,11 @@ fn main() { format!("foo }"); //~ ERROR: unmatched `}` found format!("foo %s baz", "bar"); //~ ERROR: argument never used + + format!(r##" + + {foo} + + "##); + //~^^^ ERROR: there is no argument named `foo` } diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index 4f5f37132e821..b02aa765d09a3 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -183,5 +183,11 @@ LL | format!("foo %s baz", "bar"); //~ ERROR: argument never used = help: `%s` should be written as `{}` = note: printf formatting not supported; see the documentation for `std::fmt` -error: aborting due to 26 previous errors +error: there is no argument named `foo` + --> $DIR/ifmt-bad-arg.rs:70:9 + | +LL | {foo} + | ^^^^^ + +error: aborting due to 27 previous errors From f487e39e91e7f57637243674e14e6fa48cc2e193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 09:46:41 -0700 Subject: [PATCH 06/11] Add documentation for `Parser::arg_places` --- src/libfmt_macros/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libfmt_macros/lib.rs b/src/libfmt_macros/lib.rs index f6dcebf8c50f5..30a3bbdc58e98 100644 --- a/src/libfmt_macros/lib.rs +++ b/src/libfmt_macros/lib.rs @@ -154,6 +154,7 @@ pub struct Parser<'a> { style: Option, /// How many newlines have been seen in the string so far, to adjust the error spans seen_newlines: usize, + /// Start and end byte offset of every successfuly parsed argument pub arg_places: Vec<(usize, usize)>, } From f9e37625e6e9d90ee8b7200313de3915e2fc15a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 09:51:04 -0700 Subject: [PATCH 07/11] Reword missing formatting arguments label --- src/libsyntax_ext/format.rs | 2 +- src/test/ui/ifmt-bad-arg.stderr | 4 ++-- src/test/ui/macros/format-foreign.stderr | 2 +- src/test/ui/macros/format-unused-lables.stderr | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index 3d22178eab05b..023fe77cb3c12 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -928,7 +928,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, errs.iter().map(|&(sp, _)| sp).collect::>(), "multiple unused formatting arguments", ); - diag.span_label(cx.fmtsp, "multiple missing formatting arguments"); + diag.span_label(cx.fmtsp, "multiple missing formatting specifiers"); diag } }; diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index b02aa765d09a3..92c44cf406be7 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -80,7 +80,7 @@ error: multiple unused formatting arguments LL | format!("", 1, 2); //~ ERROR: multiple unused formatting arguments | -- ^ ^ | | - | multiple missing formatting arguments + | multiple missing formatting specifiers error: argument never used --> $DIR/ifmt-bad-arg.rs:43:22 @@ -118,7 +118,7 @@ error: multiple unused formatting arguments LL | format!("{} {}", 1, 2, foo=1, bar=2); //~ ERROR: multiple unused formatting arguments | ------- ^ ^ | | - | multiple missing formatting arguments + | multiple missing formatting specifiers error: duplicate argument named `foo` --> $DIR/ifmt-bad-arg.rs:50:33 diff --git a/src/test/ui/macros/format-foreign.stderr b/src/test/ui/macros/format-foreign.stderr index 401b2f6d67e39..83ff301dc19cf 100644 --- a/src/test/ui/macros/format-foreign.stderr +++ b/src/test/ui/macros/format-foreign.stderr @@ -4,7 +4,7 @@ error: multiple unused formatting arguments LL | println!("%.*3$s %s!/n", "Hello,", "World", 4); //~ ERROR multiple unused formatting arguments | -------------- ^^^^^^^^ ^^^^^^^ ^ | | - | multiple missing formatting arguments + | multiple missing formatting specifiers | = help: `%.*3$s` should be written as `{:.2$}` = help: `%s` should be written as `{}` diff --git a/src/test/ui/macros/format-unused-lables.stderr b/src/test/ui/macros/format-unused-lables.stderr index f764190438f33..0b024facc8e38 100644 --- a/src/test/ui/macros/format-unused-lables.stderr +++ b/src/test/ui/macros/format-unused-lables.stderr @@ -4,13 +4,13 @@ error: multiple unused formatting arguments LL | println!("Test", 123, 456, 789); | ------ ^^^ ^^^ ^^^ | | - | multiple missing formatting arguments + | multiple missing formatting specifiers error: multiple unused formatting arguments --> $DIR/format-unused-lables.rs:16:9 | LL | println!("Test2", - | ------- multiple missing formatting arguments + | ------- multiple missing formatting specifiers LL | 123, //~ ERROR multiple unused formatting arguments | ^^^ LL | 456, @@ -28,7 +28,7 @@ error: multiple unused formatting arguments --> $DIR/format-unused-lables.rs:24:9 | LL | println!("Some more $STUFF", - | ------------------ multiple missing formatting arguments + | ------------------ multiple missing formatting specifiers LL | "woo!", //~ ERROR multiple unused formatting arguments | ^^^^^^ LL | STUFF= From 4d8aa5989c3ecbcee9da63fdcf4564f71ac328fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 16:01:38 -0700 Subject: [PATCH 08/11] Use suggestions for `printf` format --- src/libsyntax_ext/format.rs | 17 +++++++- src/libsyntax_ext/format_foreign.rs | 54 ++++++++++++++++++++++-- src/test/ui/ifmt-bad-arg.stderr | 5 ++- src/test/ui/macros/format-foreign.rs | 5 +++ src/test/ui/macros/format-foreign.stderr | 35 ++++++++++++--- 5 files changed, 102 insertions(+), 14 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index 023fe77cb3c12..ad05db91770a3 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -948,6 +948,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, ($kind:ident) => {{ let mut show_doc_note = false; + let mut suggestions = vec![]; for sub in foreign::$kind::iter_subs(fmt_str) { let trn = match sub.translate() { Some(trn) => trn, @@ -956,6 +957,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, None => continue, }; + let pos = sub.position(); let sub = String::from(sub.as_str()); if explained.contains(&sub) { continue; @@ -967,7 +969,14 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, show_doc_note = true; } - diag.help(&format!("`{}` should be written as `{}`", sub, trn)); + if let Some((start, end)) = pos { + // account for `"` and account for raw strings `r#` + let padding = str_style.map(|i| i + 2).unwrap_or(1); + let sp = fmt_sp.from_inner_byte_pos(start + padding, end + padding); + suggestions.push((sp, trn)); + } else { + diag.help(&format!("`{}` should be written as `{}`", sub, trn)); + } } if show_doc_note { @@ -976,6 +985,12 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, " formatting not supported; see the documentation for `std::fmt`", )); } + if suggestions.len() > 0 { + diag.multipart_suggestion( + "format specifiers in Rust are written using `{}`", + suggestions, + ); + } }}; } diff --git a/src/libsyntax_ext/format_foreign.rs b/src/libsyntax_ext/format_foreign.rs index ff9663cdd3cc5..115f51c5e813a 100644 --- a/src/libsyntax_ext/format_foreign.rs +++ b/src/libsyntax_ext/format_foreign.rs @@ -14,7 +14,7 @@ pub mod printf { /// Represents a single `printf`-style substitution. #[derive(Clone, PartialEq, Debug)] pub enum Substitution<'a> { - /// A formatted output substitution. + /// A formatted output substitution with its internal byte offset. Format(Format<'a>), /// A literal `%%` escape. Escape, @@ -28,6 +28,23 @@ pub mod printf { } } + pub fn position(&self) -> Option<(usize, usize)> { + match *self { + Substitution::Format(ref fmt) => Some(fmt.position), + _ => None, + } + } + + pub fn set_position(&mut self, start: usize, end: usize) { + match self { + Substitution::Format(ref mut fmt) => { + fmt.position = (start, end); + } + _ => {} + } + } + + /// Translate this substitution into an equivalent Rust formatting directive. /// /// This ignores cases where the substitution does not have an exact equivalent, or where @@ -57,6 +74,8 @@ pub mod printf { pub length: Option<&'a str>, /// Type of parameter being converted. pub type_: &'a str, + /// Byte offset for the start and end of this formatting directive. + pub position: (usize, usize), } impl<'a> Format<'a> { @@ -257,19 +276,28 @@ pub mod printf { pub fn iter_subs(s: &str) -> Substitutions { Substitutions { s, + pos: 0, } } /// Iterator over substitutions in a string. pub struct Substitutions<'a> { s: &'a str, + pos: usize, } impl<'a> Iterator for Substitutions<'a> { type Item = Substitution<'a>; fn next(&mut self) -> Option { - let (sub, tail) = parse_next_substitution(self.s)?; + let (mut sub, tail) = parse_next_substitution(self.s)?; self.s = tail; + match sub { + Substitution::Format(_) => if let Some((start, end)) = sub.position() { + sub.set_position(start + self.pos, end + self.pos); + self.pos += end; + } + Substitution::Escape => self.pos += 2, + } Some(sub) } @@ -301,7 +329,9 @@ pub mod printf { _ => {/* fall-through */}, } - Cur::new_at_start(&s[start..]) + //let _ = Cur::new_at_start_with_pos(&s[..], start); + //Cur::new_at_start(&s[start..]) + Cur::new_at_start_with_pos(&s[..], start) }; // This is meant to be a translation of the following regex: @@ -355,6 +385,7 @@ pub mod printf { precision: None, length: None, type_: at.slice_between(next).unwrap(), + position: (start.at, next.at), }), next.slice_after() )); @@ -541,6 +572,7 @@ pub mod printf { drop(next); end = at; + let position = (start.at, end.at); let f = Format { span: start.slice_between(end).unwrap(), @@ -550,6 +582,7 @@ pub mod printf { precision, length, type_, + position, }; Some((Substitution::Format(f), end.slice_after())) } @@ -755,6 +788,12 @@ pub mod shell { } } + pub fn position(&self) -> Option<(usize, usize)> { + match *self { + _ => None, + } + } + pub fn translate(&self) -> Option { match *self { Substitution::Ordinal(n) => Some(format!("{{{}}}", n)), @@ -918,7 +957,7 @@ mod strcursor { pub struct StrCursor<'a> { s: &'a str, - at: usize, + pub at: usize, } impl<'a> StrCursor<'a> { @@ -929,6 +968,13 @@ mod strcursor { } } + pub fn new_at_start_with_pos(s: &'a str, at: usize) -> StrCursor<'a> { + StrCursor { + s, + at, + } + } + pub fn at_next_cp(mut self) -> Option> { match self.try_seek_right_cp() { true => Some(self), diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index 92c44cf406be7..a126998355e0c 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -178,9 +178,10 @@ error: argument never used --> $DIR/ifmt-bad-arg.rs:66:27 | LL | format!("foo %s baz", "bar"); //~ ERROR: argument never used - | ^^^^^ + | -- ^^^^^ + | | + | help: format specifiers in Rust are written using `{}`: `{}` | - = help: `%s` should be written as `{}` = note: printf formatting not supported; see the documentation for `std::fmt` error: there is no argument named `foo` diff --git a/src/test/ui/macros/format-foreign.rs b/src/test/ui/macros/format-foreign.rs index ec0eaed43aea6..33401424c9ada 100644 --- a/src/test/ui/macros/format-foreign.rs +++ b/src/test/ui/macros/format-foreign.rs @@ -11,6 +11,11 @@ fn main() { println!("%.*3$s %s!\n", "Hello,", "World", 4); //~ ERROR multiple unused formatting arguments println!("%1$*2$.*3$f", 123.456); //~ ERROR never used + println!(r###"%.*3$s + %s!\n +"###, "Hello,", "World", 4); + //~^ ERROR multiple unused formatting arguments + // correctly account for raw strings in inline suggestions // This should *not* produce hints, on the basis that there's equally as // many "correct" format specifiers. It's *probably* just an actual typo. diff --git a/src/test/ui/macros/format-foreign.stderr b/src/test/ui/macros/format-foreign.stderr index 83ff301dc19cf..93e68183b140e 100644 --- a/src/test/ui/macros/format-foreign.stderr +++ b/src/test/ui/macros/format-foreign.stderr @@ -6,27 +6,48 @@ LL | println!("%.*3$s %s!/n", "Hello,", "World", 4); //~ ERROR multiple unus | | | multiple missing formatting specifiers | - = help: `%.*3$s` should be written as `{:.2$}` - = help: `%s` should be written as `{}` = note: printf formatting not supported; see the documentation for `std::fmt` +help: format specifiers in Rust are written using `{}` + | +LL | println!("{:.2$} {}!/n", "Hello,", "World", 4); //~ ERROR multiple unused formatting arguments + | ^^^^^^ ^^ error: argument never used --> $DIR/format-foreign.rs:13:29 | LL | println!("%1$*2$.*3$f", 123.456); //~ ERROR never used - | ^^^^^^^ + | ----------- ^^^^^^^ + | | + | help: format specifiers in Rust are written using `{}`: `{0:1$.2$}` + | + = note: printf formatting not supported; see the documentation for `std::fmt` + +error: multiple unused formatting arguments + --> $DIR/format-foreign.rs:16:7 + | +LL | println!(r###"%.*3$s + | ______________- +LL | | %s!/n +LL | | "###, "Hello,", "World", 4); + | | - ^^^^^^^^ ^^^^^^^ ^ + | |____| + | multiple missing formatting specifiers | - = help: `%1$*2$.*3$f` should be written as `{0:1$.2$}` = note: printf formatting not supported; see the documentation for `std::fmt` +help: format specifiers in Rust are written using `{}` + | +LL | println!(r###"{:.2$} +LL | {}!/n + | error: argument never used - --> $DIR/format-foreign.rs:17:30 + --> $DIR/format-foreign.rs:22:30 | LL | println!("{} %f", "one", 2.0); //~ ERROR never used | ^^^ error: named argument never used - --> $DIR/format-foreign.rs:19:39 + --> $DIR/format-foreign.rs:24:39 | LL | println!("Hi there, $NAME.", NAME="Tim"); //~ ERROR never used | ^^^^^ @@ -34,5 +55,5 @@ LL | println!("Hi there, $NAME.", NAME="Tim"); //~ ERROR never used = help: `$NAME` should be written as `{NAME}` = note: shell formatting not supported; see the documentation for `std::fmt` -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors From 3298b9f8c74cd859c6b6a3bb6f41c022f5605393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 18:44:34 -0700 Subject: [PATCH 09/11] Fix unittest --- src/libsyntax_ext/format_foreign.rs | 50 +++++++++++++++-------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/libsyntax_ext/format_foreign.rs b/src/libsyntax_ext/format_foreign.rs index 115f51c5e813a..136aedeb71d06 100644 --- a/src/libsyntax_ext/format_foreign.rs +++ b/src/libsyntax_ext/format_foreign.rs @@ -649,6 +649,7 @@ pub mod printf { ($in_:expr, { $param:expr, $flags:expr, $width:expr, $prec:expr, $len:expr, $type_:expr, + $pos:expr, }) => { assert_eq!( pns(concat!($in_, "!")), @@ -661,6 +662,7 @@ pub mod printf { precision: $prec, length: $len, type_: $type_, + position: $pos, }), "!" )) @@ -669,53 +671,53 @@ pub mod printf { } assert_pns_eq_sub!("%!", - { None, "", None, None, None, "!", }); + { None, "", None, None, None, "!", (0, 2), }); assert_pns_eq_sub!("%c", - { None, "", None, None, None, "c", }); + { None, "", None, None, None, "c", (0, 2), }); assert_pns_eq_sub!("%s", - { None, "", None, None, None, "s", }); + { None, "", None, None, None, "s", (0, 2), }); assert_pns_eq_sub!("%06d", - { None, "0", Some(N::Num(6)), None, None, "d", }); + { None, "0", Some(N::Num(6)), None, None, "d", (0, 4), }); assert_pns_eq_sub!("%4.2f", - { None, "", Some(N::Num(4)), Some(N::Num(2)), None, "f", }); + { None, "", Some(N::Num(4)), Some(N::Num(2)), None, "f", (0, 5), }); assert_pns_eq_sub!("%#x", - { None, "#", None, None, None, "x", }); + { None, "#", None, None, None, "x", (0, 3), }); assert_pns_eq_sub!("%-10s", - { None, "-", Some(N::Num(10)), None, None, "s", }); + { None, "-", Some(N::Num(10)), None, None, "s", (0, 5), }); assert_pns_eq_sub!("%*s", - { None, "", Some(N::Next), None, None, "s", }); + { None, "", Some(N::Next), None, None, "s", (0, 3), }); assert_pns_eq_sub!("%-10.*s", - { None, "-", Some(N::Num(10)), Some(N::Next), None, "s", }); + { None, "-", Some(N::Num(10)), Some(N::Next), None, "s", (0, 7), }); assert_pns_eq_sub!("%-*.*s", - { None, "-", Some(N::Next), Some(N::Next), None, "s", }); + { None, "-", Some(N::Next), Some(N::Next), None, "s", (0, 6), }); assert_pns_eq_sub!("%.6i", - { None, "", None, Some(N::Num(6)), None, "i", }); + { None, "", None, Some(N::Num(6)), None, "i", (0, 4), }); assert_pns_eq_sub!("%+i", - { None, "+", None, None, None, "i", }); + { None, "+", None, None, None, "i", (0, 3), }); assert_pns_eq_sub!("%08X", - { None, "0", Some(N::Num(8)), None, None, "X", }); + { None, "0", Some(N::Num(8)), None, None, "X", (0, 4), }); assert_pns_eq_sub!("%lu", - { None, "", None, None, Some("l"), "u", }); + { None, "", None, None, Some("l"), "u", (0, 3), }); assert_pns_eq_sub!("%Iu", - { None, "", None, None, Some("I"), "u", }); + { None, "", None, None, Some("I"), "u", (0, 3), }); assert_pns_eq_sub!("%I32u", - { None, "", None, None, Some("I32"), "u", }); + { None, "", None, None, Some("I32"), "u", (0, 5), }); assert_pns_eq_sub!("%I64u", - { None, "", None, None, Some("I64"), "u", }); + { None, "", None, None, Some("I64"), "u", (0, 5), }); assert_pns_eq_sub!("%'d", - { None, "'", None, None, None, "d", }); + { None, "'", None, None, None, "d", (0, 3), }); assert_pns_eq_sub!("%10s", - { None, "", Some(N::Num(10)), None, None, "s", }); + { None, "", Some(N::Num(10)), None, None, "s", (0, 4), }); assert_pns_eq_sub!("%-10.10s", - { None, "-", Some(N::Num(10)), Some(N::Num(10)), None, "s", }); + { None, "-", Some(N::Num(10)), Some(N::Num(10)), None, "s", (0, 8), }); assert_pns_eq_sub!("%1$d", - { Some(1), "", None, None, None, "d", }); + { Some(1), "", None, None, None, "d", (0, 4), }); assert_pns_eq_sub!("%2$.*3$d", - { Some(2), "", None, Some(N::Arg(3)), None, "d", }); + { Some(2), "", None, Some(N::Arg(3)), None, "d", (0, 8), }); assert_pns_eq_sub!("%1$*2$.*3$d", - { Some(1), "", Some(N::Arg(2)), Some(N::Arg(3)), None, "d", }); + { Some(1), "", Some(N::Arg(2)), Some(N::Arg(3)), None, "d", (0, 11), }); assert_pns_eq_sub!("%-8ld", - { None, "-", Some(N::Num(8)), None, Some("l"), "d", }); + { None, "-", Some(N::Num(8)), None, Some("l"), "d", (0, 5), }); } #[test] From 7bd94e0738dfaa91fe9fc16085969a7c9c3f8129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 20:37:38 -0700 Subject: [PATCH 10/11] Rename method and remove commented out code --- src/libsyntax_ext/format_foreign.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libsyntax_ext/format_foreign.rs b/src/libsyntax_ext/format_foreign.rs index 136aedeb71d06..1613f193351fb 100644 --- a/src/libsyntax_ext/format_foreign.rs +++ b/src/libsyntax_ext/format_foreign.rs @@ -329,9 +329,7 @@ pub mod printf { _ => {/* fall-through */}, } - //let _ = Cur::new_at_start_with_pos(&s[..], start); - //Cur::new_at_start(&s[start..]) - Cur::new_at_start_with_pos(&s[..], start) + Cur::new_at(&s[..], start) }; // This is meant to be a translation of the following regex: @@ -970,7 +968,7 @@ mod strcursor { } } - pub fn new_at_start_with_pos(s: &'a str, at: usize) -> StrCursor<'a> { + pub fn new_at(s: &'a str, at: usize) -> StrCursor<'a> { StrCursor { s, at, From 9a893cc2b82ac6259aead1319758404b80b8a959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Jul 2018 20:46:22 -0700 Subject: [PATCH 11/11] Add span label for format str missing specifier --- src/libsyntax_ext/format.rs | 19 +++++++----- src/test/ui/ifmt-bad-arg.stderr | 30 ++++++++++++++----- src/test/ui/macros/format-foreign.stderr | 10 ++++--- .../ui/macros/format-unused-lables.stderr | 4 ++- 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index ad05db91770a3..98de3d80b1e1f 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -915,12 +915,13 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, errs.push((cx.args[i].span, msg)); } } - if errs.len() > 0 { - let args_used = cx.arg_types.len() - errs.len(); - let args_unused = errs.len(); + let errs_len = errs.len(); + if errs_len > 0 { + let args_used = cx.arg_types.len() - errs_len; + let args_unused = errs_len; let mut diag = { - if errs.len() == 1 { + if errs_len == 1 { let (sp, msg) = errs.into_iter().next().unwrap(); cx.ecx.struct_span_err(sp, msg) } else { @@ -933,6 +934,8 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } }; + // Used to ensure we only report translations for *one* kind of foreign format. + let mut found_foreign = false; // Decide if we want to look for foreign formatting directives. if args_used < args_unused { use super::format_foreign as foreign; @@ -941,9 +944,6 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, // with `%d should be written as {}` over and over again. let mut explained = HashSet::new(); - // Used to ensure we only report translations for *one* kind of foreign format. - let mut found_foreign = false; - macro_rules! check_foreign { ($kind:ident) => {{ let mut show_doc_note = false; @@ -987,7 +987,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } if suggestions.len() > 0 { diag.multipart_suggestion( - "format specifiers in Rust are written using `{}`", + "format specifiers use curly braces", suggestions, ); } @@ -999,6 +999,9 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, check_foreign!(shell); } } + if !found_foreign && errs_len == 1 { + diag.span_label(cx.fmtsp, "formatting specifier missing"); + } diag.emit(); } diff --git a/src/test/ui/ifmt-bad-arg.stderr b/src/test/ui/ifmt-bad-arg.stderr index a126998355e0c..c8fd8bad19ba5 100644 --- a/src/test/ui/ifmt-bad-arg.stderr +++ b/src/test/ui/ifmt-bad-arg.stderr @@ -16,7 +16,9 @@ error: argument never used --> $DIR/ifmt-bad-arg.rs:19:20 | LL | format!("{1}", 1); - | ^ + | ----- ^ + | | + | formatting specifier missing error: 2 positional arguments in format string, but no arguments were given --> $DIR/ifmt-bad-arg.rs:23:14 @@ -86,31 +88,41 @@ error: argument never used --> $DIR/ifmt-bad-arg.rs:43:22 | LL | format!("{}", 1, 2); //~ ERROR: argument never used - | ^ + | ---- ^ + | | + | formatting specifier missing error: argument never used --> $DIR/ifmt-bad-arg.rs:44:20 | LL | format!("{1}", 1, 2); //~ ERROR: argument never used - | ^ + | ----- ^ + | | + | formatting specifier missing error: named argument never used --> $DIR/ifmt-bad-arg.rs:45:26 | LL | format!("{}", 1, foo=2); //~ ERROR: named argument never used - | ^ + | ---- ^ + | | + | formatting specifier missing error: argument never used --> $DIR/ifmt-bad-arg.rs:46:22 | LL | format!("{foo}", 1, foo=2); //~ ERROR: argument never used - | ^ + | ------- ^ + | | + | formatting specifier missing error: named argument never used --> $DIR/ifmt-bad-arg.rs:47:21 | LL | format!("", foo=2); //~ ERROR: named argument never used - | ^ + | -- ^ + | | + | formatting specifier missing error: multiple unused formatting arguments --> $DIR/ifmt-bad-arg.rs:48:32 @@ -148,7 +160,9 @@ error: named argument never used --> $DIR/ifmt-bad-arg.rs:55:51 | LL | format!("{valuea} {valueb}", valuea=5, valuec=7); - | ^ + | ------------------- ^ + | | + | formatting specifier missing error: invalid format string: expected `'}'` but string was terminated --> $DIR/ifmt-bad-arg.rs:61:15 @@ -180,7 +194,7 @@ error: argument never used LL | format!("foo %s baz", "bar"); //~ ERROR: argument never used | -- ^^^^^ | | - | help: format specifiers in Rust are written using `{}`: `{}` + | help: format specifiers use curly braces: `{}` | = note: printf formatting not supported; see the documentation for `std::fmt` diff --git a/src/test/ui/macros/format-foreign.stderr b/src/test/ui/macros/format-foreign.stderr index 93e68183b140e..5e76c0a322e51 100644 --- a/src/test/ui/macros/format-foreign.stderr +++ b/src/test/ui/macros/format-foreign.stderr @@ -7,7 +7,7 @@ LL | println!("%.*3$s %s!/n", "Hello,", "World", 4); //~ ERROR multiple unus | multiple missing formatting specifiers | = note: printf formatting not supported; see the documentation for `std::fmt` -help: format specifiers in Rust are written using `{}` +help: format specifiers use curly braces | LL | println!("{:.2$} {}!/n", "Hello,", "World", 4); //~ ERROR multiple unused formatting arguments | ^^^^^^ ^^ @@ -18,7 +18,7 @@ error: argument never used LL | println!("%1$*2$.*3$f", 123.456); //~ ERROR never used | ----------- ^^^^^^^ | | - | help: format specifiers in Rust are written using `{}`: `{0:1$.2$}` + | help: format specifiers use curly braces: `{0:1$.2$}` | = note: printf formatting not supported; see the documentation for `std::fmt` @@ -34,7 +34,7 @@ LL | | "###, "Hello,", "World", 4); | multiple missing formatting specifiers | = note: printf formatting not supported; see the documentation for `std::fmt` -help: format specifiers in Rust are written using `{}` +help: format specifiers use curly braces | LL | println!(r###"{:.2$} LL | {}!/n @@ -44,7 +44,9 @@ error: argument never used --> $DIR/format-foreign.rs:22:30 | LL | println!("{} %f", "one", 2.0); //~ ERROR never used - | ^^^ + | ------- ^^^ + | | + | formatting specifier missing error: named argument never used --> $DIR/format-foreign.rs:24:39 diff --git a/src/test/ui/macros/format-unused-lables.stderr b/src/test/ui/macros/format-unused-lables.stderr index 0b024facc8e38..81171a1ed01de 100644 --- a/src/test/ui/macros/format-unused-lables.stderr +++ b/src/test/ui/macros/format-unused-lables.stderr @@ -22,7 +22,9 @@ error: named argument never used --> $DIR/format-unused-lables.rs:21:35 | LL | println!("Some stuff", UNUSED="args"); //~ ERROR named argument never used - | ^^^^^^ + | ------------ ^^^^^^ + | | + | formatting specifier missing error: multiple unused formatting arguments --> $DIR/format-unused-lables.rs:24:9