Improve format string errors

- Point at format string position inside the formatting string
 - Explain that argument names can't start with an underscore

src/libfmt_macros/lib.rs

@@ -127,6 +127,14 @@ pub enum Count<'a> {
     CountImplied,
 }
 
+pub struct ParseError {
+    pub description: string::String,
+    pub note: Option<string::String>,
+    pub label: string::String,
+    pub start: usize,
+    pub end: usize,
+}
+
 /// The parser structure for interpreting the input format string. This is
 /// modeled as an iterator over `Piece` structures to form a stream of tokens
 /// being output.
@@ -137,7 +145,7 @@ pub struct Parser<'a> {
     input: &'a str,
     cur: iter::Peekable<str::CharIndices<'a>>,
     /// Error messages accumulated during parsing
-    pub errors: Vec<(string::String, Option<string::String>)>,
+    pub errors: Vec<ParseError>,
     /// Current position of implicit positional argument pointer
     curarg: usize,
 }
@@ -160,12 +168,17 @@ impl<'a> Iterator for Parser<'a> {
                 }
                 '}' => {
                     self.cur.next();
+                    let pos = pos + 1;
                     if self.consume('}') {
-                        Some(String(self.string(pos + 1)))
+                        Some(String(self.string(pos)))
                     } else {
-                        self.err_with_note("unmatched `}` found",
-                                           "if you intended to print `}`, \
-                                           you can escape it using `}}`");
+                        self.err_with_note(
+                            "unmatched `}` found",
+                            "unmatched `}`",
+                            "if you intended to print `}`, you can escape it using `}}`",
+                            pos,
+                            pos,
+                        );
                         None
                     }
                 }
