Repository: pest-parser/pest (language: rust)

Issue:
Integer underflow in Error::fmt
Trying to display an error created from a `Span((8, 11), (9, 1))` results in an error.

This is because when trying to reverse the underline, `start` gets set to 0 before the line `let offset = start - 1` is executed.

https://github.com/pest-parser/pest/blob/49847de8f41badc14c353c8f221a92fc5e376ee8/pest/src/error.rs#L266-L280

This occurs if the span points to the first char of the new line, which happens when `\n` is the last matched char. I think the actual span that should be underlined in that case is the one ending one char earlier, on the previous line.


Discussion / hints:
Thanks for reporting this. Underlining the character before makes a lot of sense. This code there is quite sensitive and has a lot of edge cases. I had hoped that it was decently well-tested, but that doesn't seem to be the case.

Fix patch:
diff --git a/pest/src/error.rs b/pest/src/error.rs
index 07c9f3e8..81cedd1a 100644
--- a/pest/src/error.rs
+++ b/pest/src/error.rs
@@ -100,7 +100,7 @@ impl<R: RuleType> Error<R> {
             variant,
             location: InputLocation::Pos(pos.pos()),
             path: None,
-            line: pos.line_of().to_owned(),
+            line: visualize_whitespace(pos.line_of()),
             continued_line: None,
             line_col: LineColLocation::Pos(pos.line_col()),
         }
@@ -136,19 +136,28 @@ impl<R: RuleType> Error<R> {
     /// ```
     #[allow(clippy::needless_pass_by_value)]
     pub fn new_from_span(variant: ErrorVariant<R>, span: Span) -> Error<R> {
-        let continued_line = if span.start_pos().line_col().0 != span.end_pos().line_col().0 {
-            Some(span.end_pos().line_of().to_owned())
-        } else {
-            None
+        let end = span.end_pos();
+
+        let mut end_line_col = end.line_col();
+        // end position is after a \n, so we want to point to the visual lf symbol
+        if end_line_col.1 == 1 {
+            let mut visual_end = end.clone();
+            visual_end.skip_back(1);
+            let lc = visual_end.line_col();
+            end_line_col = (lc.0, lc.1 + 1);
         };
 
+        let mut line_iter = span.lines();
+        let start_line = visualize_whitespace(line_iter.next().unwrap_or(""));
+        let continued_line = line_iter.last().map(visualize_whitespace);
+
         Error {
             variant,
-            location: InputLocation::Span((span.start(), span.end())),
+            location: InputLocation::Span((span.start(), end.pos())),
             path: None,
-            line: span.start_pos().line_of().to_owned(),
+            line: start_line,
             continued_line,
-            line_col: LineColLocation::Span(span.start_pos().line_col(), span.end_pos().line_col()),
+            line_col: LineColLocation::Span(span.start_pos().line_col(), end_line_col),
         }
     }
 
@@ -433,6 +442,10 @@ impl<'i, R: RuleType> error::Error for Error<R> {
     }
 }
 
