Use core::time::Duration via an enum

[esheppa]

Setting this up so it can run through the CI for now, this shouldn't be merged until at least version 0.5, or possibly never.

This is one option to use core::time::Duration in chrono's duration. The main change here is to remove the oldtime modules, and rename the Duration type to SignedDuration which can now be infallibly converted both ways with core::time::Duration (albeit via an absolute value when going to core::time::Duration)

pub enum SignedDuration {
    /// A duration heading in forwards in time
    Forwards(Duration),
    /// A duration heading in backwards in time
    Backwards(Duration),
}

[djc]

It's an interesting idea. Can you explain explicitly:

(a) what problems does this solve?
(b) which downsides do you see?
(c) are there alternative solutions to (a) that we should consider?

(Effectively, this is like a mini-RFC.)

[esheppa]

Thanks @djc - good idea re. mini-RFC. Worth noting here that I'll (hopefully shortly) set up another branch with a different approach using core::time::Duration only.

(a) what problems does this solve?

1. Making Duration::seconds and similar functions const #638 - however I'd need to replicate all the Add, Sub, etc impls with core::time::Duration or have them work with Into<SignedDuration> to get this, so it's somewhat debatable whether this helps.
2. Moves some code from chrono into core/std - less code to review/test/maintain
3. core::time::Duration => chrono::SignedDuration is now infallible, allowing APIs that work with SignedDuration to potentially be changed to Into<SignedDuration>
4. chrono::SignedDuration => core::time::Duration now has a lossy but infallible option
5. Disambiguates the name of the type from equivalents in core/std
6. Somewhat resolves Consider adding a "Default" implementation to Duration #717 - although removing the oldtime feature also resolves this
7. Resolves some longstanding issues: Conversion to/from Duration from std library. #58 and Completely migrate to core::time::Duration #497 and Add Serialize/Deserialize for Duration #117 and Provide access to subsecond nanos in the public interface of Duration. #154

(b) which downsides do you see?

• Risk of incorrect implementation or subtle semantic change that breaks existing code without a compile error (could be partially resolved by adding even more test coverage)
• Migration, when going from 0.4 to 0.5, all instances of chrono::Duration have to be renamed - however after a bulk rename is done code should compile and work as before, unless the implementation details of the original type were being relied upon.
• The total size of SignedDuration is larger than the previous chrono::Duration as it includes the enum tag as well
• Increase in MSRV

(c) are there alternative solutions to (a) that we should consider?

• Keep the chrono::Duration as is
• Use only core::time::Duration (this means removing or changing some APIs, so its more of a breaking change)
• Potential better solutions that people comment with when they see this PR :D

[esheppa]

Looks like this might also help with #613 / #586 - assuming we encourage users towards using core::time::Duration where possible, or otherwise change the return types on the SignedDuration methods

[Liamolucko]

This is just an opinion, but I think TimeOffset might be a better name, since a negative offset makes a bit more semantic sense than a negative duration.

[esheppa]

Thanks for the feedback @Liamolucko. There is also some discussion to be had whether we use core::time::Duration directly and don't allow for negative duration, however that may be too much of a breaking change. I'd originally planned to have two PRs but it is probably easier to just have the discussion on this one and only implement the other option if that looks to be better.

Having the add/sub methods take Into<chrono::Duration> means that at least cases where users wanted to use core::time::Duration (eg in constants or with serde) are solved by this

[esheppa]

@Liamolucko - what are your thoughts on TimeDifference or TemporalDifference, my thinking here is to disambiguate from the concept of Offset in relation to a timezone - although perhaps it might make sense to investigate harmonizing these instead.

One difference between the current Duration and FixedOffset is that FixedOffset's resolution is seconds, while Duration is nanoseconds.

[Liamolucko]

Those work, although they're a bit long. Maybe TimeDelta?

Another difference between this and FixedOffset is that FixedOffset can only be +/- 24 hours.

[esheppa]

TimeDelta is especially nice due to precedent so that sounds like a great choice to me!

Another difference between this and FixedOffset is that FixedOffset can only be +/- 24 hours.

Excellent point

[djc]

The TimeDelta name is great and makes it much clearer what this is for. I think I'm inclined to not make this an enum but just a simple struct Duration like we have today in oldtime.

[esheppa]

The advantage of it being an enum is that it internally stores the core::time::Duration which means we can delegate to all its trait implementations for derives (serde being an important one here) - it could also be a struct with a boolean for the direction.

However even if we were to move away from either of those two options, I'd argue that we should ensure that we have an impl From<core::time::Duration> for TimeDelta so that all the operations can use Into<TimeDelta> and most users can just use the core::time::Duration for their operations (which has const constructors)

[djc]

Hmm, I was thinking we should use TimeDelta as an output type but allow both TimeDelta and core::time::Duration as input types? Having impl Into<TimeDelta> as an argument all over the place has the potential of incurring a decent amount of compile-time overhead because we potentially have to compile all the functions twice (and it adds a layer of abstraction that makes things harder to understand).

