Repository: toml-rs/toml
Pull request #912 (2025-05-28 21:19:13)

fix(datetime): Improve error message quality
This includes fixing some validation bugs in the parser.

The parser is effectively untested at this time.  I have some experimental changes that allow running the TOML conformance tests on this parser which is how I validated this.  My hope is we'll switch exclusively to parsing using `toml_datetime`s parser.

This doesn't add span tracking to get the same level of quality of error messages as the `winnow` parser in `toml_edit` but I'm hoping that that is ok.

There is also some bad error messages when it comes to months with a max day below 31.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index d037d308..246317a3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -955,6 +955,7 @@ name = "toml_datetime"
 version = "0.6.9"
 dependencies = [
  "serde",
+ "snapbox",
 ]
 
 [[package]]
diff --git a/crates/toml_datetime/Cargo.toml b/crates/toml_datetime/Cargo.toml
index 72090738..3ced7de1 100644
--- a/crates/toml_datetime/Cargo.toml
+++ b/crates/toml_datetime/Cargo.toml
@@ -30,3 +30,6 @@ serde = { version = "1.0.145", optional = true }
 
 [lints]
 workspace = true
+
+[dev-dependencies]
+snapbox = "0.6.21"
diff --git a/crates/toml_datetime/src/datetime.rs b/crates/toml_datetime/src/datetime.rs
index a9100255..2ed7f04d 100644
--- a/crates/toml_datetime/src/datetime.rs
+++ b/crates/toml_datetime/src/datetime.rs
@@ -92,11 +92,6 @@ pub struct Datetime {
     pub offset: Option<Offset>,
 }
 
-/// Error returned from parsing a `Datetime` in the `FromStr` implementation.
-#[derive(Debug, Clone)]
-#[non_exhaustive]
-pub struct DatetimeParseError {}
-
 // Currently serde itself doesn't have a datetime type, so we map our `Datetime`
 // to a special value in the serde data model. Namely one with these special
 // fields/struct names.
