Improve format string errors

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

1 files changed, 68 insertions, 14 deletions

diff --git a/src/libfmt_macros/lib.rs b/src/libfmt_macros/lib.rs
index a551b1b770a..a77751d65d0 100644
--- a/src/libfmt_macros/lib.rs
+++ b/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