ACP: Add saturating artihmetic for `SystemTime`

[cvengler]

Proposal

Problem statement

The acception of #692 and the merge of rust-lang/rust#148825 led to the inclusion of a MAX and MIN constant to SystemTime.

Naturally it would only make sense to add saturating arithmetic to SystemTime as a follow-up to it, as it is already (partially) present in Instant and Duration, the other primitives found in the std::time module.

Next to the greater consistency with the standard library, a motivating factor for this is, that SystemTime::MAX and SystemTime::MIN are often times the most useful in context of saturating artihmetic, i.e. when handling failures of SystemTime::checked_add and SystemTime::checked_sub.
Instead of letting users repeat themselves, it is useful to provide this as part of the standard library.

Motivating examples or use cases

// Let `expiration` and `valid_after` be arbitrary.
let expiration: SystemTime = ...;
let valid_after: SystemTime = ...;

// Now calculate the lifetime for the object as expiration - valid_after.
let lifetime: Duration = expiration.duration_since(valid_after).unwrap_or(Duration::ZERO);

// Do some addition/subtraction as further examples.
let foo = expiration.checked_add(valid_after).unwrap_or(SystemTime::MAX);
let bar = expiration.checked_sub(valid_after).unwrap_or(SystemTime::MIN);

// Convert a SystemTime to a Unix timestamp
let timestamp: u64 = expiration.duration_since(SystemTime::UNIX_EPOCH).unwrap_or(Duration::ZERO).as_secs();

Solution sketch

// As part of std.
impl SystemTime {
    // ...

    fn saturating_add(&self, rhs: Duration) -> Self {
        self.checked_add(rhs).unwrap_or(SystemTime::MAX)
    }

    fn saturating_sub(&self, rhs: Duration) -> Self {
        self.checked_sub(rhs).unwrap_or(SystemTime::MIN)
    }

    fn saturating_duration_since(&self, rhs: Self) -> Duration {
        self.duration_since(rhs).unwrap_or(Duration::ZERO)
    }
}

Alternatives

With #![feature(time_systemtime_limits)]:
See Solution sketch above which effectively outlines a solution except that one may not extend SystemTime easily except with a custom (sealed) trait.

Without #![feature(time_systemtime_limits)]::
See the saturating-time crate which implements this outside of the standard library.
It has implements saturating_add, saturating_sub, and saturating_duration_since for SystemTime and Instant using a custom sealed trait.

Links and related work

#692
rust-lang/rust#148825
rust-lang/rust#71224
https://internals.rust-lang.org/t/instant-systemtime-min-max/21375

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[BurntSushi]

SystemTime::saturating_{add,sub} sound reasonable to me. As with the MIN and MAX constants, the saturating arithmetic methods should be explicitly documented such that the boundaries at which arithmetic saturates depend on the MIN and MAX constants, and thus are platform dependent.

I am less certain about SystemTime::saturating_duration_since though. What is the use case? This seems like a more subtle affair to me that could benefit from remaining a bit more explicit.

[cvengler]

I am less certain about SystemTime::saturating_duration_since though. What is the use case? This seems like a more subtle affair to me that could benefit from remaining a bit more explicit.

From my own experience, I tend to use a SystemTime::saturating_duration_since pedant when trying to convert a SystemTime into a Unix timestamp, such as the following:

let timestamp: SystemTime = ...;

let unix: u64 = timestamp.duration_since(SystemTime::UNIX_EPOCH).unwrap_or(Duration::ZERO).as_secs();

This often happens to me when interacting with databases for examples, where dates are stored in a u64.

[BurntSushi]

Yeah, but that is throwing away all timestamps before the Unix epoch. I understand that's the point of saturating here, but it strikes me as a bit too subtle. This is a very hand wavy concern though and I don't think I feel strongly about it. I'd be curious to hear from others on libs-api.

[cvengler]

Yeah, but that is throwing away all timestamps before the Unix epoch. I understand that's the point of saturating here, but it strikes me as a bit too subtle. This is a very hand wavy concern though and I don't think I feel strongly about it. I'd be curious to hear from others on libs-api.

That is a valid point you are making there.
From my own experience however, I think that throwing away earlier times is often an acceptable behavior.

There are indeed cases where this may not be acceptable but the Rust API is already expecting authors to think about this when deciding about whether to use a checking or a saturating method, regardless of the SystemTime context we are having here.
Similarly, there are also use-cases with types such as u64 or Duration where a saturation (and thereby throwing away parts) may not be acceptable behavior, yet the standard library still offers both ways: checking and saturating, because it is the responsibility of the author to decide what is best here depending on the use case.
For example, I would not use saturating_duration_since in my database example if I wouldn't ensure timestamps to be non-negative using db constraints.

Personally, I do not see a reason to offer checking arithmetic without offering saturating arithmetic as well when the respective limits exist, as the saturating version is oftentimes just an unwrap_or wrapper around the result of the checked version.

You can see this pattern already being applied in the Rust standard library.
Take Instant for example, which lacks a MAX and a MIN and therefore does not offer a saturating_add or saturating_sub, yet it offers a saturating_duration_since, because Duration has a lower limit.

The point I am trying to make is that checked arithmetic functions that return type T should have a saturating pedant when T has a defined upper or lower limit because it saves authors from implementing the unwrap_or anti-pattern(?) over and over again.

[BurntSushi]

Yes, but the unwrap_or in this case is kind of the point. I understand the consistency argument. I'm just pushing back a bit here because the actual semantic meaning of a saturating SystemTime::duration_since means that you're throwing away timestamps prior to the Unix epoch. That is a concern that applies to SystemTime very specifically, and not necessarily to Instant or Duration more broadly. That you have to write unwrap_or to explicitly knowledge the error case is arguably a good thing in this specific context.

I do agree that discarding negative timestamps may indeed be a not uncommon and valid thing to do. That, combined with the consistency argument is why I don't think I feel too strongly here.

[joshtriplett]

We discussed these in today's @rust-lang/libs-api meeting. We were broadly in favor of adding all three of these functions. We agreed with @BurntSushi's point that you often don't want to use saturating_duration_since (and generally felt like the right answer was often "go use a time library like jiff ;)"), but we also felt like these were still useful in enough cases to make them worth adding.

@BurntSushi, you said "I don't think I feel too strongly here"; can you confirm that you wouldn't object to adding these three methods, given appropriate comments on them for when they should and shouldn't be used?

[BurntSushi]

Yeah I think we can move forward here. I'm still a little hesitant on saturating_duration_since personally, but I don't feel strong enough to block it at this point. So I think we can mark this as accepted.

Please open a tracking issue and open a PR to rust-lang/rust to add it as an unstable feature. You can close this ACP once the tracking issue has been created.

[cvengler]

Very nice, thank you!
I intend to open the tracking issue alongside the implementation pull request before the week will end.

[cvengler]

See rust-lang/rust#151199