Report parsing error messages with LocatedSpan<&str, Option<&str>>.

The LocatedSpan<T, X = ()> type supports an optional user-defined
`extra: X` field, which we can use to smuggle meaningful custom error
messages out of the parser.

apollo-federation/src/sources/connect/json_selection/helpers.rs

@@ -1,10 +1,10 @@
 use nom::character::complete::multispace0;
-use nom::IResult;
 use nom::Slice;
 use serde_json_bytes::Value as JSON;
 
 use super::location::Span;
 use super::location::WithRange;
+use super::ParseResult;
 
 // This macro is handy for tests, but it absolutely should never be used with
 // dynamic input at runtime, since it panics if the selection string fails to
@@ -26,7 +26,7 @@ macro_rules! selection {
 
 // Consumes any amount of whitespace and/or comments starting with # until the
 // end of the line.
-pub(crate) fn spaces_or_comments(input: Span) -> IResult<Span, WithRange<&str>> {
+pub(crate) fn spaces_or_comments(input: Span) -> ParseResult<WithRange<&str>> {
     let mut suffix = input;
     loop {
         let mut made_progress = false;
@@ -87,11 +87,12 @@ pub(crate) fn vec_push<T>(mut vec: Vec<T>, item: T) -> Vec<T> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::sources::connect::json_selection::location::new_span;
 
     #[test]
     fn test_spaces_or_comments() {
         fn check(input: &str, (exp_remainder, exp_spaces): (&str, &str)) {
-            match spaces_or_comments(Span::new(input)) {
+            match spaces_or_comments(new_span(input)) {
                 Ok((remainder, parsed)) => {
                     assert_eq!(*remainder.fragment(), exp_remainder);
                     assert_eq!(*parsed.as_ref(), exp_spaces);

apollo-federation/src/sources/connect/json_selection/lit_expr.rs

@@ -16,18 +16,19 @@ use nom::multi::many1;
 use nom::sequence::pair;
 use nom::sequence::preceded;
 use nom::sequence::tuple;
-use nom::IResult;
 
 use super::helpers::spaces_or_comments;
 use super::location::merge_ranges;
 use super::location::ranged_span;
 use super::location::Ranged;
 use super::location::Span;
 use super::location::WithRange;
+use super::nom_error_message;
 use super::parser::parse_string_literal;
 use super::parser::Key;
 use super::parser::PathSelection;
 use super::ExternalVarPaths;
+use super::ParseResult;
 
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub(crate) enum LitExpr {
@@ -43,7 +44,7 @@ pub(crate) enum LitExpr {
 impl LitExpr {
     // LitExpr      ::= LitPrimitive | LitObject | LitArray | PathSelection
     // LitPrimitive ::= LitString | LitNumber | "true" | "false" | "null"
-    pub(crate) fn parse(input: Span) -> IResult<Span, WithRange<Self>> {
+    pub(crate) fn parse(input: Span) -> ParseResult<WithRange<Self>> {
         tuple((
             spaces_or_comments,
             alt((
@@ -70,7 +71,7 @@ impl LitExpr {
     }
 
     // LitNumber ::= "-"? ([0-9]+ ("." [0-9]*)? | "." [0-9]+)
-    fn parse_number(input: Span) -> IResult<Span, WithRange<Self>> {
+    fn parse_number(input: Span) -> ParseResult<WithRange<Self>> {
         let (suffix, (_, neg, _, num)) = tuple((
             spaces_or_comments,
             opt(ranged_span("-")),
@@ -148,15 +149,17 @@ impl LitExpr {
             let range = merge_ranges(neg.and_then(|n| n.range()), num.range());
             Ok((suffix, WithRange::new(lit_number, range)))
         } else {
-            Err(nom::Err::Failure(nom::error::Error::new(
+            Err(nom_error_message(
                 input,
-                nom::error::ErrorKind::IsNot,
-            )))
+                // We could include the faulty number in the error message, but
+                // it will also appear at the beginning of the input span.
+                "Failed to parse numeric literal",
+            ))
         }
     }
 
     // LitObject ::= "{" (LitProperty ("," LitProperty)* ","?)? "}"
-    fn parse_object(input: Span) -> IResult<Span, WithRange<Self>> {
+    fn parse_object(input: Span) -> ParseResult<WithRange<Self>> {
         tuple((
             spaces_or_comments,
             ranged_span("{"),
@@ -191,13 +194,13 @@ impl LitExpr {
     }
 
     // LitProperty ::= Key ":" LitExpr
-    fn parse_property(input: Span) -> IResult<Span, (WithRange<Key>, WithRange<Self>)> {
+    fn parse_property(input: Span) -> ParseResult<(WithRange<Key>, WithRange<Self>)> {
         tuple((Key::parse, spaces_or_comments, char(':'), Self::parse))(input)
             .map(|(input, (key, _, _colon, value))| (input, (key, value)))
     }
 
     // LitArray ::= "[" (LitExpr ("," LitExpr)* ","?)? "]"
-    fn parse_array(input: Span) -> IResult<Span, WithRange<Self>> {
+    fn parse_array(input: Span) -> ParseResult<WithRange<Self>> {
         tuple((
             spaces_or_comments,
             ranged_span("["),
@@ -272,10 +275,11 @@ mod tests {
     use super::super::location::strip_ranges::StripRanges;
     use super::*;
     use crate::sources::connect::json_selection::helpers::span_is_all_spaces_or_comments;
+    use crate::sources::connect::json_selection::location::new_span;
     use crate::sources::connect::json_selection::PathList;
 
     fn check_parse(input: &str, expected: LitExpr) {
-        match LitExpr::parse(Span::new(input)) {
+        match LitExpr::parse(new_span(input)) {
             Ok((remainder, parsed)) => {
                 assert!(span_is_all_spaces_or_comments(remainder));
                 assert_eq!(parsed.strip_ranges(), WithRange::new(expected, None));

apollo-federation/src/sources/connect/json_selection/location.rs

@@ -1,9 +1,27 @@
 use nom::bytes::complete::tag;
 use nom::combinator::map;
-use nom::IResult;
 use nom_locate::LocatedSpan;
 
-pub(crate) type Span<'a> = LocatedSpan<&'a str>;
+use super::ParseResult;
+
+// Currently, all our error messages are &'static str, which allows the Span
+// type to remain Copy, which is convenient to avoid having to clone Spans
+// frequently in the parser code.
+//
+// If we wanted to introduce any error messages computed using format!, we'd
+// have to switch to Option<String> here (or some other type containing owned
+// String data), which would make Span no longer Copy, requiring more cloning.
+// Not the end of the world, but something to keep in mind for the future.
+//
+// The cloning would still be relatively cheap because we use None throughout
+// parsing and then only set Some(message) when we need to report an error, so
+// we would not be cloning long String messages very often (and the rest of the
+// Span fields are cheap to clone).
+pub(crate) type Span<'a> = LocatedSpan<&'a str, Option<&'static str>>;
+
+pub(super) fn new_span(input: &str) -> Span {
+    Span::new_extra(input, None)
+}
 
 // Some parsed AST structures, like PathSelection and NamedSelection, can
 // produce a range directly from their children, so they do not need to be
@@ -108,7 +126,7 @@ pub(super) fn merge_ranges(left: OffsetRange, right: OffsetRange) -> OffsetRange
 // matched string and the range of the match.
 pub(super) fn ranged_span<'a, 'b>(
     s: &'a str,
-) -> impl FnMut(Span<'b>) -> IResult<Span<'b>, WithRange<&'b str>> + 'a
+) -> impl FnMut(Span<'b>) -> ParseResult<WithRange<&'b str>> + 'a
 where
     'b: 'a,
 {