Fixed phantom error generation when mapping errors

zesterer · Dec 31, 2024 · ed345d9 · ed345d9
1 parent 30f62d0
commit ed345d9
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 37 deletions.
diff --git a/src/combinator.rs b/src/combinator.rs
@@ -827,7 +827,7 @@ where
 
         if res.is_err() {
             let alt = inp.take_alt();
-            inp.memos.insert(key, Some(alt));
+            inp.memos.insert(key, alt);
         } else {
             inp.memos.remove(&key);
         }
@@ -2596,18 +2596,9 @@ where
     where
         Self: Sized,
     {
-        let old_alt = inp.take_alt();
-        let res = self.parser.go::<M>(inp);
-
-        if res.is_err() {
-            let mut new_alt = inp.take_alt();
-            new_alt.err = (self.mapper)(new_alt.err);
-
-            inp.errors.alt = Some(old_alt);
-            inp.add_alt_err(&new_alt.pos, new_alt.err);
-        }
-
-        res
+        (&self.parser)
+            .map_err_with_state(|e, _, _| (self.mapper)(e))
+            .go::<M>(inp)
     }
 
     go_extra!(O);
@@ -2648,6 +2639,7 @@ where
 //     go_extra!(O);
 // }
 
+// TODO: Remove combinator, replace with map_err_with
 /// See [`Parser::map_err_with_state`].
 #[derive(Copy, Clone)]
 pub struct MapErrWithState<A, F> {
@@ -2668,13 +2660,17 @@ where
         Self: Sized,
     {
         let start = inp.cursor();
+        let old_alt = inp.take_alt();
         let res = self.parser.go::<M>(inp);
 
         if res.is_err() {
-            let mut e = inp.take_alt();
+            // Can't fail!
+            let mut new_alt = inp.take_alt().unwrap();
             let span = inp.span_since(&start);
-            e.err = (self.mapper)(e.err, span, inp.state());
-            inp.errors.alt = Some(e);
+            new_alt.err = (self.mapper)(new_alt.err, span, inp.state());
+
+            inp.errors.alt = old_alt;
+            inp.add_alt_err(&new_alt.pos, new_alt.err);
         }
 
         res

diff --git a/src/input.rs b/src/input.rs
@@ -1524,7 +1524,8 @@ impl<'src, 'parse, I: Input<'src>, E: ParserExtra<'src, I>> InputRef<'src, 'pars
     pub fn parse<O, P: Parser<'src, I, O, E>>(&mut self, parser: P) -> Result<O, E::Error> {
         match parser.go::<Emit>(self) {
             Ok(out) => Ok(out),
-            Err(()) => Err(self.take_alt().err),
+            // Can't fail!
+            Err(()) => Err(self.take_alt().unwrap().err),
         }
     }
 
@@ -1536,7 +1537,8 @@ impl<'src, 'parse, I: Input<'src>, E: ParserExtra<'src, I>> InputRef<'src, 'pars
     pub fn check<O, P: Parser<'src, I, O, E>>(&mut self, parser: P) -> Result<(), E::Error> {
         match parser.go::<Check>(self) {
             Ok(()) => Ok(()),
-            Err(()) => Err(self.take_alt().err),
+            // Can't fail!
+            Err(()) => Err(self.take_alt().unwrap().err),
         }
     }
 
