Repository: toml-rs/toml (rust)

Issue:
Incorrect parsing of sub-hour timezone offsets
toml-rs parses the following TOML document incorrectly:
```
d = 1911-01-01T10:11:12-00:36
```
This can be seen for example by running the example program `toml2json` which outputs:
```json
{
  "d": "1911-01-01T10:11:12+00:36"
}
```

It looks like the current `toml_datetime::Offset` structure is not even capable of representing a sub-hour negative offset.

This issue applies to the recent commit cc869e9dd5d87a2c7bee3fabcc94cbbed7538086 on main.

I'm experimenting with a TOML fuzzer which revealed this issue. I guess most normal applications are not likely to hit this case, so I understand if this gets low priority. It still seems worthwhile to fix it though.

Pull request:
fix(date)!: Allow negative minute Offsets
This is trying to balance correctness with API stability.  By doing this soon after the last release, we have the lowest adoption and this will be the least disruptive.

Looking through the top 300 or so dependents (by download count), the impact for other crate's APIs looks to be negligible,

Of the following
- crates-index
- cargo-expand
- pyproject-toml
- cargo-config2
- mdbook-katex

Only crates-index did a breaking change around the upgrade time and I suspect that was because they upgraded git2 and not us as they have little reason to put us in their public API.

There were a lot of different directions to go for the API and I prioritized:
1. Being consistent in fields being public
2. Keep the number of invalid cases down
3. Simplest to integrate with `time` and `chrono`

See #496 for more details

Fixes #496 

BREAKING CHANGE: `Offset::Custom` changed form having separate `hours` and `minutes` fields to just `minutes`

Patch:
diff --git a/crates/toml_datetime/src/datetime.rs b/crates/toml_datetime/src/datetime.rs
index d468c420..15781b91 100644
--- a/crates/toml_datetime/src/datetime.rs
+++ b/crates/toml_datetime/src/datetime.rs
@@ -94,9 +94,8 @@ pub struct Datetime {
 
 /// Error returned from parsing a `Datetime` in the `FromStr` implementation.
 #[derive(Debug, Clone)]
-pub struct DatetimeParseError {
-    _private: (),
-}
+#[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
@@ -180,11 +179,8 @@ pub enum Offset {
 
     /// Offset between local time and UTC
     Custom {
-        /// Hours: -12 to +12
-        hours: i8,
-
-        /// Minutes: 0 to 59
-        minutes: u8,
+        /// Minutes: -1_440..1_440
+        minutes: i16,
     },
 }
 
@@ -247,7 +243,16 @@ impl fmt::Display for Offset {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match *self {
             Offset::Z => write!(f, "Z"),
-            Offset::Custom { hours, minutes } => write!(f, "{:+03}:{:02}", hours, minutes),
+            Offset::Custom { mut minutes } => {
+                let mut sign = '+';
+                if minutes < 0 {
+                    minutes *= -1;
+                    sign = '-';
+                }
+                let hours = minutes / 60;
+                let minutes = minutes % 60;
+                write!(f, "{}{:02}:{:02}", sign, hours, minutes)
+            }
         }
     }
 }