[esheppa]

Makes sense, so we just duplicate the impls, once for core::time::Duration and once for TimeDelta?

[djc]

Where it makes sense, yes.

[esheppa]

ok - and do you have a preference between the below and using an enum?

struct TimeDelta {
    duration: core::time::Duration,
    forwards: bool,
}

[djc]

I guess it doesn't really matter -- which one do you think will make the code most readable?

[esheppa]

I think the enum is cleaner from a readability perspective - will get the type renamed to TimeDelta and the operations impls fixed, plus to a rebase

[esheppa]

Will rebase this again once 0.4.22 is released

[esheppa]

Hmm these test failures probably warrant more discussion. The fundamental issue stems from the core::time::Duration having seconds as u64 and the functions needing to return i64 for API compatibilty. One option is to change the functions to return i128 which would make it fully compatible with the MIN/MAX values of core::time::Duration, but the resulting i128 could be a bit of a pain to deal with

[djc]

Where do we need to return i64 for compatibility?

[esheppa]

So my plan here was to make the migration process as painless as possible. To do this I've tried to keep as many as possible of the function signatures the same, while changing the name and underlying implementation of the type (I could potentially even add back the fallible conversions for improved compatibility). The existing Duration::num_seconds method returns i64, so I'm trying to avoid returning something else like i128. Another way to solve it could be abs_seconds which returns a u64.

However, IMO there is a better way to solve this - which is that, realistically even i64 is a larger range of seconds than would ever be useful (except perhaps for physics use-cases, which might be better served by hifitime or some other custom implementation, probably using floating point numbers). My proposal would be to have some lower, sensible limits on TimeDelta MAX/MIN, and potentially equivalents on NaiveDate/NaiveDateTime/etc. For example:

• Define TimeDelta::MAX as i64::MAX milli/microseconds, or even a number less than this, and when converting from core::time::Duration then panic / error / truncate when out of range, meaning only num_nanoseconds needs to deal with i128/u128. (microseconds is my preference here as this allows all but num_nanoseconds to infallibly return i64 and still covers a very large range of years).
• Define NaiveDate or NaiveDateTime MIN/MAX, and then define TimeDelta's MIN/MAX in the context of that

This potentially has an effect on doing zero-copy de/serialization however as there are byte patterns that aren't valid for the type itself (eg underlying storage might be i64/u64, but some of the valid i64/u64 values wouldn't be valid for TimeDelta)

[djc]

So my plan here was to make the migration process as painless as possible. To do this I've tried to keep as many as possible of the function signatures the same, while changing the name and underlying implementation of the type (I could potentially even add back the fallible conversions for improved compatibility).

Hmm, not sure I agree with this plan. Given the fact that we have such a broad user base, we won't get to break API compatibility very often. I think 0.5 should be more correct/robust (fewer, and well-documented, panics) and expose a minimal API, even if that means migration would be more work. Given that 0.5 will get rid of the time 0.1 compatibility, I think willingness for maintained downstream packages to migrate will be reasonably high even in the face of substantial changes.

[esheppa]

I think 0.5 should be more correct/robust (fewer, and well-documented, panics) and expose a minimal API, even if that means migration would be more work.

Excellent, this sounds like a good approach!

Another thought - perhaps TimeDelta could look like the below, (and be renamed?) to be compatible with Months and Days (alternatively they could get their own *Delta types. This would allow it to share helper methods etc as well. We could potentially include Backwards and Forwards in the prelude as well. In this case it wouldn't have methods like num_seconds() etc, the user would need to get to the underlying core::time::Duration and ask that.

pub enum TimeDelta<D = core::time::Duration> {
    Forwards(D),
    Backwards(D),
}

impl<D: Copy> TimeDelta<D> {
    fn abs(self) -> D {
        todo!()   
    }
   fn forwards(self) -> Option<D> {
        todo!()  
   }
   fn backwards(self) -> Option<D> {
       todo!()    
  }
}

and use cases like:

// handle the direction
match my_datetime - my_other_datetime {
    TimeDelta::Forwards(d) => (),
    TimeDelta::Backwards(d) => (),
}

// just get the magnitude
(my_datetime - my_other_datetime).abs()

// handle the direction
// could be just `Delta` here?
match my_date - my_other_date {
    TimeDelta::Forwards(d) => (),
    TimeDelta::Backwards(d) => (),
}

// just get the magnitude
(my_date - my_other_date).abs()

// handle the direction
// could be just `Delta` here?
match my_date.signed_months_since(my_other_date) {
    TimeDelta::Forwards(d) => (),
    TimeDelta::Backwards(d) => (),
}

// just get the magnitude
my_date.signed_months_since(my_other_date).abs()

[djc]

Let's first do a version without generics and see where we feel a need for generics. My spidey sense is that there's too much complexity to justify the benefit but I don't have a good feel for the potential benefit right now.

[esheppa]

closing in favour of #824