Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

ahcodedthat
Copy link

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?

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate 😞.

src/datetime/mod.rs Show resolved Hide resolved
Copy link

codecov bot commented Feb 11, 2024

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.

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, very nice to get these improvements!

I only have some minor requests.

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

@pitdicker pitdicker Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this instead of an unwrap be an expect?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comments on overflow cases 👍

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)))
Copy link
Collaborator

@pitdicker pitdicker Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate 😞.

/// 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've removed it.

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djc Will probably also want to have a look.

let dur = e.duration();
let (sec, nsec) = (dur.as_secs() as i64, dur.subsec_nanos());
let sec: u64 = dur.as_secs();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

(-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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

type Error = OutOfRange;

fn try_from(dt: DateTime<Tz>) -> Result<SystemTime, OutOfRange> {
let sec: i64 = dt.timestamp();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

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())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue #1440.

@pitdicker pitdicker merged commit b129569 into chronotope:0.5.x Feb 14, 2024
36 of 37 checks passed
@ahcodedthat ahcodedthat deleted the systemtime-convert-no-panic branch February 14, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants