From 862ebc4c38958b8ba5e30dd5eb1203e6109c40a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 20 Dec 2018 22:33:16 -0800 Subject: [PATCH] Various changes to string format diagnostics - Point at opening mismatched formatting brace - Account for differences between raw and regular strings - Account for differences between the code snippet and `InternedString` - Add more tests --- src/libfmt_macros/lib.rs | 144 +++++++++++++------ src/librustc/traits/on_unimplemented.rs | 4 +- src/libsyntax_ext/format.rs | 77 ++++++++-- src/test/ui/fmt/format-string-error-2.rs | 70 +++++++++ src/test/ui/fmt/format-string-error-2.stderr | 137 ++++++++++++++++++ src/test/ui/fmt/format-string-error.rs | 2 +- src/test/ui/fmt/format-string-error.stderr | 29 +++- src/test/ui/if/ifmt-bad-arg.stderr | 8 +- src/test/ui/issues/issue-51848.stderr | 4 +- 9 files changed, 415 insertions(+), 60 deletions(-) create mode 100644 src/test/ui/fmt/format-string-error-2.rs create mode 100644 src/test/ui/fmt/format-string-error-2.stderr diff --git a/src/libfmt_macros/lib.rs b/src/libfmt_macros/lib.rs index 74b55673470..fc8325e5915 100644 --- a/src/libfmt_macros/lib.rs +++ b/src/libfmt_macros/lib.rs @@ -123,8 +123,9 @@ pub struct ParseError { pub description: string::String, pub note: Option, pub label: string::String, - pub start: usize, - pub end: usize, + pub start: SpanIndex, + pub end: SpanIndex, + pub secondary_label: Option<(string::String, SpanIndex, SpanIndex)>, } /// The parser structure for interpreting the input format string. This is @@ -142,22 +143,39 @@ pub struct Parser<'a> { curarg: usize, /// `Some(raw count)` when the string is "raw", used to position spans correctly 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 successfully parsed argument pub arg_places: Vec<(usize, usize)>, + /// Characters that need to be shifted + skips: Vec, + /// Span offset of the last opening brace seen, used for error reporting + last_opening_brace_pos: Option, + /// Wether the source string is comes from `println!` as opposed to `format!` or `print!` + append_newline: bool, +} + +#[derive(Clone, Copy, Debug)] +pub struct SpanIndex(usize); + +impl SpanIndex { + pub fn unwrap(self) -> usize { + self.0 + } } impl<'a> Iterator for Parser<'a> { type Item = Piece<'a>; fn next(&mut self) -> Option> { - let raw = self.style.map(|raw| raw + self.seen_newlines).unwrap_or(0); + let raw = self.raw(); if let Some(&(pos, c)) = self.cur.peek() { match c { '{' => { + let curr_last_brace = self.last_opening_brace_pos; + self.last_opening_brace_pos = Some(self.to_span_index(pos)); self.cur.next(); if self.consume('{') { + self.last_opening_brace_pos = curr_last_brace; + Some(String(self.string(pos + 1))) } else { let arg = self.argument(); @@ -174,7 +192,7 @@ impl<'a> Iterator for Parser<'a> { if self.consume('}') { Some(String(self.string(pos + 1))) } else { - let err_pos = pos + raw + 1; + let err_pos = self.to_span_index(pos); self.err_with_note( "unmatched `}` found", "unmatched `}`", @@ -186,7 +204,6 @@ impl<'a> Iterator for Parser<'a> { } } '\n' => { - self.seen_newlines += 1; Some(String(self.string(pos))) } _ => Some(String(self.string(pos))), @@ -199,15 +216,22 @@ impl<'a> Iterator for Parser<'a> { impl<'a> Parser<'a> { /// Creates a new parser for the given format string - pub fn new(s: &'a str, style: Option) -> Parser<'a> { + pub fn new( + s: &'a str, + style: Option, + skips: Vec, + append_newline: bool, + ) -> Parser<'a> { Parser { input: s, cur: s.char_indices().peekable(), errors: vec![], curarg: 0, style, - seen_newlines: 0, arg_places: vec![], + skips, + last_opening_brace_pos: None, + append_newline, } } @@ -218,8 +242,8 @@ impl<'a> Parser<'a> { &mut self, description: S1, label: S2, - start: usize, - end: usize, + start: SpanIndex, + end: SpanIndex, ) { self.errors.push(ParseError { description: description.into(), @@ -227,6 +251,7 @@ impl<'a> Parser<'a> { label: label.into(), start, end, + secondary_label: None, }); } @@ -238,8 +263,8 @@ impl<'a> Parser<'a> { description: S1, label: S2, note: S3, - start: usize, - end: usize, + start: SpanIndex, + end: SpanIndex, ) { self.errors.push(ParseError { description: description.into(), @@ -247,6 +272,7 @@ impl<'a> Parser<'a> { label: label.into(), start, end, + secondary_label: None, }); } @@ -266,47 +292,86 @@ impl<'a> Parser<'a> { } } + fn raw(&self) -> usize { + self.style.map(|raw| raw + 1).unwrap_or(0) + } + + fn to_span_index(&self, pos: usize) -> SpanIndex { + let mut pos = pos; + for skip in &self.skips { + if pos > *skip { + pos += 1; + } else if pos == *skip && self.raw() == 0 { + pos += 1; + } else { + break; + } + } + SpanIndex(self.raw() + pos + 1) + } + /// Forces consumption of the specified character. If the character is not /// found, an error is emitted. fn must_consume(&mut self, c: char) -> Option { self.ws(); - let raw = self.style.unwrap_or(0); - let padding = raw + self.seen_newlines; if let Some(&(pos, maybe)) = self.cur.peek() { if c == maybe { self.cur.next(); Some(pos) } else { - let pos = pos + raw + 1; - self.err(format!("expected `{:?}`, found `{:?}`", c, maybe), - format!("expected `{}`", c), - pos, - pos); + let pos = self.to_span_index(pos); + let description = format!("expected `'}}'`, found `{:?}`", maybe); + let label = "expected `}`".to_owned(); + let (note, secondary_label) = if c == '}' { + (Some("if you intended to print `{`, you can escape it using `{{`".to_owned()), + self.last_opening_brace_pos.map(|pos| { + ("because of this opening brace".to_owned(), pos, pos) + })) + } else { + (None, None) + }; + self.errors.push(ParseError { + description, + note, + label, + start: pos, + end: pos, + secondary_label, + }); None } } else { - let msg = format!("expected `{:?}` but string was terminated", c); - // point at closing `"`, unless the last char is `\n` to account for `println` - let pos = match self.input.chars().last() { - Some('\n') => self.input.len(), - _ => self.input.len() + 1, - }; + let description = format!("expected `{:?}` but string was terminated", c); + // point at closing `"` + let pos = self.input.len() - if self.append_newline { 1 } else { 0 }; + let pos = self.to_span_index(pos); if c == '}' { - self.err_with_note(msg, - format!("expected `{:?}`", c), - "if you intended to print `{`, you can escape it using `{{`", - pos + padding, - pos + padding); + let label = format!("expected `{:?}`", c); + let (note, secondary_label) = if c == '}' { + (Some("if you intended to print `{`, you can escape it using `{{`".to_owned()), + self.last_opening_brace_pos.map(|pos| { + ("because of this opening brace".to_owned(), pos, pos) + })) + } else { + (None, None) + }; + self.errors.push(ParseError { + description, + note, + label, + start: pos, + end: pos, + secondary_label, + }); } else { - self.err(msg, format!("expected `{:?}`", c), pos, pos); + self.err(description, format!("expected `{:?}`", c), pos, pos); } None } } - /// Consumes all whitespace characters until the first non-whitespace - /// character + /// Consumes all whitespace characters until the first non-whitespace character fn ws(&mut self) { while let Some(&(_, c)) = self.cur.peek() { if c.is_whitespace() { @@ -334,8 +399,7 @@ impl<'a> Parser<'a> { &self.input[start..self.input.len()] } - /// Parses an Argument structure, or what's contained within braces inside - /// the format string + /// Parses an Argument structure, or what's contained within braces inside the format string fn argument(&mut self) -> Argument<'a> { let pos = self.position(); let format = self.format(); @@ -371,8 +435,8 @@ impl<'a> Parser<'a> { self.err_with_note(format!("invalid argument name `{}`", invalid_name), "invalid argument name", "argument names cannot start with an underscore", - pos + 1, // add 1 to account for leading `{` - pos + 1 + invalid_name.len()); + self.to_span_index(pos), + self.to_span_index(pos + invalid_name.len())); Some(ArgumentNamed(invalid_name)) }, @@ -553,7 +617,7 @@ mod tests { use super::*; fn same(fmt: &'static str, p: &[Piece<'static>]) { - let parser = Parser::new(fmt, None); + let parser = Parser::new(fmt, None, vec![], false); assert!(parser.collect::>>() == p); } @@ -569,7 +633,7 @@ mod tests { } fn musterr(s: &str) { - let mut p = Parser::new(s, None); + let mut p = Parser::new(s, None, vec![], false); p.next(); assert!(!p.errors.is_empty()); } diff --git a/src/librustc/traits/on_unimplemented.rs b/src/librustc/traits/on_unimplemented.rs index 68648454c21..4b188d25175 100644 --- a/src/librustc/traits/on_unimplemented.rs +++ b/src/librustc/traits/on_unimplemented.rs @@ -234,7 +234,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString { { let name = tcx.item_name(trait_def_id); let generics = tcx.generics_of(trait_def_id); - let parser = Parser::new(&self.0, None); + let parser = Parser::new(&self.0, None, vec![], false); let mut result = Ok(()); for token in parser { match token { @@ -293,7 +293,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString { }).collect::>(); let empty_string = String::new(); - let parser = Parser::new(&self.0, None); + let parser = Parser::new(&self.0, None, vec![], false); parser.map(|p| match p { Piece::String(s) => s, diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs index e5747214f1b..220765fd8c7 100644 --- a/src/libsyntax_ext/format.rs +++ b/src/libsyntax_ext/format.rs @@ -765,18 +765,74 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, } }; - let is_literal = match ecx.source_map().span_to_snippet(fmt_sp) { - Ok(ref s) if s.starts_with("\"") || s.starts_with("r#") => true, - _ => false, + let (is_literal, fmt_snippet) = match ecx.source_map().span_to_snippet(fmt_sp) { + Ok(s) => (s.starts_with("\"") || s.starts_with("r#"), Some(s)), + _ => (false, None), }; - let fmt_str = &*fmt.node.0.as_str(); let str_style = match fmt.node.1 { ast::StrStyle::Cooked => None, - ast::StrStyle::Raw(raw) => Some(raw as usize), + ast::StrStyle::Raw(raw) => { + Some(raw as usize) + }, }; - let mut parser = parse::Parser::new(fmt_str, str_style); + /// Find the indices of all characters that have been processed and differ between the actual + /// written code (code snippet) and the `InternedString` that get's processed in the `Parser` + /// in order to properly synthethise the intra-string `Span`s for error diagnostics. + fn find_skips(snippet: &str, is_raw: bool) -> Vec { + let mut eat_ws = false; + let mut s = snippet.chars().enumerate().peekable(); + let mut skips = vec![]; + while let Some((pos, c)) = s.next() { + match (c, s.peek()) { + // skip whitespace and empty lines ending in '\\' + ('\\', Some((next_pos, '\n'))) if !is_raw => { + eat_ws = true; + skips.push(pos); + skips.push(*next_pos); + let _ = s.next(); + } + ('\\', Some((next_pos, '\n'))) | + ('\\', Some((next_pos, 'n'))) | + ('\\', Some((next_pos, 't'))) if eat_ws => { + skips.push(pos); + skips.push(*next_pos); + let _ = s.next(); + } + (' ', _) | + ('\n', _) | + ('\t', _) if eat_ws => { + skips.push(pos); + } + ('\\', Some((next_pos, 'n'))) | + ('\\', Some((next_pos, 't'))) | + ('\\', Some((next_pos, '\\'))) | + ('\\', Some((next_pos, '\''))) | + ('\\', Some((next_pos, '\"'))) => { + skips.push(*next_pos); + let _ = s.next(); + } + _ if eat_ws => { // `take_while(|c| c.is_whitespace())` + eat_ws = false; + } + _ => {} + } + } + skips + } + + let skips = if let (true, Some(ref snippet)) = (is_literal, fmt_snippet.as_ref()) { + let r_start = str_style.map(|r| r + 1).unwrap_or(0); + let r_end = str_style.map(|r| r).unwrap_or(0); + let s = &snippet[r_start + 1..snippet.len() - r_end - 1]; + find_skips(s, str_style.is_some()) + } else { + vec![] + }; + + let fmt_str = &*fmt.node.0.as_str(); // for the suggestions below + let mut parser = parse::Parser::new(fmt_str, str_style, skips.clone(), append_newline); let mut unverified_pieces = Vec::new(); while let Some(piece) = parser.next() { @@ -789,12 +845,17 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, if !parser.errors.is_empty() { let err = parser.errors.remove(0); - let sp = fmt.span.from_inner_byte_pos(err.start, err.end); - let mut e = ecx.struct_span_err(sp, &format!("invalid format string: {}", err.description)); + let sp = fmt.span.from_inner_byte_pos(err.start.unwrap(), err.end.unwrap()); + let mut e = ecx.struct_span_err(sp, &format!("invalid format string: {}", + err.description)); e.span_label(sp, err.label + " in format string"); if let Some(note) = err.note { e.note(¬e); } + if let Some((label, start, end)) = err.secondary_label { + let sp = fmt.span.from_inner_byte_pos(start.unwrap(), end.unwrap()); + e.span_label(sp, label); + } e.emit(); return DummyResult::raw_expr(sp); } diff --git a/src/test/ui/fmt/format-string-error-2.rs b/src/test/ui/fmt/format-string-error-2.rs new file mode 100644 index 00000000000..fd6e41ec6fc --- /dev/null +++ b/src/test/ui/fmt/format-string-error-2.rs @@ -0,0 +1,70 @@ +// ignore-tidy-tab + +fn main() { + format!("{ + a"); + //~^ ERROR invalid format string + format!("{ \ + + b"); + //~^ ERROR invalid format string + format!(r#"{ \ + + rawc"#); + //~^^^ ERROR invalid format string + format!(r#"{ \n +\n + rawd"#); + //~^^^ ERROR invalid format string + format!("{ \n +\n + e"); + //~^ ERROR invalid format string + format!(" + { + a"); + //~^ ERROR invalid format string + format!(" + { + a + "); + //~^^ ERROR invalid format string + format!(" \ + { \ + \ + b"); + //~^ ERROR invalid format string + format!(" \ + { \ + \ + b \ + + "); + //~^^^ ERROR invalid format string + format!(r#" +raw { \ + + c"#); + //~^^^ ERROR invalid format string + format!(r#" +raw { \n +\n + d"#); + //~^^^ ERROR invalid format string + format!(" + { \n +\n + e"); + //~^ ERROR invalid format string + + format!(" + {asdf + } + ", asdf=1); + // ok - this is supported + format!(" + { + asdf} + ", asdf=1); + //~^^ ERROR invalid format string +} diff --git a/src/test/ui/fmt/format-string-error-2.stderr b/src/test/ui/fmt/format-string-error-2.stderr new file mode 100644 index 00000000000..0b3f08c1fbe --- /dev/null +++ b/src/test/ui/fmt/format-string-error-2.stderr @@ -0,0 +1,137 @@ +error: invalid format string: expected `'}'`, found `'a'` + --> $DIR/format-string-error-2.rs:5:5 + | +LL | format!("{ + | - because of this opening brace +LL | a"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'b'` + --> $DIR/format-string-error-2.rs:9:5 + | +LL | format!("{ / + | - because of this opening brace +LL | +LL | b"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'/'` + --> $DIR/format-string-error-2.rs:11:18 + | +LL | format!(r#"{ / + | - ^ expected `}` in format string + | | + | because of this opening brace + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'/'` + --> $DIR/format-string-error-2.rs:15:18 + | +LL | format!(r#"{ /n + | - ^ expected `}` in format string + | | + | because of this opening brace + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'e'` + --> $DIR/format-string-error-2.rs:21:5 + | +LL | format!("{ /n + | - because of this opening brace +LL | /n +LL | e"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'a'` + --> $DIR/format-string-error-2.rs:25:5 + | +LL | { + | - because of this opening brace +LL | a"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'a'` + --> $DIR/format-string-error-2.rs:29:5 + | +LL | { + | - because of this opening brace +LL | a + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'b'` + --> $DIR/format-string-error-2.rs:35:5 + | +LL | { / + | - because of this opening brace +LL | / +LL | b"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'b'` + --> $DIR/format-string-error-2.rs:40:5 + | +LL | { / + | - because of this opening brace +LL | / +LL | b / + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'/'` + --> $DIR/format-string-error-2.rs:45:8 + | +LL | raw { / + | - ^ expected `}` in format string + | | + | because of this opening brace + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'/'` + --> $DIR/format-string-error-2.rs:50:8 + | +LL | raw { /n + | - ^ expected `}` in format string + | | + | because of this opening brace + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'e'` + --> $DIR/format-string-error-2.rs:57:5 + | +LL | { /n + | - because of this opening brace +LL | /n +LL | e"); + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: invalid format string: expected `'}'`, found `'a'` + --> $DIR/format-string-error-2.rs:67:5 + | +LL | { + | - because of this opening brace +LL | asdf} + | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` + +error: aborting due to 13 previous errors + diff --git a/src/test/ui/fmt/format-string-error.rs b/src/test/ui/fmt/format-string-error.rs index 6d2837e35b7..cca949aab63 100644 --- a/src/test/ui/fmt/format-string-error.rs +++ b/src/test/ui/fmt/format-string-error.rs @@ -31,7 +31,7 @@ fn main() { { "###); - //~^^ ERROR invalid format string + //~^ ERROR invalid format string let _ = format!(r###" diff --git a/src/test/ui/fmt/format-string-error.stderr b/src/test/ui/fmt/format-string-error.stderr index 4dffec45618..86ab163591e 100644 --- a/src/test/ui/fmt/format-string-error.stderr +++ b/src/test/ui/fmt/format-string-error.stderr @@ -2,7 +2,9 @@ error: invalid format string: expected `'}'` but string was terminated --> $DIR/format-string-error.rs:4:16 | LL | println!("{"); - | ^ expected `'}'` in format string + | -^ expected `'}'` in format string + | | + | because of this opening brace | = note: if you intended to print `{`, you can escape it using `{{` @@ -34,7 +36,9 @@ error: invalid format string: expected `'}'` but string was terminated --> $DIR/format-string-error.rs:13:23 | LL | let _ = format!("{"); - | ^ expected `'}'` in format string + | -^ expected `'}'` in format string + | | + | because of this opening brace | = note: if you intended to print `{`, you can escape it using `{{` @@ -50,13 +54,19 @@ error: invalid format string: expected `'}'`, found `'/'` --> $DIR/format-string-error.rs:17:23 | LL | let _ = format!("{/}"); - | ^ expected `}` in format string + | -^ expected `}` in format string + | | + | because of this opening brace + | + = note: if you intended to print `{`, you can escape it using `{{` error: invalid format string: expected `'}'` but string was terminated - --> $DIR/format-string-error.rs:19:29 + --> $DIR/format-string-error.rs:19:35 | LL | let _ = format!("/n/n/n{/n/n/n"); - | ^ expected `'}'` in format string + | - ^ expected `'}'` in format string + | | + | because of this opening brace | = note: if you intended to print `{`, you can escape it using `{{` @@ -64,14 +74,19 @@ error: invalid format string: expected `'}'` but string was terminated --> $DIR/format-string-error.rs:25:3 | LL | {"###); - | ^ expected `'}'` in format string + | -^ expected `'}'` in format string + | | + | because of this opening brace | = note: if you intended to print `{`, you can escape it using `{{` error: invalid format string: expected `'}'` but string was terminated - --> $DIR/format-string-error.rs:32:1 + --> $DIR/format-string-error.rs:33:1 | +LL | { + | - because of this opening brace LL | +LL | "###); | ^ expected `'}'` in format string | = note: if you intended to print `{`, you can escape it using `{{` diff --git a/src/test/ui/if/ifmt-bad-arg.stderr b/src/test/ui/if/ifmt-bad-arg.stderr index 32a05b7c4e3..3ae8669a7f2 100644 --- a/src/test/ui/if/ifmt-bad-arg.stderr +++ b/src/test/ui/if/ifmt-bad-arg.stderr @@ -168,7 +168,9 @@ error: invalid format string: expected `'}'` but string was terminated --> $DIR/ifmt-bad-arg.rs:51:15 | LL | format!("{"); //~ ERROR: expected `'}'` but string was terminated - | ^ expected `'}'` in format string + | -^ expected `'}'` in format string + | | + | because of this opening brace | = note: if you intended to print `{`, you can escape it using `{{` @@ -207,8 +209,12 @@ LL | {foo} error: invalid format string: expected `'}'`, found `'t'` --> $DIR/ifmt-bad-arg.rs:75:1 | +LL | ninth number: { + | - because of this opening brace LL | tenth number: {}", | ^ expected `}` in format string + | + = note: if you intended to print `{`, you can escape it using `{{` error: aborting due to 28 previous errors diff --git a/src/test/ui/issues/issue-51848.stderr b/src/test/ui/issues/issue-51848.stderr index 62937fc8c3e..1bb6f3c1f1f 100644 --- a/src/test/ui/issues/issue-51848.stderr +++ b/src/test/ui/issues/issue-51848.stderr @@ -2,7 +2,9 @@ error: invalid format string: expected `'}'` but string was terminated --> $DIR/issue-51848.rs:6:20 | LL | println!("{"); //~ ERROR invalid - | ^ expected `'}'` in format string + | -^ expected `'}'` in format string + | | + | because of this opening brace ... LL | macro_with_error!(); | -------------------- in this macro invocation