@@ -298,205 +293,469 @@ impl FromStr for Datetime {
         // 0000-00-00T00:00:00.00
         // 0000-00-00
         // 00:00:00.00
-        if date.len() < 3 {
-            return Err(DatetimeParseError {});
-        }
-        let mut offset_allowed = true;
-        let mut chars = date.chars();
-
-        // First up, parse the full date if we can
-        let full_date = if chars.clone().nth(2) == Some(':') {
-            offset_allowed = false;
-            None
-        } else {
-            let y1 = u16::from(digit(&mut chars)?);
-            let y2 = u16::from(digit(&mut chars)?);
-            let y3 = u16::from(digit(&mut chars)?);
-            let y4 = u16::from(digit(&mut chars)?);
-
-            match chars.next() {
-                Some('-') => {}
-                _ => return Err(DatetimeParseError {}),
-            }
-
-            let m1 = digit(&mut chars)?;
-            let m2 = digit(&mut chars)?;
-
-            match chars.next() {
-                Some('-') => {}
-                _ => return Err(DatetimeParseError {}),
-            }
-
-            let d1 = digit(&mut chars)?;
-            let d2 = digit(&mut chars)?;
+        //
+        // ```abnf
+        // ;; Date and Time (as defined in RFC 3339)
+        //
+        // date-time      = offset-date-time / local-date-time / local-date / local-time
+        //
+        // date-fullyear  = 4DIGIT
+        // date-month     = 2DIGIT  ; 01-12
+        // date-mday      = 2DIGIT  ; 01-28, 01-29, 01-30, 01-31 based on month/year
+        // time-delim     = "T" / %x20 ; T, t, or space
+        // time-hour      = 2DIGIT  ; 00-23
+        // time-minute    = 2DIGIT  ; 00-59
+        // time-second    = 2DIGIT  ; 00-58, 00-59, 00-60 based on leap second rules
+        // time-secfrac   = "." 1*DIGIT
+        // time-numoffset = ( "+" / "-" ) time-hour ":" time-minute
+        // time-offset    = "Z" / time-numoffset
+        //
+        // partial-time   = time-hour ":" time-minute ":" time-second [ time-secfrac ]
+        // full-date      = date-fullyear "-" date-month "-" date-mday
+        // full-time      = partial-time time-offset
+        //
+        // ;; Offset Date-Time
+        //
+        // offset-date-time = full-date time-delim full-time
+        //
+        // ;; Local Date-Time
+        //
+        // local-date-time = full-date time-delim partial-time
+        //
+        // ;; Local Date
+        //
+        // local-date = full-date
+        //
+        // ;; Local Time
+        //
+        // local-time = partial-time
+        // ```
+        let mut result = Datetime {
+            date: None,
+            time: None,
+            offset: None,
+        };
 
-            let date = Date {
-                year: y1 * 1000 + y2 * 100 + y3 * 10 + y4,
-                month: m1 * 10 + m2,
-                day: d1 * 10 + d2,
-            };
+        let mut lexer = Lexer::new(date);
+
+        let digits = lexer
+            .next()
+            .ok_or(DatetimeParseError::new().expected("year or hour"))?;
+        digits
+            .is(TokenKind::Digits)
+            .map_err(|err| err.expected("year or hour"))?;
+        let sep = lexer
+            .next()
+            .ok_or(DatetimeParseError::new().expected("`-` or `:`"))?;
+        match sep.kind {
+            TokenKind::Dash => {
+                let year = digits;
+                let month = lexer
+                    .next()
+                    .ok_or_else(|| DatetimeParseError::new().what("date").expected("month"))?;
+                month
+                    .is(TokenKind::Digits)
+                    .map_err(|err| err.what("date").expected("month"))?;
+                let sep = lexer
+                    .next()
+                    .ok_or(DatetimeParseError::new().what("date").expected("`-`"))?;
+                sep.is(TokenKind::Dash)
+                    .map_err(|err| err.what("date").expected("`-`"))?;
+                let day = lexer
+                    .next()
+                    .ok_or(DatetimeParseError::new().what("date").expected("day"))?;
+                day.is(TokenKind::Digits)
+                    .map_err(|err| err.what("date").expected("day"))?;
+
+                if year.raw.len() != 4 {
+                    return Err(DatetimeParseError::new()
+                        .what("date")
+                        .expected("4 digit year"));
+                }
+                if month.raw.len() != 2 {
+                    return Err(DatetimeParseError::new()
+                        .what("date")
+                        .expected("2 digit month"));
+                }
+                if day.raw.len() != 2 {
+                    return Err(DatetimeParseError::new()
+                        .what("date")
+                        .expected("2 digit day"));
+                }
+                let date = Date {
+                    year: year.raw.parse().map_err(|_err| DatetimeParseError::new())?,
+                    month: month
+                        .raw
+                        .parse()
+                        .map_err(|_err| DatetimeParseError::new())?,
+                    day: day.raw.parse().map_err(|_err| DatetimeParseError::new())?,
+                };
+                if date.month < 1 || date.month > 12 {
+                    return Err(DatetimeParseError::new()
+                        .what("date")
+                        .expected("month between 1 and 12"));
+                }
+                let is_leap_year =
+                    (date.year % 4 == 0) && ((date.year % 100 != 0) || (date.year % 400 == 0));
+                let max_days_in_month = match date.month {
+                    2 if is_leap_year => 29,
+                    2 => 28,
+                    4 | 6 | 9 | 11 => 30,
+                    _ => 31,
+                };
+                if date.day < 1 || date.day > max_days_in_month {
+                    return Err(DatetimeParseError::new()
+                        .what("date")
+                        .expected("day between 1 and 31"));
+                }
 
-            if date.month < 1 || date.month > 12 {
-                return Err(DatetimeParseError {});
+                result.date = Some(date);
             }
-            let is_leap_year =
-                (date.year % 4 == 0) && ((date.year % 100 != 0) || (date.year % 400 == 0));
-            let max_days_in_month = match date.month {
-                2 if is_leap_year => 29,
-                2 => 28,
-                4 | 6 | 9 | 11 => 30,
-                _ => 31,
-            };
-            if date.day < 1 || date.day > max_days_in_month {
-                return Err(DatetimeParseError {});
+            TokenKind::Colon => lexer = Lexer::new(date),
+            _ => {
+                return Err(DatetimeParseError::new().expected("`-` or `:`"));
             }
-
-            Some(date)
-        };
+        }
 
         // Next parse the "partial-time" if available
-        let next = chars.clone().next();
-        let partial_time = if full_date.is_some()
-            && (next == Some('T') || next == Some('t') || next == Some(' '))
-        {
-            chars.next();
-            true
+        let partial_time = if result.date.is_some() {
+            let sep = lexer.next();
+            match sep {
+                Some(token) if matches!(token.kind, TokenKind::T | TokenKind::Space) => true,
+                Some(_token) => {
+                    return Err(DatetimeParseError::new()
+                        .what("date-time")
+                        .expected("`T` between date and time"));
+                }
+                None => false,
+            }
         } else {
-            full_date.is_none()
+            result.date.is_none()
         };
 
-        let time = if partial_time {
-            let h1 = digit(&mut chars)?;
-            let h2 = digit(&mut chars)?;
-            match chars.next() {
-                Some(':') => {}
-                _ => return Err(DatetimeParseError {}),
+        if partial_time {
+            let hour = lexer
+                .next()
+                .ok_or_else(|| DatetimeParseError::new().what("time").expected("hour"))?;
+            hour.is(TokenKind::Digits)
+                .map_err(|err| err.what("time").expected("hour"))?;
+            let sep = lexer
+                .next()
+                .ok_or(DatetimeParseError::new().what("time").expected("`:`"))?;
+            sep.is(TokenKind::Colon)
+                .map_err(|err| err.what("time").expected("`:`"))?;
+            let minute = lexer
+                .next()
+                .ok_or(DatetimeParseError::new().what("time").expected("minute"))?;
+            minute
+                .is(TokenKind::Digits)
+                .map_err(|err| err.what("time").expected("minute"))?;
+            let sep = lexer
+                .next()
+                .ok_or(DatetimeParseError::new().what("time").expected("`:`"))?;
+            sep.is(TokenKind::Colon)
+                .map_err(|err| err.what("time").expected("`:`"))?;
+            let second = lexer
+                .next()
+                .ok_or(DatetimeParseError::new().what("time").expected("second"))?;
+            second
+                .is(TokenKind::Digits)
+                .map_err(|err| err.what("time").expected("second"))?;
+
+            let nanosecond = if lexer.clone().next().map(|t| t.kind) == Some(TokenKind::Dot) {
+                let sep = lexer.next().ok_or(DatetimeParseError::new())?;
+                sep.is(TokenKind::Dot)?;
+                let nanosecond = lexer.next().ok_or(
+                    DatetimeParseError::new()
+                        .what("time")
+                        .expected("nanosecond"),
+                )?;
+                nanosecond
+                    .is(TokenKind::Digits)
+                    .map_err(|err| err.what("time").expected("nanoseconds"))?;
+                Some(nanosecond)
+            } else {
+                None
+            };
+
+            if hour.raw.len() != 2 {
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("2 digit hour"));
             }
-            let m1 = digit(&mut chars)?;
-            let m2 = digit(&mut chars)?;
-            match chars.next() {
-                Some(':') => {}
-                _ => return Err(DatetimeParseError {}),
+            if minute.raw.len() != 2 {
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("2 digit minute"));
             }
-            let s1 = digit(&mut chars)?;
-            let s2 = digit(&mut chars)?;
-
-            let mut nanosecond = 0;
-            if chars.clone().next() == Some('.') {
-                chars.next();
-                let whole = chars.as_str();
-
-                let mut end = whole.len();
-                for (i, byte) in whole.bytes().enumerate() {
-                    #[allow(clippy::single_match_else)]
-                    match byte {
-                        b'0'..=b'9' => {
-                            if i < 9 {
-                                let p = 10_u32.pow(8 - i as u32);
-                                nanosecond += p * u32::from(byte - b'0');
-                            }
-                        }
-                        _ => {
-                            end = i;
-                            break;
-                        }
-                    }
-                }
-                if end == 0 {
-                    return Err(DatetimeParseError {});
-                }
-                chars = whole[end..].chars();
+            if second.raw.len() != 2 {
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("2 digit second"));
             }
 
             let time = Time {
-                hour: h1 * 10 + h2,
-                minute: m1 * 10 + m2,
-                second: s1 * 10 + s2,
-                nanosecond,
+                hour: hour.raw.parse().map_err(|_err| DatetimeParseError::new())?,
+                minute: minute
+                    .raw
+                    .parse()
+                    .map_err(|_err| DatetimeParseError::new())?,
+                second: second
+                    .raw
+                    .parse()
+                    .map_err(|_err| DatetimeParseError::new())?,
+                nanosecond: nanosecond.map(|t| s_to_nanoseconds(t.raw)).unwrap_or(0),
             };
 
-            if time.hour > 24 {
-                return Err(DatetimeParseError {});
+            if time.hour > 23 {
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("hour between 1 and 23"));
             }
             if time.minute > 59 {
-                return Err(DatetimeParseError {});
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("minute between 1 and 59"));
             }
             // 00-58, 00-59, 00-60 based on leap second rules
             if time.second > 60 {
-                return Err(DatetimeParseError {});
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("second between 1 and 60"));
             }
             if time.nanosecond > 999_999_999 {
-                return Err(DatetimeParseError {});
+                return Err(DatetimeParseError::new()
+                    .what("time")
+                    .expected("nanoseconds overflowed"));
             }
 
-            Some(time)
-        } else {
-            offset_allowed = false;
-            None
-        };
+            result.time = Some(time);
+        }
 
         // And finally, parse the offset
-        let offset = if offset_allowed {
-            let next = chars.clone().next();
-            if next == Some('Z') || next == Some('z') {
-                chars.next();
-                Some(Offset::Z)
-            } else if next.is_none() {
-                None
-            } else {
-                let sign = match next {
-                    Some('+') => 1,
-                    Some('-') => -1,
-                    _ => return Err(DatetimeParseError {}),
-                };
-                chars.next();
-                let h1 = digit(&mut chars)? as i16;
-                let h2 = digit(&mut chars)? as i16;
-                match chars.next() {
-                    Some(':') => {}
-                    _ => return Err(DatetimeParseError {}),
+        if result.date.is_some() && result.time.is_some() {
+            match lexer.next() {
+                Some(token) if token.kind == TokenKind::Z => {
+                    result.offset = Some(Offset::Z);
                 }
-                let m1 = digit(&mut chars)? as i16;
-                let m2 = digit(&mut chars)? as i16;
+                Some(token) if matches!(token.kind, TokenKind::Plus | TokenKind::Dash) => {
+                    let sign = if token.kind == TokenKind::Plus { 1 } else { -1 };
+                    let hours = lexer
+                        .next()
+                        .ok_or(DatetimeParseError::new().what("offset").expected("hours"))?;
+                    hours
+                        .is(TokenKind::Digits)
+                        .map_err(|err| err.what("offset").expected("hours"))?;
+                    let sep = lexer
+                        .next()
+                        .ok_or(DatetimeParseError::new().what("offset").expected("`:`"))?;
+                    sep.is(TokenKind::Colon)
+                        .map_err(|err| err.what("offset").expected("`:`"))?;
+                    let minutes = lexer
+                        .next()
+                        .ok_or(DatetimeParseError::new().what("offset").expected("minutes"))?;
+                    minutes
+                        .is(TokenKind::Digits)
+                        .map_err(|err| err.what("offset").expected("minutes"))?;
+
+                    if hours.raw.len() != 2 {
+                        return Err(DatetimeParseError::new()
+                            .what("offset")
+                            .expected("2 digit hours"));
+                    }
+                    if minutes.raw.len() != 2 {
+                        return Err(DatetimeParseError::new()
+                            .what("offset")
+                            .expected("2 digit minutes"));
+                    }
 
-                let hours = h1 * 10 + h2;
-                let minutes = m1 * 10 + m2;
+                    let hours = hours
+                        .raw
+                        .parse::<u8>()
+                        .map_err(|_err| DatetimeParseError::new())?;
+                    let minutes = minutes
+                        .raw
+                        .parse::<u8>()
+                        .map_err(|_err| DatetimeParseError::new())?;
+
+                    if hours > 23 {
+                        return Err(DatetimeParseError::new()
+                            .what("offset")
+                            .expected("hours between 1 and 23"));
+                    }
+                    if minutes > 59 {
+                        return Err(DatetimeParseError::new()
+                            .what("offset")
+                            .expected("minutes between 1 and 59"));
+                    }
 
-                let total_minutes = sign * (hours * 60 + minutes);
+                    let total_minutes = sign * (hours as i16 * 60 + minutes as i16);
 
-                if !((-24 * 60)..=(24 * 60)).contains(&total_minutes) {
-                    return Err(DatetimeParseError {});
-                }
+                    if !((-24 * 60)..=(24 * 60)).contains(&total_minutes) {
+                        return Err(DatetimeParseError::new().what("offset"));
+                    }
 
-                Some(Offset::Custom {
-                    minutes: total_minutes,
-                })
+                    result.offset = Some(Offset::Custom {
+                        minutes: total_minutes,
+                    });
+                }
+                Some(_token) => {
+                    return Err(DatetimeParseError::new()
+                        .what("offset")
+                        .expected("`Z`, +offset, -offset"));
+                }
+                None => {}
             }
-        } else {
-            None
-        };
+        }
 
         // Return an error if we didn't hit eof, otherwise return our parsed
         // date
-        if chars.next().is_some() {
-            return Err(DatetimeParseError {});
+        if lexer.unknown().is_some() {
+            return Err(DatetimeParseError::new());
+        }
+
+        Ok(result)
+    }
+}
+
+fn s_to_nanoseconds(input: &str) -> u32 {
+    let mut nanosecond = 0;
+    for (i, byte) in input.bytes().enumerate() {
+        if byte.is_ascii_digit() {
+            if i < 9 {
+                let p = 10_u32.pow(8 - i as u32);
+                nanosecond += p * u32::from(byte - b'0');
+            }
+        } else {
+            panic!("invalid nanoseconds {input:?}");
+        }
+    }
+    nanosecond
+}
+
+#[derive(Copy, Clone)]
+struct Token<'s> {
+    kind: TokenKind,
+    raw: &'s str,
+}
+
+impl Token<'_> {
+    fn is(&self, kind: TokenKind) -> Result<(), DatetimeParseError> {
+        if self.kind == kind {
+            Ok(())
+        } else {
+            Err(DatetimeParseError::new())
         }
+    }
+}
 
-        Ok(Datetime {
-            date: full_date,
-            time,
-            offset,
+#[derive(Copy, Clone, PartialEq, Eq)]
+enum TokenKind {
+    Digits,
+    Dash,
+    Colon,
+    Dot,
+    T,
+    Space,
+    Z,
+    Plus,
+    Unknown,
+}
+
+#[derive(Copy, Clone)]
+struct Lexer<'s> {
+    stream: &'s str,
+}
+
+impl<'s> Lexer<'s> {
+    fn new(input: &'s str) -> Self {
+        Self { stream: input }
+    }
+
+    fn unknown(&mut self) -> Option<Token<'s>> {
+        let remaining = self.stream.len();
+        if remaining == 0 {
+            return None;
+        }
+        let raw = self.stream;
+        self.stream = &self.stream[remaining..remaining];
+        Some(Token {
+            kind: TokenKind::Unknown,
+            raw,
         })
     }
 }
 
-fn digit(chars: &mut str::Chars<'_>) -> Result<u8, DatetimeParseError> {
-    match chars.next() {
-        Some(c) if c.is_ascii_digit() => Ok(c as u8 - b'0'),
-        _ => Err(DatetimeParseError {}),
+impl<'s> Iterator for Lexer<'s> {
+    type Item = Token<'s>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let (kind, end) = match self.stream.as_bytes().first()? {
+            b'0'..=b'9' => {
+                let end = self
+                    .stream
+                    .as_bytes()
+                    .iter()
+                    .position(|b| !b.is_ascii_digit())
+                    .unwrap_or(self.stream.len());
+                (TokenKind::Digits, end)
+            }
+            b'-' => (TokenKind::Dash, 1),
+            b':' => (TokenKind::Colon, 1),
+            b'T' | b't' => (TokenKind::T, 1),
+            b' ' => (TokenKind::Space, 1),
+            b'Z' | b'z' => (TokenKind::Z, 1),
+            b'+' => (TokenKind::Plus, 1),
+            b'.' => (TokenKind::Dot, 1),
+            _ => (TokenKind::Unknown, self.stream.len()),
+        };
+        let (raw, rest) = self.stream.split_at(end);
+        self.stream = rest;
+        Some(Token { kind, raw })
+    }
+}
+
+/// Error returned from parsing a `Datetime` in the `FromStr` implementation.
+#[derive(Debug, Clone)]
+#[non_exhaustive]
+pub struct DatetimeParseError {
+    what: Option<&'static str>,
+    expected: Option<&'static str>,
+}
+
+impl DatetimeParseError {
+    fn new() -> Self {
+        Self {
+            what: None,
+            expected: None,
+        }
+    }
+    fn what(mut self, what: &'static str) -> Self {
+        self.what = Some(what);
+        self
+    }
+    fn expected(mut self, expected: &'static str) -> Self {
+        self.expected = Some(expected);
+        self
+    }
+}
+
+impl fmt::Display for DatetimeParseError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if let Some(what) = self.what {
+            write!(f, "invalid {what}")?;
+        } else {
+            "invalid datetime".fmt(f)?;
+        }
+        if let Some(expected) = self.expected {
+            write!(f, ", expected {expected}")?;
+        }
+        Ok(())
     }
 }
 
