Better error message for txn parse errors

This is not yet perfect nor complete, as there are some tricky corners with current parser logic to commit into error state and emit correct parse error message. But this is much better than what it was, and this is much better than the old ANTLR based reporting. Signed-off-by: 35V LG84 <[email protected]>
e257-fi · Feb 22, 2025 · b6cc30d · b6cc30d
1 parent 734fd23
commit b6cc30d
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 86 deletions.
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -39,8 +39,8 @@ New features:
 
 Changed functionality:
 
-* ...
-** ...
+* Better and more informative error messages of invalid transaction data ("parse errors")
+
 
 ==== Fixes
 

diff --git a/tackler-core/src/parser/parts/identifier.rs b/tackler-core/src/parser/parts/identifier.rs
@@ -4,7 +4,8 @@
  */
 
 use crate::parser::Stream;
-use winnow::combinator::repeat;
+use winnow::combinator::{cut_err, repeat};
+use winnow::error::{StrContext, StrContextValue};
 use winnow::token::{one_of, take_while};
 use winnow::{ModalResult, Parser};
 /*
@@ -45,6 +46,8 @@ NameStartChar
    ;
  */
 
+const CTX_LABEL: &str = "name";
+
 fn id_char(c: char) -> bool {
     id_start_char(c)
         | matches!(
@@ -91,16 +94,31 @@ pub(crate) fn p_identifier<'s>(is: &mut Stream<'s>) -> ModalResult<&'s str> {
 }
 
 fn p_id_part_helper<'s>(is: &mut Stream<'s>) -> ModalResult<&'s str> {
-    (take_while(1, ':'), p_id_part).take().parse_next(is)
+    (
+        take_while(1, ':'),
+        cut_err(p_id_part)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description(
+                "sub-part of name",
+            ))),
+    )
+        .take()
+        .parse_next(is)
 }
 
 pub(crate) fn p_multi_part_id<'s>(is: &mut Stream<'s>) -> ModalResult<&'s str> {
     let dec_str = (
         p_identifier,
-        repeat(0.., p_id_part_helper).fold(String::new, |mut string, s| {
-            string.push_str(s);
-            string
-        }),
+        cut_err(
+            repeat(0.., p_id_part_helper).fold(String::new, |mut string, s| {
+                string.push_str(s);
+                string
+            }),
+        )
+        .context(StrContext::Label(CTX_LABEL))
+        .context(StrContext::Expected(StrContextValue::Description(
+            "for multi part name",
+        ))),
     )
         .take()
         .parse_next(is)?;

diff --git a/tackler-core/src/parser/parts/posting_value.rs b/tackler-core/src/parser/parts/posting_value.rs
@@ -34,21 +34,39 @@ amount: INT | NUMBER;
 unit: ID;
  */
 
+use winnow::combinator::cut_err;
+use winnow::error::StrContext;
+use winnow::error::StrContextValue;
+
 struct Value<'s> {
     value: Decimal,
     commodity: &'s str,
 }
 
 fn p_opening_pos<'s>(is: &mut Stream<'s>) -> ModalResult<Value<'s>> {
+    const CTX_LABEL: &str = "opening position";
     let m = seq!(
         _: space1,
         _: '{',
         _: space0,
-        p_number,
-        _: space1,
-        p_identifier,
+
+        cut_err(p_number)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("number"))),
+
+        _: cut_err(space1)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("space"))),
+
+        cut_err(p_identifier)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("commodity name"))),
+
         _: space0,
-        _: '}'
+
+        _: cut_err('}')
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("closing '}'"))),
     )
     .parse_next(is)?;
 
@@ -64,13 +82,26 @@ enum PriceType {
 }
 
 fn p_closing_pos<'s>(is: &mut Stream<'s>) -> ModalResult<(PriceType, Value<'s>)> {
+    const CTX_LABEL: &str = "closing position";
     let m = seq!(
         _:space1,
         alt(('@', '=')),
-        _:space1,
-        p_number,
-        _:space1,
-        p_identifier,
+        _:cut_err(space1)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("space"))),
+
+        cut_err(p_number)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("number"))),
+
+        _:cut_err(space1)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("space"))),
+
+        cut_err(p_identifier)
+            .context(StrContext::Label(CTX_LABEL))
+            .context(StrContext::Expected(StrContextValue::Description("commodity name"))),
+
     )
     .parse_next(is)?;
 