@@ -263,7 +268,7 @@ impl FromStr for Datetime {
         // 0000-00-00
         // 00:00:00.00
         if date.len() < 3 {
-            return Err(DatetimeParseError { _private: () });
+            return Err(DatetimeParseError {});
         }
         let mut offset_allowed = true;
         let mut chars = date.chars();
@@ -280,7 +285,7 @@ impl FromStr for Datetime {
 
             match chars.next() {
                 Some('-') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
 
             let m1 = digit(&mut chars)?;
@@ -288,7 +293,7 @@ impl FromStr for Datetime {
 
             match chars.next() {
                 Some('-') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
 
             let d1 = digit(&mut chars)?;
@@ -301,10 +306,10 @@ impl FromStr for Datetime {
             };
 
             if date.month < 1 || date.month > 12 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if date.day < 1 || date.day > 31 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
 
             Some(date)
@@ -326,13 +331,13 @@ impl FromStr for Datetime {
             let h2 = digit(&mut chars)?;
             match chars.next() {
                 Some(':') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
             let m1 = digit(&mut chars)?;
             let m2 = digit(&mut chars)?;
             match chars.next() {
                 Some(':') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
             let s1 = digit(&mut chars)?;
             let s2 = digit(&mut chars)?;
@@ -358,7 +363,7 @@ impl FromStr for Datetime {
                     }
                 }
                 if end == 0 {
-                    return Err(DatetimeParseError { _private: () });
+                    return Err(DatetimeParseError {});
                 }
                 chars = whole[end..].chars();
             }
@@ -371,16 +376,16 @@ impl FromStr for Datetime {
             };
 
             if time.hour > 24 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.minute > 59 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.second > 59 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.nanosecond > 999_999_999 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
 
             Some(time)
@@ -401,21 +406,29 @@ impl FromStr for Datetime {
                 let sign = match next {
                     Some('+') => 1,
                     Some('-') => -1,
-                    _ => return Err(DatetimeParseError { _private: () }),
+                    _ => return Err(DatetimeParseError {}),
                 };
                 chars.next();
-                let h1 = digit(&mut chars)? as i8;
-                let h2 = digit(&mut chars)? as i8;
+                let h1 = digit(&mut chars)? as i16;
+                let h2 = digit(&mut chars)? as i16;
                 match chars.next() {
                     Some(':') => {}
-                    _ => return Err(DatetimeParseError { _private: () }),
+                    _ => return Err(DatetimeParseError {}),
+                }
+                let m1 = digit(&mut chars)? as i16;
+                let m2 = digit(&mut chars)? as i16;
+
+                let hours = h1 * 10 + h2;
+                let minutes = m1 * 10 + m2;
+
+                let total_minutes = sign * (hours * 60 + minutes);
+
+                if !((-24 * 60)..=(24 * 60)).contains(&total_minutes) {
+                    return Err(DatetimeParseError {});
                 }
-                let m1 = digit(&mut chars)?;
-                let m2 = digit(&mut chars)?;
 
                 Some(Offset::Custom {
-                    hours: sign * (h1 * 10 + h2),
-                    minutes: m1 * 10 + m2,
+                    minutes: total_minutes,
                 })
             }
         } else {
@@ -425,7 +438,7 @@ impl FromStr for Datetime {
         // Return an error if we didn't hit eof, otherwise return our parsed
         // date
         if chars.next().is_some() {
-            return Err(DatetimeParseError { _private: () });
+            return Err(DatetimeParseError {});
         }
 
         Ok(Datetime {
@@ -439,7 +452,7 @@ impl FromStr for Datetime {
 fn digit(chars: &mut str::Chars<'_>) -> Result<u8, DatetimeParseError> {
     match chars.next() {
         Some(c) if ('0'..='9').contains(&c) => Ok(c as u8 - b'0'),
-        _ => Err(DatetimeParseError { _private: () }),
+        _ => Err(DatetimeParseError {}),
     }
 }
 
diff --git a/crates/toml_edit/src/parser/datetime.rs b/crates/toml_edit/src/parser/datetime.rs
index 5a0235a4..63a76165 100644
--- a/crates/toml_edit/src/parser/datetime.rs
+++ b/crates/toml_edit/src/parser/datetime.rs
@@ -75,17 +75,17 @@ pub(crate) fn partial_time(input: Input<'_>) -> IResult<Input<'_>, Time, ParserE
 pub(crate) fn time_offset(input: Input<'_>) -> IResult<Input<'_>, Offset, ParserError<'_>> {
     alt((
         one_of((b'Z', b'z')).value(Offset::Z),
-        (one_of((b'+', b'-')), cut((time_hour, b':', time_minute))).map(
-            |(sign, (hours, _, minutes))| {
-                let hours = hours as i8;
-                let hours = match sign {
-                    b'+' => hours,
-                    b'-' => -hours,
+        (one_of((b'+', b'-')), cut((time_hour, b':', time_minute)))
+            .map(|(sign, (hours, _, minutes))| {
+                let sign = match sign {
+                    b'+' => 1,
+                    b'-' => -1,
                     _ => unreachable!("Parser prevents this"),
                 };
-                Offset::Custom { hours, minutes }
-            },
-        ),
+                sign * (hours as i16 * 60 + minutes as i16)
+            })
+            .verify(|minutes| ((-24 * 60)..=(24 * 60)).contains(minutes))
+            .map(|minutes| Offset::Custom { minutes }),
     ))
     .context(Context::Expression("time offset"))
     .parse(input)
@@ -229,40 +229,196 @@ mod test {
     #[test]
     fn offset_date_time() {
         let inputs = [
-            "1979-05-27T07:32:00Z",
-            "1979-05-27T00:32:00-07:00",
-            "1979-05-27T00:32:00.999999-07:00",
+            (
+                "1979-05-27T07:32:00Z",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Z),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00-07:00",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Custom { minutes: -7 * 60 }),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00-00:36",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Custom { minutes: -36 }),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00.999999",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
         ];
-        for input in inputs {
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_date_time() {
-        let inputs = ["1979-05-27T07:32:00", "1979-05-27T00:32:00.999999"];
-        for input in inputs {
+        let inputs = [
+            (
+                "1979-05-27T07:32:00",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: None,
+                },
+            ),
+            (
+                "1979-05-27T00:32:00.999999",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_date() {
-        let inputs = ["1979-05-27", "2017-07-20"];
-        for input in inputs {
+        let inputs = [
+            (
+                "1979-05-27",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: None,
+                    offset: None,
+                },
+            ),
+            (
+                "2017-07-20",
+                Datetime {
+                    date: Some(Date {
+                        year: 2017,
+                        month: 7,
+                        day: 20,
+                    }),
+                    time: None,
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_time() {
-        let inputs = ["07:32:00", "00:32:00.999999"];
-        for input in inputs {
+        let inputs = [
+            (
+                "07:32:00",
+                Datetime {
+                    date: None,
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: None,
+                },
+            ),
+            (
+                "00:32:00.999999",
+                Datetime {
+                    date: None,
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 


Test patch:
diff --git a/crates/toml/tests/testsuite/serde.rs b/crates/toml/tests/testsuite/serde.rs
index 88990f23..8bbff56e 100644
--- a/crates/toml/tests/testsuite/serde.rs
+++ b/crates/toml/tests/testsuite/serde.rs
@@ -1050,3 +1050,11 @@ fn serialize_datetime_issue_333() {
     .unwrap();
     assert_eq!(toml, "date = 2022-01-01\n");
 }
+
+#[test]
+fn datetime_offset_issue_496() {
+    let original = "value = 1911-01-01T10:11:12-00:36\n";
+    let toml = original.parse::<toml::Table>().unwrap();
+    let output = toml.to_string();
+    snapbox::assert_eq(original, output);
+}