[nextest-filtering] better error reporting for invalid escapes

Previously we'd report something like

  error: expected close parenthesis
   ╭────
 1 │ package(nexte\$)
   ·              ▲
   ·              ╰── missing `)`
   ╰────

  error: expected end of expression
   ╭────
 1 │ package(nexte\$)
   ·              ─┬─
   ·               ╰── unparsed input
   ╰────

Now, we report:

  error: invalid escape character
   ╭────
 1 │ package(nexte\$)
   ·              ─┬
   ·               ╰── invalid escape character
   ╰────

nextest-filtering/src/errors.rs

@@ -69,6 +69,10 @@ pub enum ParseSingleError {
     #[error("expected close parenthesis")]
     ExpectedCloseParenthesis(#[label("missing `)`")] SourceSpan),
 
+    /// An invalid escape character was found.
+    #[error("invalid escape character")]
+    InvalidEscapeCharacter(#[label("invalid escape character")] SourceSpan),
+
     /// An expression was expected in this position but not found.
     #[error("expected expression")]
     ExpectedExpr(#[label("missing expression")] SourceSpan),

nextest-filtering/src/parsing.rs

@@ -212,6 +212,7 @@ impl ExprResult {
 enum SpanLength {
     Unknown,
     Exact(usize),
+    Offset(isize, usize),
 }
 
 fn expect_inner<'a, F, T>(
@@ -226,11 +227,21 @@ where
         Ok((remaining, out)) => Ok((remaining, Some(out))),
         Err(nom::Err::Error(err)) | Err(nom::Err::Failure(err)) => {
             let nom::error::Error { input, .. } = err;
-            let start = input.location_offset();
-            let len = input.fragment().len();
+            let fragment_start = input.location_offset();
+            let fragment_length = input.fragment().len();
             let span = match limit {
-                SpanLength::Unknown => (start, len).into(),
-                SpanLength::Exact(x) => (start, x.min(len)).into(),
+                SpanLength::Unknown => (fragment_start, fragment_length).into(),
+                SpanLength::Exact(x) => (fragment_start, x.min(fragment_length)).into(),
+                SpanLength::Offset(offset, x) => {
+                    // e.g. fragment_start = 5, fragment_length = 2, offset = -1, x = 3.
+                    // Here, start = 4.
+                    let effective_start = fragment_start.saturating_add_signed(offset);
+                    // end = 6.
+                    let effective_end = effective_start + fragment_length;
+                    // len = min(3, 6 - 4) = 2.
+                    let len = (effective_end - effective_start).min(x);
+                    (effective_start, len).into()
+                }
             };
             let err = make_err(span);
             input.extra.report_error(err);
@@ -250,6 +261,17 @@ where
     expect_inner(parser, make_err, SpanLength::Unknown)
 }
 
+fn expect_n<'a, F, T>(
+    parser: F,
+    make_err: fn(SourceSpan) -> ParseSingleError,
+    limit: SpanLength,
+) -> impl FnMut(Span<'a>) -> IResult<'a, Option<T>>
+where
+    F: FnMut(Span<'a>) -> IResult<T>,
+{
+    expect_inner(parser, make_err, limit)
+}
+
 fn expect_char<'a>(
     c: char,
     make_err: fn(SourceSpan) -> ParseSingleError,
@@ -305,7 +327,7 @@ fn parse_matcher_text(input: Span) -> IResult<Option<String>> {
         ParseSingleError::InvalidString,
     )(input.clone())
     {
-        Ok((i, res)) => (i, res),
+        Ok((i, res)) => (i, res.flatten()),
         Err(nom::Err::Incomplete(_)) => {
             let i = input.slice(input.fragment().len()..);
             // No need for error reporting, missing closing ')' will be detected after
@@ -319,6 +341,7 @@ fn parse_matcher_text(input: Span) -> IResult<Option<String>> {
         i.extra
             .report_error(ParseSingleError::InvalidString((start..0).into()));
     }
+
     Ok((i, res))
 }
 
@@ -1227,9 +1250,15 @@ mod tests {
     }
 
     macro_rules! assert_error {
-        ($error:ident, $name:ident, $start:literal, $end:literal) => {
-            assert!(matches!($error, ParseSingleError::$name(span) if span == ($start, $end).into()));
-        };
+        ($error:ident, $name:ident, $start:literal, $end:literal) => {{
+            let matches = matches!($error, ParseSingleError::$name(span) if span == ($start, $end).into());
+            assert!(
+                matches,
+                "expected: {:?}, actual: error: {:?}",
+                ParseSingleError::$name(($start, $end).into()),
+                $error,
+            );
+        }};
     }
 
     #[test]
@@ -1261,6 +1290,23 @@ mod tests {
         assert_error!(error, ExpectedCloseParenthesis, 3, 0);
     }
 
+    #[test]
+    fn test_invalid_escapes() {
+        let src = r"package(foobar\$\#\@baz)";
+        let mut errors = parse_err(src);
+        assert_eq!(3, errors.len());
+
+        // Ensure all three errors are reported.
+        let error = errors.remove(0);
+        assert_error!(error, InvalidEscapeCharacter, 14, 2);
+
+        let error = errors.remove(0);
+        assert_error!(error, InvalidEscapeCharacter, 16, 2);
+
+        let error = errors.remove(0);
+        assert_error!(error, InvalidEscapeCharacter, 18, 2);
+    }
+
     #[test]
     fn test_invalid_regex() {
         let src = "package(/)/)";

nextest-filtering/src/parsing/unicode_string.rs

@@ -3,7 +3,8 @@
 
 // Adapted from https://github.com/Geal/nom/blob/294ffb3d9e0ade2c3b7ddfff52484b6d643dcce1/examples/string.rs
 
-use super::{IResult, Span};
+use super::{expect_n, IResult, Span, SpanLength};
+use crate::errors::ParseSingleError;
 use nom::{
     branch::alt,
     bytes::streaming::{is_not, take_while_m_n},
@@ -56,21 +57,28 @@ fn parse_unicode(input: Span) -> IResult<char> {
 }
 
 #[tracable_parser]
-fn parse_escaped_char(input: Span) -> IResult<char> {
+fn parse_escaped_char(input: Span) -> IResult<Option<char>> {
+    let valid = alt((
+        parse_unicode,
+        value('\n', char('n')),
+        value('\r', char('r')),
+        value('\t', char('t')),
+        value('\u{08}', char('b')),
+        value('\u{0C}', char('f')),
+        value('\\', char('\\')),
+        value('/', char('/')),
+        value(')', char(')')),
+        value(',', char(',')),
+    ));
     preceded(
         char('\\'),
-        alt((
-            parse_unicode,
-            value('\n', char('n')),
-            value('\r', char('r')),
-            value('\t', char('t')),
-            value('\u{08}', char('b')),
-            value('\u{0C}', char('f')),
-            value('\\', char('\\')),
-            value('/', char('/')),
-            value(')', char(')')),
-            value(',', char(',')),
-        )),
+        // If none of the valid characters are found, this will report an error.
+        expect_n(
+            valid,
+            ParseSingleError::InvalidEscapeCharacter,
+            // -1 to account for the preceding backslash.
+            SpanLength::Offset(-1, 2),
+        ),
     )(input)
 }
 
@@ -105,25 +113,44 @@ enum StringFragment<'a> {
 }
 
 #[tracable_parser]
-fn parse_fragment(input: Span) -> IResult<StringFragment<'_>> {
+fn parse_fragment(input: Span) -> IResult<Option<StringFragment<'_>>> {
     alt((
         map(parse_literal, |span| {
-            StringFragment::Literal(span.fragment())
+            Some(StringFragment::Literal(span.fragment()))
+        }),
+        map(parse_escaped_char, |res| {
+            res.map(StringFragment::EscapedChar)
         }),
-        map(parse_escaped_char, StringFragment::EscapedChar),
     ))(input)
 }
 
-/// Construct a string by consuming the input until the next unescaped `'`
+/// Construct a string by consuming the input until the next unescaped ) or ,.
+///
+/// Returns None if the string isn't valid.
 ///
-/// Return Err(Incomplete(1)) if not ending `'` is found
+/// Returns Err(Incomplete(1)) if an ending delimiter ) or , is not found.
 #[tracable_parser]
-pub(super) fn parse_string(input: Span) -> IResult<String> {
-    fold_many0(parse_fragment, String::new, |mut string, fragment| {
-        match fragment {
-            StringFragment::Literal(s) => string.push_str(s),
-            StringFragment::EscapedChar(c) => string.push(c),
-        }
-        string
-    })(input)
+pub(super) fn parse_string(input: Span) -> IResult<Option<String>> {
+    fold_many0(
+        parse_fragment,
+        || Some(String::new()),
+        |string, fragment| {
+            match (string, fragment) {
+                (Some(mut string), Some(StringFragment::Literal(s))) => {
+                    string.push_str(s);
+                    Some(string)
+                }
+                (Some(mut string), Some(StringFragment::EscapedChar(c))) => {
+                    string.push(c);
+                    Some(string)
+                }
+                (Some(_), None) => {
+                    // We encountered a parsing error, and at this point we'll stop returning
+                    // values.
+                    None
+                }
+                (None, _) => None,
+            }
+        },
+    )(input)
 }