From 29febf07b26bb812dc52fdf1792873394d90898a Mon Sep 17 00:00:00 2001
From: Andrew Gallant <jamslam@gmail.com>
Date: Thu, 25 Jul 2024 20:03:33 -0400
Subject: [PATCH] timestamp: make saturating arithmetic panic for non-uniform
 units

The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
---
 COMPARE.md       |  2 +-
 src/timestamp.rs | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/COMPARE.md b/COMPARE.md
index 5c57c27d..ccb05936 100644
--- a/COMPARE.md
+++ b/COMPARE.md
@@ -1093,7 +1093,7 @@ use jiff::{Timestamp, ToSpan};
 fn main() -> anyhow::Result<()> {
     let ts = Timestamp::MAX;
     assert!(ts.checked_add(1.day()).is_err());
-    assert_eq!(ts.saturating_add(1.day()), ts);
+    assert_eq!(ts.saturating_add(1.hour()), ts);
 
     Ok(())
 }
diff --git a/src/timestamp.rs b/src/timestamp.rs
index 1f266d5b..45f5cbb8 100644
--- a/src/timestamp.rs
+++ b/src/timestamp.rs
@@ -1386,6 +1386,13 @@ impl Timestamp {
     /// Add the given span of time to this timestamp. If the sum would overflow
     /// the minimum or maximum timestamp values, then the result saturates.
     ///
+    /// # Panics
+    ///
+    /// This panics if the given `Span` contains any non-zero units greater
+    /// than hours. In `jiff 0.2`, this panic will be changed to an error. It
+    /// panics in `jiff 0.1` to avoid giving incorrect results. (It was an
+    /// oversight to make this method infallible.)
+    ///
     /// # Example
     ///
     /// This example shows that arithmetic saturates on overflow.
@@ -1404,6 +1411,12 @@ impl Timestamp {
     /// ```
     #[inline]
     pub fn saturating_add(self, span: Span) -> Timestamp {
+        if let Some(err) = span.smallest_non_time_non_zero_unit_error() {
+            panic!(
+                "saturing Timestamp arithmetic \
+                 requires only time units: {err}"
+            )
+        }
         self.checked_add(span).unwrap_or_else(|_| {
             if span.is_negative() {
                 Timestamp::MIN
@@ -1417,6 +1430,13 @@ impl Timestamp {
     /// would overflow the minimum or maximum timestamp values, then the result
     /// saturates.
     ///
+    /// # Panics
+    ///
+    /// This panics if the given `Span` contains any non-zero units greater
+    /// than hours. In `jiff 0.2`, this panic will be changed to an error. It
+    /// panics in `jiff 0.1` to avoid giving incorrect results. (It was an
+    /// oversight to make this method infallible.)
+    ///
     /// # Example
     ///
     /// This example shows that arithmetic saturates on overflow.
@@ -3087,6 +3107,18 @@ mod tests {
         assert_eq!(inst, got);
     }
 
+    #[test]
+    #[should_panic]
+    fn timestamp_saturating_add() {
+        Timestamp::MIN.saturating_add(Span::new().days(1));
+    }
+
+    #[test]
+    #[should_panic]
+    fn timestamp_saturating_sub() {
+        Timestamp::MAX.saturating_sub(Span::new().days(1));
+    }
+
     quickcheck::quickcheck! {
         fn prop_unix_seconds_roundtrip(t: Timestamp) -> quickcheck::TestResult {
             let dt = t.to_zoned(TimeZone::UTC).datetime();