+impl error::Error for DatetimeParseError {}
+
 #[cfg(feature = "serde")]
 impl ser::Serialize for Datetime {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
@@ -674,11 +933,3 @@ impl<'de> de::Deserialize<'de> for DatetimeFromString {
         deserializer.deserialize_str(Visitor)
     }
 }
-
-impl fmt::Display for DatetimeParseError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "failed to parse datetime".fmt(f)
-    }
-}
-
-impl error::Error for DatetimeParseError {}


Test patch:
diff --git a/crates/toml_datetime/tests/parse.rs b/crates/toml_datetime/tests/parse.rs
new file mode 100644
index 00000000..b492f4f2
--- /dev/null
+++ b/crates/toml_datetime/tests/parse.rs
@@ -0,0 +1,62 @@
+use snapbox::prelude::*;
+use snapbox::str;
+
+#[track_caller]
+fn t(input: &str, expected: impl IntoData) {
+    let actual = input.parse::<toml_datetime::Datetime>();
+    snapbox::assert_data_eq!(actual.to_debug(), expected.raw());
+}
+
+#[test]
+fn only_t() {
+    t(
+        "T",
+        str![[r#"
+Err(
+    DatetimeParseError {
+        what: None,
+        expected: Some(
+            "year or hour",
+        ),
+    },
+)
+
+"#]],
+    );
+}
+
+#[test]
+fn only_tz() {
+    t(
+        "TZ",
+        str![[r#"
+Err(
+    DatetimeParseError {
+        what: None,
+        expected: Some(
+            "year or hour",
+        ),
+    },
+)
+
+"#]],
+    );
+}
+
+#[test]
+fn only_tz_dot() {
+    t(
+        "TZ.",
+        str![[r#"
+Err(
+    DatetimeParseError {
+        what: None,
+        expected: Some(
+            "year or hour",
+        ),
+    },
+)
+
+"#]],
+    );
+}