Make conversions to and from `std::time::SystemTime` non-panicking #1421

ahcodedthat · 2024-02-09T23:43:10Z

As discussed in in #1049 (specifically here and here), this PR replaces the panicking From implementations between std::time::SystemTime and chrono::DateTime with non-panicking TryFrom implementations.

I see that #1410 introduces a new error type, but it has not yet landed, so this PR uses the existing OutOfRange error type. Should I open a PR against #1410's branch instead, and use the new error type?

pitdicker

Thank you for working on this!

I see that #1410 introduces a new error type, but it has not yet landed, so this PR uses the existing OutOfRange error type. Should I open a PR against #1410's branch instead, and use the new error type?

I prefer what you did here until our error work is a bit further along. It would be even better to target the 0.4 branch and get this improvements available earlier.

I did not do a good review yet, just an initial comment.

pitdicker · 2024-02-10T18:54:32Z

src/datetime/mod.rs

@@ -1536,42 +1538,67 @@ impl str::FromStr for DateTime<Local> {
 }

 #[cfg(feature = "std")]
-impl From<SystemTime> for DateTime<Utc> {
-    fn from(t: SystemTime) -> DateTime<Utc> {
+impl TryFrom<SystemTime> for DateTime<Utc> {


I think it is nice to already have this feature on the 0.4 branch (main). Do you want to target that branch, and add these implementations instead of replacing the existing ones?

I can't. impl From<T> and impl TryFrom<T> are mutually exclusive. The standard library has a blanket impl TryFrom<T> here that covers every impl From<T>.

That's unfortunate 😞.

src/datetime/mod.rs

codecov · 2024-02-11T05:41:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5d1dc65) 94.16% compared to head (cbbf354) 94.19%.
Report is 52 commits behind head on 0.5.x.

Additional details and impacted files

@@            Coverage Diff             @@
##            0.5.x    #1421      +/-   ##
==========================================
+ Coverage   94.16%   94.19%   +0.02%     
==========================================
  Files          34       37       +3     
  Lines       16824    16819       -5     
==========================================
  Hits        15842    15842              
+ Misses        982      977       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker

Good work, very nice to get these improvements!

I only have some minor requests.

pitdicker · 2024-02-12T18:43:49Z

src/offset/utc.rs

+    /// # ;
+    /// ```
+    ///
+    /// This will yield a <code>Result&lt;DateTime&lt;Utc&gt;, [OutOfRange]&gt;</code>,


Suggested change

/// This will yield a <code>Result<DateTime<Utc>, [OutOfRange]></code>,

/// This will yield a `Result<DateTime<Utc>, OutOfRange>`,

Per your later comment, I've gone ahead and removed this entirely, but in case you're wondering, I did this ugly HTML thing so that OutOfRange would be a link. As far as I know, the only way to make an intra-doc link inside an inline code element is by using HTML like that. It is admittedly ugly in source code, though.

pitdicker · 2024-02-12T18:47:16Z

src/offset/utc.rs

-        let naive =
-            NaiveDateTime::from_timestamp(now.as_secs() as i64, now.subsec_nanos()).unwrap();
-        Utc.from_utc_datetime(&naive)
+        SystemTime::now().try_into().unwrap()


Could this instead of an unwrap be an expect?

pitdicker · 2024-02-12T18:52:35Z

src/datetime/mod.rs

+                let sec = i64::try_from(dur.as_secs()).map_err(|_| OutOfRange::new())?;
+                let nsec = dur.subsec_nanos();
+                (sec, nsec)
+            }
            Err(e) => {
                // unlikely but should be handled


Because it wasn't obvious to me what was happening here: could you change the comment to say that SystemTime is before the UNIX_EPOCH and the error type contains the Duration that it is before?

pitdicker · 2024-02-12T18:53:45Z

src/datetime/mod.rs

                if nsec == 0 {
+                    // Overflow safety: `sec` was returned by `dur.as_secs()`, and is guaranteed to
+                    // be non-negative. Negating a non-negative signed integer cannot overflow.


Good comments on overflow cases 👍

pitdicker · 2024-02-12T18:59:40Z

src/datetime/mod.rs

-            UNIX_EPOCH - Duration::new(-sec as u64, 0) + Duration::new(0, nsec)
+            UNIX_EPOCH
+                .checked_sub(Duration::new(sec_abs, 0))
+                .and_then(|t| t.checked_add(Duration::new(0, nsec)))


I don't think this needs to be a checked_add. Doesn't harm either.

Ah, you're right. I've gone ahead and changed it.

pitdicker · 2024-02-12T19:01:36Z

src/datetime/mod.rs

        let nsec = dt.timestamp_subsec_nanos();
        if sec < 0 {
            // unlikely but should be handled
-            UNIX_EPOCH - Duration::new(-sec as u64, 0) + Duration::new(0, nsec)
+            UNIX_EPOCH
+                .checked_sub(Duration::new(sec_abs, 0))


Why not just Duration::new(-sec as u64, 0)?

If sec == i64::MIN and the compiler is configured to panic on overflow, then -sec will panic. I used sec.unsigned_abs() to avoid that.

I could replace let sec_abs with two separate invocations of sec.unsigned_abs(), if you'd like.

I missed the unsigned_ part. Very nice, better than something like checked_abs. Compliments for also catching this case (I used to know this, but would have made the mistake).

pitdicker · 2024-02-12T19:02:47Z

src/datetime/mod.rs

@@ -1536,42 +1538,67 @@ impl str::FromStr for DateTime<Local> {
 }

 #[cfg(feature = "std")]
-impl From<SystemTime> for DateTime<Utc> {
-    fn from(t: SystemTime) -> DateTime<Utc> {
+impl TryFrom<SystemTime> for DateTime<Utc> {


That's unfortunate 😞.

pitdicker · 2024-02-12T19:07:22Z

src/offset/utc.rs

+    /// that it is out of the range representable by `DateTime<Utc>`. It is assumed that this
+    /// crate will no longer be in use by that time.
+    ///
+    /// If panic in this situation is not acceptable, use [`SystemTime::now`] instead, like this:


I am not sure this is an advice we want to give in general, or want to give this prominent.

Ok, I've removed it.

pitdicker

@djc Will probably also want to have a look.

djc · 2024-02-13T08:52:11Z

src/datetime/mod.rs

                let dur = e.duration();
-                let (sec, nsec) = (dur.as_secs() as i64, dur.subsec_nanos());
+                let sec: u64 = dur.as_secs();


Please remove the type annotations here, which should be unnecessary for as_secs() and subsec_nanos(). You can use i64::try_from() for the conversion.

djc · 2024-02-13T08:52:36Z

src/datetime/mod.rs

-                    (-sec - 1, 1_000_000_000 - nsec)
+                    // Overflow safety: In addition to the above, `-x - 1`, where `x` is a
+                    // non-negative signed integer, also cannot overflow.
+                    let sec: i64 = -sec - 1;


Don't think the type annotations are needed here, either.

djc · 2024-02-13T08:53:09Z

src/datetime/mod.rs

+    type Error = OutOfRange;
+
+    fn try_from(dt: DateTime<Tz>) -> Result<SystemTime, OutOfRange> {
+        let sec: i64 = dt.timestamp();


djc · 2024-02-13T08:54:03Z

src/datetime/mod.rs

-            UNIX_EPOCH - Duration::new(-sec as u64, 0) + Duration::new(0, nsec)
+            // `dt` is before the Unix epoch.
+            let mut t =
+                UNIX_EPOCH.checked_sub(Duration::new(sec_abs, 0)).ok_or(OutOfRange::new())?;


If we're going to call OutOfRange::new(), should probably use ok_or_else() instead?

Ok, done.

I didn't think it would matter either way, since OutOfRange::new doesn't do anything at run time. 🤷‍♂️

djc · 2024-02-13T08:54:46Z

src/offset/utc.rs

-        let naive =
-            NaiveDateTime::from_timestamp(now.as_secs() as i64, now.subsec_nanos()).unwrap();
-        Utc.from_utc_datetime(&naive)
+        SystemTime::now().try_into().expect(


I think we'll want to make this fallible, but can leave this for a future PR.

Opened an issue #1440.

Related to chronotope#1049.

ahcodedthat mentioned this pull request Feb 9, 2024

Converting the API to return Results #1049

Closed

pitdicker reviewed Feb 10, 2024

View reviewed changes

pitdicker reviewed Feb 12, 2024

View reviewed changes

ahcodedthat force-pushed the systemtime-convert-no-panic branch from 4819eaf to da6fb12 Compare February 12, 2024 19:57

pitdicker approved these changes Feb 12, 2024

View reviewed changes

djc reviewed Feb 13, 2024

View reviewed changes

Make conversions to and from std::time::SystemTime non-panicking.

cbbf354

Related to chronotope#1049.

ahcodedthat force-pushed the systemtime-convert-no-panic branch from da6fb12 to cbbf354 Compare February 13, 2024 23:54

djc approved these changes Feb 14, 2024

View reviewed changes

pitdicker merged commit b129569 into chronotope:0.5.x Feb 14, 2024
36 of 37 checks passed

pitdicker mentioned this pull request Feb 14, 2024

Utc::now can fail with dates that are very far into the past/future #1440

Closed

ahcodedthat deleted the systemtime-convert-no-panic branch February 14, 2024 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make conversions to and from `std::time::SystemTime` non-panicking #1421

Make conversions to and from `std::time::SystemTime` non-panicking #1421

ahcodedthat commented Feb 9, 2024

pitdicker left a comment

pitdicker Feb 10, 2024

ahcodedthat Feb 11, 2024

pitdicker Feb 12, 2024

codecov bot commented Feb 11, 2024 •

edited

Loading

pitdicker left a comment

pitdicker Feb 12, 2024 •

edited

Loading

ahcodedthat Feb 12, 2024

pitdicker Feb 12, 2024

ahcodedthat Feb 12, 2024

pitdicker Feb 12, 2024

ahcodedthat Feb 12, 2024

pitdicker Feb 12, 2024

pitdicker Feb 12, 2024 •

edited

Loading

ahcodedthat Feb 12, 2024

pitdicker Feb 12, 2024

ahcodedthat Feb 12, 2024

pitdicker Feb 12, 2024

pitdicker Feb 12, 2024

pitdicker Feb 12, 2024

ahcodedthat Feb 12, 2024

pitdicker left a comment

djc Feb 13, 2024

ahcodedthat Feb 13, 2024

djc Feb 13, 2024

ahcodedthat Feb 13, 2024

djc Feb 13, 2024

ahcodedthat Feb 13, 2024

djc Feb 13, 2024

ahcodedthat Feb 13, 2024

djc Feb 13, 2024

pitdicker Feb 14, 2024

	/// This will yield a <code>Result<DateTime<Utc>, [OutOfRange]></code>,
	/// This will yield a `Result<DateTime<Utc>, OutOfRange>`,

Make conversions to and from std::time::SystemTime non-panicking #1421

Make conversions to and from std::time::SystemTime non-panicking #1421

Conversation

ahcodedthat commented Feb 9, 2024

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

pitdicker left a comment

Choose a reason for hiding this comment

pitdicker Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make conversions to and from `std::time::SystemTime` non-panicking #1421

Make conversions to and from `std::time::SystemTime` non-panicking #1421

codecov bot commented Feb 11, 2024 •

edited

Loading

pitdicker Feb 12, 2024 •

edited

Loading

pitdicker Feb 12, 2024 •

edited

Loading