@@ -278,19 +309,11 @@ mod tests {
                "|1.23 ACME·INC
                 |"
             ).strip_margin(),),
-            (indoc!(
-               "|1.23 ACME·INC @ 1.23
-                |"
-            ).strip_margin(),),
             (indoc!(
                "|1.23 ACME·INC @ 1.23 €
                 |"
             ).strip_margin(),),
 
-            (indoc!(
-               "|1.23 ACME·INC = 1.23
-                |"
-            ).strip_margin(),),
             (indoc!(
                "|1.23 ACME·INC = 1.23 €
                 |"
@@ -305,14 +328,6 @@ mod tests {
                 |"
             ).strip_margin(),),
 
-            (indoc!(
-               "|1.23 {4.56} ACME·INC = 5.67
-                |"
-            ).strip_margin(),),
-            (indoc!(
-               "|1.23 {4.56 $} ACME·INC = 5.67
-                |"
-            ).strip_margin(),),
             (indoc!(
                "|1.23 {4.56} ACME·INC = 5.67 £
                 |"
@@ -323,14 +338,6 @@ mod tests {
             ).strip_margin(),),
 
 
-            (indoc!(
-               "|1.23 {4.56} ACME·INC @ 5.67
-                |"
-            ).strip_margin(),),
-            (indoc!(
-               "|1.23 {4.56 $} ACME·INC @ 5.67
-                |"
-            ).strip_margin(),),
             (indoc!(
                "|1.23 {4.56} ACME·INC @ 5.67 £
                 |"
@@ -355,7 +362,7 @@ mod tests {
         ];
 
         let mut count = 0;
-        for t in pok_values {
+        for t in &pok_values {
             let mut settings = Settings::default();
             let mut is = Stream {
                 input: t.0.as_str(),
@@ -370,6 +377,6 @@ mod tests {
             );
             count += 1;
         }
-        assert_eq!(count, 19);
+        assert_eq!(count, pok_values.len());
     }
 }
diff --git a/tackler-core/src/parser/parts/timestamp.rs b/tackler-core/src/parser/parts/timestamp.rs
@@ -118,9 +118,17 @@ fn p_offset(is: &mut Stream<'_>) -> ModalResult<jiff::tz::Offset> {
     let (sign, h, m) =
         seq!(
             alt(('+'.value(1i32), '-'.value(-1i32))),
-            take_while(2, AsChar::is_dec_digit).try_map(i32::from_str),
-            _: ":",
-            take_while(2, AsChar::is_dec_digit).try_map(i32::from_str),
+            cut_err(take_while(2, AsChar::is_dec_digit))
+                .context(StrContext::Label(CTX_LABEL))
+                .context(StrContext::Expected(StrContextValue::Description("offset hours 'HH' for offset 'HH:MM'")))
+        .try_map(i32::from_str),
+            _: cut_err(":")
+                .context(StrContext::Label(CTX_LABEL))
+                .context(StrContext::Expected(StrContextValue::Description("offset separator ':' for offset 'HH:MM'"))),
+            cut_err(take_while(2, AsChar::is_dec_digit))
+                .context(StrContext::Label(CTX_LABEL))
+                .context(StrContext::Expected(StrContextValue::Description("offset minutes 'MM' for offset 'HH:MM'")))
+            .try_map(i32::from_str),
         )
         .parse_next(is)?;
 

diff --git a/tackler-core/src/parser/parts/txn_header.rs b/tackler-core/src/parser/parts/txn_header.rs
@@ -32,7 +32,14 @@ pub(crate) fn parse_txn_header(is: &mut Stream<'_>) -> ModalResult<TxnHeader> {
         _: preceded(opt(space1),
             cut_err(line_ending)
                 .context(StrContext::Label("Txn Header"))
-                .context(StrContext::Expected(StrContextValue::Description("format: timestamp [(code)] ['description]"))),
+                .context(StrContext::Expected(StrContextValue::Description(
+"format: timestamp [(code)] ['description]
+ISO 8601 Timestamp:
+    YYYY-MM-DD
+    YYYY-MM-DDThh:mm:ss[+-HH:MM]
+    YYYY-MM-DDThh:mm:ss.sss[+-HH:MM]
+"
+            ))),
         ),
         opt(parse_txn_meta),
         opt(repeat(1.., parse_txn_comment))

diff --git a/tackler-core/src/parser/parts/txn_header_code.rs b/tackler-core/src/parser/parts/txn_header_code.rs
@@ -6,6 +6,7 @@
 use crate::parser::Stream;
 use winnow::combinator::cut_err;
 use winnow::error::StrContext;
+use winnow::error::StrContextValue;
 use winnow::token::take_while;
 use winnow::{ModalResult, Parser, seq};
 
@@ -21,7 +22,13 @@ pub(crate) fn parse_txn_code<'s>(is: &mut Stream<'s>) -> ModalResult<&'s str> {
         _: '(',
         take_while(0..,valid_code_char),
         _: cut_err(')')
-            .context(StrContext::Label("code")),
+            .context(StrContext::Label("transaction code"))
+            .context(StrContext::Expected(StrContextValue::Description(
+"valid characters or closing ')'
+Invalid characters for code value are:
+    '\\'', '(', ')', '[', ']', '{', '}', '<', '>', \\r', '\\n'
+"
+        ))),
     )
     .parse_next(is)?;
 

diff --git a/tackler-core/src/parser/parts/txn_meta_uuid.rs b/tackler-core/src/parser/parts/txn_meta_uuid.rs
@@ -17,7 +17,6 @@ const CTX_LABEL: &str = "txn metadata uuid";
 const UUID_HELP: &str = " # uuid: d77b6b92-42f1-419d-834c-66d69f155ad6";
 
 fn p_uuid(is: &mut Stream<'_>) -> ModalResult<Uuid> {
-    // todo: check uuid from bytes
     let uuid_str = seq!(
         cut_err(take_while(8, AsChar::is_hex_digit))
             .context(StrContext::Label(CTX_LABEL))

diff --git a/tackler-core/src/parser/tackler_parser.rs b/tackler-core/src/parser/tackler_parser.rs
@@ -5,53 +5,21 @@
 
 use crate::parser::Stream;
 use crate::parser::parts::txns::parse_txns;
-use std::fmt::Write;
-
-use std::fs::File;
-use std::io::Read;
-use std::path::Path;
 
 use crate::kernel::Settings;
 use crate::model::Txns;
 use crate::tackler;
+use std::fs::File;
+use std::io::Read;
+use std::path::Path;
+use winnow::Parser;
 
 pub(crate) fn txns_text(input: &mut &str, settings: &mut Settings) -> Result<Txns, tackler::Error> {
-    let mut is = Stream {
+    let is = Stream {
         input,
         state: settings,
     };
-
-    match parse_txns(&mut is) {
-        Ok(txns) => Ok(txns),
-        Err(err) => {
-            let mut msg = "Failed to process txn input\n".to_string();
-            //let _ = writeln!(msg, "Error: {}", err);
-            match err.into_inner() {
-                Ok(ce) => {
-                    if let Some(cause) = ce.cause() {
-                        let _ = writeln!(msg, "Cause:\n{}\n", cause);
-                    }
-                    let _ = writeln!(msg, "Error backtrace:");
-                    for c in ce.context() {
-                        let _ = writeln!(msg, "   {}", c);
-                    }
-                }
-                Err(_err) => {
-                    let _ = write!(msg, "Incomplete input");
-                }
-            }
-            let i = is.input.lines().next().unwrap_or(is.input);
-            let i_err = if i.chars().count() < 1024 {
-                i.to_string()
-            } else {
-                i.chars().take(1024).collect::<String>()
-            };
-
-            let _ = write!(msg, "Failed input:\n{}\n\n", i_err);
-
-            Err(msg.into())
-        }
-    }
+    parse_txns.parse(is).map_err(|err| err.to_string().into())
 }
 
 pub(crate) fn txns_file(path: &Path, settings: &mut Settings) -> Result<Txns, tackler::Error> {
-Original file line number
+Diff line change
@@ Expand Up / @@ -39,8 +39,8 @@ New features: @@
     Changed functionality:
-    * ...
-    ** ...
+    * Better and more informative error messages of invalid transaction data ("parse errors")
     ==== Fixes
@@ Expand Down @@