+fn visualize_whitespace(input: &str) -> String {
+    input.to_owned().replace('\r', "␍").replace('\n', "␊")
+}
+
 #[cfg(test)]
 mod tests {
     use super::super::position;
@@ -455,7 +468,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = unexpected 4, 5, or 6; expected 1, 2, or 3",
@@ -481,7 +494,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = expected 1 or 2",
@@ -507,7 +520,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = unexpected 4, 5, or 6",
@@ -533,7 +546,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = unknown parsing error",
@@ -558,7 +571,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = error: big one",
@@ -584,7 +597,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "3 | efgh",
                 "  |  ^^",
                 "  |",
@@ -611,7 +624,7 @@ mod tests {
             vec![
                 " --> 1:2",
                 "  |",
-                "1 | ab",
+                "1 | ab␊",
                 "  | ...",
                 "3 | efgh",
                 "  |  ^^",
@@ -639,7 +652,7 @@ mod tests {
             vec![
                 " --> 1:6",
                 "  |",
-                "1 | abcdef",
+                "1 | abcdef␊",
                 "2 | gh",
                 "  | ^----^",
                 "  |",
@@ -649,6 +662,64 @@ mod tests {
         );
     }
 
+    #[test]
+    fn display_custom_span_end_after_newline() {
+        let input = "abcdef\n";
+        let start = position::Position::new(input, 0).unwrap();
+        let end = position::Position::new(input, 7).unwrap();
+        assert!(start.at_start());
+        assert!(end.at_end());
+
+        let error: Error<u32> = Error::new_from_span(
+            ErrorVariant::CustomError {
+                message: "error: big one".to_owned(),
+            },
+            start.span(&end),
+        );
+
+        assert_eq!(
+            format!("{}", error),
+            vec![
+                " --> 1:1",
+                "  |",
+                "1 | abcdef␊",
+                "  | ^-----^",
+                "  |",
+                "  = error: big one",
+            ]
+            .join("\n")
+        );
+    }
+
+    #[test]
+    fn display_custom_span_empty() {
+        let input = "";
+        let start = position::Position::new(input, 0).unwrap();
+        let end = position::Position::new(input, 0).unwrap();
+        assert!(start.at_start());
+        assert!(end.at_end());
+
+        let error: Error<u32> = Error::new_from_span(
+            ErrorVariant::CustomError {
+                message: "error: empty".to_owned(),
+            },
+            start.span(&end),
+        );
+
+        assert_eq!(
+            format!("{}", error),
+            vec![
+                " --> 1:1",
+                "  |",
+                "1 | ",
+                "  | ^",
+                "  |",
+                "  = error: empty",
+            ]
+            .join("\n")
+        );
+    }
+
     #[test]
     fn mapped_parsing_error() {
         let input = "ab\ncd\nef";
@@ -667,7 +738,7 @@ mod tests {
             vec![
                 " --> 2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = unexpected 5, 6, or 7; expected 2, 3, or 4",
@@ -694,7 +765,7 @@ mod tests {
             vec![
                 " --> file.rs:2:2",
                 "  |",
-                "2 | cd",
+                "2 | cd␊",
                 "  |  ^---",
                 "  |",
                 "  = unexpected 4, 5, or 6; expected 1, 2, or 3",
diff --git a/pest/src/lib.rs b/pest/src/lib.rs
index ce158a32..a0ff75d7 100644
--- a/pest/src/lib.rs
+++ b/pest/src/lib.rs
@@ -64,7 +64,7 @@ extern crate ucd_trie;
 pub use parser::Parser;
 pub use parser_state::{state, Atomicity, Lookahead, MatchDir, ParseResult, ParserState};
 pub use position::Position;
-pub use span::Span;
+pub use span::{Lines, Span};
 use std::fmt::Debug;
 use std::hash::Hash;
 pub use token::Token;
diff --git a/pest/src/position.rs b/pest/src/position.rs
index 8d79619d..76e3383b 100644
--- a/pest/src/position.rs
+++ b/pest/src/position.rs
@@ -194,40 +194,36 @@ impl<'i> Position<'i> {
     /// assert_eq!(result.unwrap().position().line_of(), "a");
     /// ```
     #[inline]
-    pub fn line_of(&self) -> &str {
+    pub fn line_of(&self) -> &'i str {
         if self.pos > self.input.len() {
             panic!("position out of bounds");
-        }
+        };
+        // Safe since start and end can only be valid UTF-8 borders.
+        &self.input[self.find_line_start()..self.find_line_end()]
+    }
 
-        let start = if self.pos == 0 {
-            0
-        } else {
-            // Position's pos is always a UTF-8 border.
-            let start = self
-                .input
-                .char_indices()
-                .rev()
-                .skip_while(|&(i, _)| i >= self.pos)
-                .find(|&(_, c)| c == '\n');
-            match start {
-                Some((i, _)) => i + 1,
-                None => 0,
-            }
+    pub(crate) fn find_line_start(&self) -> usize {
+        if self.input.is_empty() {
+            return 0;
         };
-        let bytes = self.input.as_bytes();
-        let end = if bytes.is_empty() {
+        // Position's pos is always a UTF-8 border.
+        let start = self
+            .input
+            .char_indices()
+            .rev()
+            .skip_while(|&(i, _)| i >= self.pos)
+            .find(|&(_, c)| c == '\n');
+        match start {
+            Some((i, _)) => i + 1,
+            None => 0,
+        }
+    }
+
+    pub(crate) fn find_line_end(&self) -> usize {
+        if self.input.is_empty() {
             0
         } else if self.pos == self.input.len() - 1 {
-            let mut end = self.input.len();
-
-            if end > 0 && bytes[end - 1] == b'\n' {
-                end -= 1;
-            }
-            if end > 0 && bytes[end - 1] == b'\r' {
-                end -= 1;
-            }
-
-            end
+            self.input.len()
         } else {
             // Position's pos is always a UTF-8 border.
             let end = self
@@ -235,20 +231,11 @@ impl<'i> Position<'i> {
                 .char_indices()
                 .skip_while(|&(i, _)| i < self.pos)
                 .find(|&(_, c)| c == '\n');
-            let mut end = match end {
-                Some((i, _)) => i,
+            match end {
+                Some((i, _)) => i + 1,
                 None => self.input.len(),
-            };
-
-            if end > 0 && bytes[end - 1] == b'\r' {
-                end -= 1;
             }
-
-            end
-        };
-
-        // Safe since start and end can only be valid UTF-8 borders.
-        &self.input[start..end]
+        }
     }
 
     /// Returns `true` when the `Position` points to the start of the input `&str`.
@@ -285,6 +272,28 @@ impl<'i> Position<'i> {
         true
     }
 
+    /// Goes back `n` `char`s from the `Position` and returns `true` if the skip was possible or `false`
+    /// otherwise. If the return value is `false`, `pos` will not be updated.
+    #[inline]
+    pub(crate) fn skip_back(&mut self, n: usize) -> bool {
+        let skipped = {
+            let mut len = 0;
+            // Position's pos is always a UTF-8 border.
+            let mut chars = (&self.input[..self.pos]).chars().rev();
+            for _ in 0..n {
+                if let Some(c) = chars.next() {
+                    len += c.len_utf8();
+                } else {
+                    return false;
+                }
+            }
+            len
+        };
+
+        self.pos -= skipped;
+        true
+    }
+
     /// Skips until one of the given `strings` is found. If none of the `strings` can be found,
     /// this function will return `false` but its `pos` will *still* be updated.
     #[inline]
@@ -457,13 +466,13 @@ mod tests {
     fn line_of() {
         let input = "a\rb\nc\r\nd嗨";
 
-        assert_eq!(Position::new(input, 0).unwrap().line_of(), "a\rb");
-        assert_eq!(Position::new(input, 1).unwrap().line_of(), "a\rb");
-        assert_eq!(Position::new(input, 2).unwrap().line_of(), "a\rb");
-        assert_eq!(Position::new(input, 3).unwrap().line_of(), "a\rb");
-        assert_eq!(Position::new(input, 4).unwrap().line_of(), "c");
-        assert_eq!(Position::new(input, 5).unwrap().line_of(), "c");
-        assert_eq!(Position::new(input, 6).unwrap().line_of(), "c");
+        assert_eq!(Position::new(input, 0).unwrap().line_of(), "a\rb\n");
+        assert_eq!(Position::new(input, 1).unwrap().line_of(), "a\rb\n");
+        assert_eq!(Position::new(input, 2).unwrap().line_of(), "a\rb\n");
+        assert_eq!(Position::new(input, 3).unwrap().line_of(), "a\rb\n");
+        assert_eq!(Position::new(input, 4).unwrap().line_of(), "c\r\n");
+        assert_eq!(Position::new(input, 5).unwrap().line_of(), "c\r\n");
+        assert_eq!(Position::new(input, 6).unwrap().line_of(), "c\r\n");
         assert_eq!(Position::new(input, 7).unwrap().line_of(), "d嗨");
         assert_eq!(Position::new(input, 8).unwrap().line_of(), "d嗨");
         assert_eq!(Position::new(input, 11).unwrap().line_of(), "d嗨");
@@ -480,14 +489,14 @@ mod tests {
     fn line_of_new_line() {
         let input = "\n";
 
-        assert_eq!(Position::new(input, 0).unwrap().line_of(), "");
+        assert_eq!(Position::new(input, 0).unwrap().line_of(), "\n");
     }
 
     #[test]
     fn line_of_between_new_line() {
         let input = "\n\n";
 
-        assert_eq!(Position::new(input, 1).unwrap().line_of(), "");
+        assert_eq!(Position::new(input, 1).unwrap().line_of(), "\n");
     }
 
     fn measure_skip(input: &str, pos: usize, n: usize) -> Option<usize> {
diff --git a/pest/src/span.rs b/pest/src/span.rs
index 323ee245..f8beffe2 100644
--- a/pest/src/span.rs
+++ b/pest/src/span.rs
@@ -180,6 +180,31 @@ impl<'i> Span<'i> {
         // Span's start and end positions are always a UTF-8 borders.
         &self.input[self.start..self.end]
     }
+
+    /// Iterates over all lines (partially) covered by this span.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use pest;
+    /// # #[allow(non_camel_case_types)]
+    /// # #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+    /// enum Rule {}
+    ///
+    /// let input = "a\nb\nc";
+    /// let mut state: Box<pest::ParserState<Rule>> = pest::ParserState::new(input).skip(2).unwrap();
+    /// let start_pos = state.position().clone();
+    /// state = state.match_string("b\nc").unwrap();
+    /// let span = start_pos.span(&state.position().clone());
+    /// assert_eq!(span.lines().collect::<Vec<_>>(), vec!["b\n", "c"]);
+    /// ```
+    #[inline]
+    pub fn lines(&self) -> Lines {
+        Lines {
+            span: self,
+            pos: self.start,
+        }
+    }
 }
 
 impl<'i> fmt::Debug for Span<'i> {
@@ -208,6 +233,32 @@ impl<'i> Hash for Span<'i> {
     }
 }
 
+/// Line iterator for Spans, created by [`Span::lines()`].
+///
+/// Iterates all lines that are at least partially covered by the span.
+///
+/// [`Span::lines()`]: struct.Span.html#method.lines
+pub struct Lines<'i> {
+    span: &'i Span<'i>,
+    pos: usize,
+}
+
+impl<'i> Iterator for Lines<'i> {
+    type Item = &'i str;
+    fn next(&mut self) -> Option<&'i str> {
+        if self.pos > self.span.end {
+            return None;
+        }
+        let pos = position::Position::new(self.span.input, self.pos)?;
+        if pos.at_end() {
+            return None;
+        }
+        let line = pos.line_of();
+        self.pos = pos.find_line_end();
+        Some(line)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -224,4 +275,27 @@ mod tests {
 
         assert_eq!(span.split(), (start, end));
     }
+
+    #[test]
+    fn lines_mid() {
+        let input = "abc\ndef\nghi";
+        let span = Span::new(input, 1, 7).unwrap();
+        let lines: Vec<_> = span.lines().collect();
+        println!("{:?}", lines);
+        assert_eq!(lines.len(), 2);
+        assert_eq!(lines[0], "abc\n".to_owned());
+        assert_eq!(lines[1], "def\n".to_owned());
+    }
+
+    #[test]
+    fn lines_eof() {
+        let input = "abc\ndef\nghi";
+        let span = Span::new(input, 5, 11).unwrap();
+        assert!(span.end_pos().at_end());
+        let lines: Vec<_> = span.lines().collect();
+        println!("{:?}", lines);
+        assert_eq!(lines.len(), 2);
+        assert_eq!(lines[0], "def\n".to_owned());
+        assert_eq!(lines[1], "ghi".to_owned());
+    }
 }


Test patch: