From c2d2cdd09d42e82a815f01950f05a6d59ac36b9d Mon Sep 17 00:00:00 2001
From: Dan Williams <dan@dotfive.co.uk>
Date: Sat, 27 Jan 2024 05:19:54 +0000
Subject: [PATCH] Added Duration::try_milliseconds()

  - Added a new Duration::try_milliseconds() function, to attempt to
    create a new milliseconds-based Duration, and return None if it
    fails, based on checking against MAX and MIN. Currently, failure can
    only happen for exactly one value, which is i64::MIN.

  - Updated Duration::milliseconds() to call try_milliseconds() and
    panic if None is returned. Although panicking in production code and
    especially library code is bad, this is in keeping with current
    Chrono behaviour. Note that this function is now no longer const.

  - Updated the Duration::milliseconds() documentation to make it clear
    that it now panics.

  - Added documentation to Duration::microseconds() and nanoseconds() to
    make it clear that they are infallible.

  - All tests now pass, including the one previously ignored.
---
 src/duration.rs | 48 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/duration.rs b/src/duration.rs
index 26d449eede..721bc63e3b 100644
--- a/src/duration.rs
+++ b/src/duration.rs
@@ -211,14 +211,44 @@ impl Duration {
     }
 
     /// Makes a new `Duration` with the given number of milliseconds.
+    ///
+    /// # Panics
+    ///
+    /// Panics when the duration is out of bounds, i.e. when the duration is
+    /// more than `i64::MAX` milliseconds or less than `-i64::MAX` milliseconds.
+    /// Notably, this is not the same as `i64::MIN`.
     #[inline]
-    pub const fn milliseconds(milliseconds: i64) -> Duration {
+    pub fn milliseconds(milliseconds: i64) -> Duration {
+        Duration::try_milliseconds(milliseconds).expect("Duration::milliseconds out of bounds")
+    }
+
+    /// Makes a new `Duration` with the given number of milliseconds.
+    ///
+    /// # Errors
+    ///
+    /// Returns `None` when the duration is more than `i64::MAX` milliseconds or
+    /// less than `-i64::MAX` milliseconds. Notably, this is not the same as
+    /// `i64::MIN`.
+    #[inline]
+    pub fn try_milliseconds(milliseconds: i64) -> Option<Duration> {
         let (secs, millis) = div_mod_floor_64(milliseconds, MILLIS_PER_SEC);
-        let nanos = millis as i32 * NANOS_PER_MILLI;
-        Duration { secs, nanos }
+        let d = Duration { secs, nanos: millis as i32 * NANOS_PER_MILLI };
+        // Technically we don't need to compare against MAX, as this function
+        // accepts an i64, and MAX is aligned to i64::MAX milliseconds. However,
+        // it is good practice to check it here in order to catch any future
+        // changes that might be made.
+        if d < MIN || d > MAX {
+            return None;
+        }
+        Some(d)
     }
 
     /// Makes a new `Duration` with the given number of microseconds.
+    ///
+    /// The number of microseconds acceptable by this constructor is far less
+    /// than the total number that can actually be stored in a `Duration`, so it
+    /// is not possible to specify a value that would be out of bounds. This
+    /// function is therefore infallible.
     #[inline]
     pub const fn microseconds(microseconds: i64) -> Duration {
         let (secs, micros) = div_mod_floor_64(microseconds, MICROS_PER_SEC);
@@ -227,6 +257,11 @@ impl Duration {
     }
 
     /// Makes a new `Duration` with the given number of nanoseconds.
+    ///
+    /// The number of nanoseconds acceptable by this constructor is far less
+    /// than the total number that can actually be stored in a `Duration`, so it
+    /// is not possible to specify a value that would be out of bounds. This
+    /// function is therefore infallible.
     #[inline]
     pub const fn nanoseconds(nanos: i64) -> Duration {
         let (secs, nanos) = div_mod_floor_64(nanos, NANOS_PER_SEC as i64);
@@ -709,19 +744,12 @@ mod tests {
             .is_none());
     }
     #[test]
-    #[ignore]
     #[should_panic(expected = "Duration::milliseconds out of bounds")]
     fn test_duration_milliseconds_min_underflow_panic() {
         // Here we ensure that trying to create a value one millisecond below the
         // minimum storable value will fail. This test is necessary because the
         // storable range is -i64::MAX, but the constructor type of i64 will allow
         // i64::MIN, which is one value below.
-        // WARNING:
-        // This test currently fails, because Duration::milliseconds() does not
-        // check against MIN, and hence allows an unsupported value to be created.
-        // Therefore it is currently marked as "ignore", until the fix is applied.
-        // This is because the current implementation of milliseconds() cannot
-        // actually panic.
         let _ = Duration::milliseconds(i64::MIN); // Same as -i64::MAX - 1
     }