@@ -1729,9 +1731,7 @@ impl<'src, 'parse, I: Input<'src>, E: ParserExtra<'src, I>> InputRef<'src, 'pars
 
         // Prioritize errors before choosing whether to generate the alt (avoids unnecessary error creation)
         self.errors.alt = Some(match self.errors.alt.take() {
-            Some(alt) => match {
-                I::cursor_location(&alt.pos).cmp(&I::cursor_location(at))
-            } {
+            Some(alt) => match { I::cursor_location(&alt.pos).cmp(&I::cursor_location(at)) } {
                 Ordering::Equal => {
                     Located::at(alt.pos, alt.err.merge_expected_found(expected, found, span))
                 }
@@ -1762,15 +1762,9 @@ impl<'src, 'parse, I: Input<'src>, E: ParserExtra<'src, I>> InputRef<'src, 'pars
         });
     }
 
-    // Take the alt error. If one doesn't exist, generate a fake one.
-    pub(crate) fn take_alt(&mut self) -> Located<I::Cursor, E::Error> {
-        let fake_span = self.span_since(&self.cursor());
-        self.errors.alt.take().unwrap_or_else(|| {
-            Located::at(
-                self.cursor.clone(),
-                E::Error::expected_found([], None, fake_span),
-            )
-        })
+    // Take the alt error, if one exists
+    pub(crate) fn take_alt(&mut self) -> Option<Located<I::Cursor, E::Error>> {
+        self.errors.alt.take()
     }
 }
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -357,12 +357,15 @@ pub trait Parser<'src, I: Input<'src>, O, E: ParserExtra<'src, I> = extra::Defau
         let mut own = InputOwn::new_state(input, state);
         let mut inp = own.as_ref_start();
         let res = self.then_ignore(end()).go::<Emit>(&mut inp);
-        let alt = inp.take_alt();
+        let alt = inp.take_alt().map(|alt| alt.err).unwrap_or_else(|| {
+            let fake_span = inp.span_since(&inp.cursor());
+            E::Error::expected_found([], None, fake_span)
+        });
         let mut errs = own.into_errs();
         let out = match res {
             Ok(out) => Some(out),
             Err(()) => {
-                errs.push(alt.err);
+                errs.push(alt);
                 None
             }
         };
@@ -402,12 +405,15 @@ pub trait Parser<'src, I: Input<'src>, O, E: ParserExtra<'src, I> = extra::Defau
         let mut own = InputOwn::new_state(input, state);
         let mut inp = own.as_ref_start();
         let res = self.then_ignore(end()).go::<Check>(&mut inp);
-        let alt = inp.take_alt();
+        let alt = inp.take_alt().map(|alt| alt.err).unwrap_or_else(|| {
+            let fake_span = inp.span_since(&inp.cursor());
+            E::Error::expected_found([], None, fake_span)
+        });
         let mut errs = own.into_errs();
         let out = match res {
             Ok(()) => Some(()),
             Err(()) => {
-                errs.push(alt.err);
+                errs.push(alt);
                 None
             }
         };
@@ -3625,4 +3631,25 @@ mod tests {
             )]),
         );
     }
+
+    #[test]
+    fn map_err() {
+        use crate::{error::Error, util::Maybe::Val};
+
+        let parser = just::<char, &str, extra::Err<_>>('"').map_err(move |e: Rich<char>| {
+            println!("Found = {:?}", e.found());
+            println!("Expected = {:?}", e.expected().collect::<Vec<_>>());
+            println!("Span = {:?}", e.span());
+            Error::<&str>::expected_found([Some(Val('"'))], e.found().copied().map(Val), *e.span())
+        });
+
+        assert_eq!(
+            parser.parse(r#"H"#).into_result(),
+            Err(vec![Error::<&str>::expected_found(
+                [Some(Val('"'))],
+                Some(Val('H')),
+                (0..1).into()
+            )])
+        );
+    }
 }
diff --git a/src/recovery.rs b/src/recovery.rs
@@ -41,7 +41,7 @@ where
         inp: &mut InputRef<'src, '_, I, E>,
         _parser: &P,
     ) -> PResult<M, O> {
-        let alt = inp.take_alt();
+        let alt = inp.take_alt().unwrap(); // Can't fail!
         let out = match self.0.go::<M>(inp) {
             Ok(out) => out,
             Err(()) => {
@@ -110,7 +110,7 @@ where
         inp: &mut InputRef<'src, '_, I, E>,
         parser: &P,
     ) -> PResult<M, O> {
-        let alt = inp.take_alt();
+        let alt = inp.take_alt().unwrap(); // Can't fail!
         loop {
             let before = inp.save();
             if let Ok(()) = self.until.go::<Check>(inp) {
@@ -170,7 +170,7 @@ where
         inp: &mut InputRef<'src, '_, I, E>,
         _parser: &P,
     ) -> PResult<M, O> {
-        let alt = inp.take_alt();
+        let alt = inp.take_alt().unwrap(); // Can't fail!
         loop {
             let before = inp.save();
             if let Ok(()) = self.until.go::<Check>(inp) {