@@ -191,15 +204,40 @@ impl<'a> Parser<'a> {
     /// Notifies of an error. The message doesn't actually need to be of type
     /// String, but I think it does when this eventually uses conditions so it
     /// might as well start using it now.
-    fn err(&mut self, msg: &str) {
-        self.errors.push((msg.to_owned(), None));
+    fn err<S1: Into<string::String>, S2: Into<string::String>>(
+        &mut self,
+        description: S1,
+        label: S2,
+        start: usize,
+        end: usize,
+    ) {
+        self.errors.push(ParseError {
+            description: description.into(),
+            note: None,
+            label: label.into(),
+            start,
+            end,
+        });
     }
 
     /// Notifies of an error. The message doesn't actually need to be of type
     /// String, but I think it does when this eventually uses conditions so it
     /// might as well start using it now.
-    fn err_with_note(&mut self, msg: &str, note: &str) {
-        self.errors.push((msg.to_owned(), Some(note.to_owned())));
+    fn err_with_note<S1: Into<string::String>, S2: Into<string::String>, S3: Into<string::String>>(
+        &mut self,
+        description: S1,
+        label: S2,
+        note: S3,
+        start: usize,
+        end: usize,
+    ) {
+        self.errors.push(ParseError {
+            description: description.into(),
+            note: Some(note.into()),
+            label: label.into(),
+            start,
+            end,
+        });
     }
 
     /// Optionally consumes the specified character. If the character is not at
@@ -222,19 +260,26 @@ impl<'a> Parser<'a> {
     /// found, an error is emitted.
     fn must_consume(&mut self, c: char) {
         self.ws();
-        if let Some(&(_, maybe)) = self.cur.peek() {
+        if let Some(&(pos, maybe)) = self.cur.peek() {
             if c == maybe {
                 self.cur.next();
             } else {
-                self.err(&format!("expected `{:?}`, found `{:?}`", c, maybe));
+                self.err(format!("expected `{:?}`, found `{:?}`", c, maybe),
+                         format!("expected `{}`", c),
+                         pos + 1,
+                         pos + 1);
             }
         } else {
-            let msg = &format!("expected `{:?}` but string was terminated", c);
+            let msg = format!("expected `{:?}` but string was terminated", c);
+            let pos = self.input.len() + 1; // point at closing `"`
             if c == '}' {
                 self.err_with_note(msg,
-                                   "if you intended to print `{`, you can escape it using `{{`");
+                                   format!("expected `{:?}`", c),
+                                   "if you intended to print `{`, you can escape it using `{{`",
+                                   pos,
+                                   pos);
             } else {
-                self.err(msg);
+                self.err(msg, format!("expected `{:?}`", c), pos, pos);
             }
         }
     }
@@ -300,6 +345,15 @@ impl<'a> Parser<'a> {
         } else {
             match self.cur.peek() {
                 Some(&(_, c)) if c.is_alphabetic() => Some(ArgumentNamed(self.word())),
+                Some(&(pos, c)) if c == '_' => {
+                    let invalid_name = self.string(pos);
+                    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());
+                    Some(ArgumentNamed(invalid_name))
+                },
 
                 // This is an `ArgumentNext`.
                 // Record the fact and do the resolution after parsing the

src/libsyntax_ext/format.rs

@@ -767,9 +767,12 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
     }
 
     if !parser.errors.is_empty() {
-        let (err, note) = parser.errors.remove(0);
-        let mut e = cx.ecx.struct_span_err(cx.fmtsp, &format!("invalid format string: {}", err));
-        if let Some(note) = note {
+        let err = parser.errors.remove(0);
+        let sp = cx.fmtsp.from_inner_byte_pos(err.start, err.end);
+        let mut e = cx.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(&note);
         }
         e.emit();

src/libsyntax_pos/lib.rs

@@ -427,6 +427,13 @@ impl Span {
         )
     }
 
+    pub fn from_inner_byte_pos(self, start: usize, end: usize) -> Span {
+        let span = self.data();
+        Span::new(span.lo + BytePos::from_usize(start),
+                  span.lo + BytePos::from_usize(end),
+                  span.ctxt)
+    }
+
     #[inline]
     pub fn apply_mark(self, mark: Mark) -> Span {
         let span = self.data();

src/test/ui/fmt/format-string-error.rs

@@ -12,5 +12,14 @@ fn main() {
     println!("{");
     println!("{{}}");
     println!("}");
+    let _ = format!("{_foo}", _foo = 6usize);
+    //~^ ERROR invalid format string: invalid argument name `_foo`
+    let _ = format!("{_}", _ = 6usize);
+    //~^ ERROR invalid format string: invalid argument name `_`
+    let _ = format!("{");
+    //~^ ERROR invalid format string: expected `'}'` but string was terminated
+    let _ = format!("}");
+    //~^ ERROR invalid format string: unmatched `}` found
+    let _ = format!("{\\}");
+    //~^ ERROR invalid format string: expected `'}'`, found `'\\'`
 }
-

src/test/ui/fmt/format-string-error.stderr

@@ -2,7 +2,7 @@ error: invalid format string: expected `'}'` but string was terminated
   --> $DIR/format-string-error.rs:12:5
    |
 LL |     println!("{");
-   |     ^^^^^^^^^^^^^^
+   |     ^^^^^^^^^^^^^^ expected `'}'` in format string
    |
    = note: if you intended to print `{`, you can escape it using `{{`
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
@@ -11,10 +11,48 @@ error: invalid format string: unmatched `}` found
   --> $DIR/format-string-error.rs:14:5
    |
 LL |     println!("}");
-   |     ^^^^^^^^^^^^^^
+   |     ^^^^^^^^^^^^^^ unmatched `}` in format string
    |
    = note: if you intended to print `}`, you can escape it using `}}`
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
 
-error: aborting due to 2 previous errors
+error: invalid format string: invalid argument name `_foo`
+  --> $DIR/format-string-error.rs:15:23
+   |
+LL |     let _ = format!("{_foo}", _foo = 6usize);
+   |                       ^^^^ invalid argument name in format string
+   |
+   = note: argument names cannot start with an underscore
+
+error: invalid format string: invalid argument name `_`
+  --> $DIR/format-string-error.rs:17:23
+   |
+LL |     let _ = format!("{_}", _ = 6usize);
+   |                       ^ invalid argument name in format string
+   |
+   = note: argument names cannot start with an underscore
+
+error: invalid format string: expected `'}'` but string was terminated
+  --> $DIR/format-string-error.rs:19:23
+   |
+LL |     let _ = format!("{");
+   |                       ^ expected `'}'` in format string
+   |
+   = note: if you intended to print `{`, you can escape it using `{{`
+
+error: invalid format string: unmatched `}` found
+  --> $DIR/format-string-error.rs:21:22
+   |
+LL |     let _ = format!("}");
+   |                      ^ unmatched `}` in format string
+   |
+   = note: if you intended to print `}`, you can escape it using `}}`
+
+error: invalid format string: expected `'}'`, found `'/'`
+  --> $DIR/format-string-error.rs:23:23
+   |
+LL |     let _ = format!("{/}");
+   |                       ^ expected `}` in format string
+
+error: aborting due to 7 previous errors