Adding Result/Error constructs to tzrs feature in spinoso-time

[b-n]

#1902 Added a new backend - however some of the idiomatic principles weren't followed. Specifically there are some From impls which can panic - These sorts of things should be TryFrom as an example.

This PR fixes a number of these issues. Specifically it aims to achieve these:

• ops would ideally be checked_add and checked_sub due to overflows
• Offset::fixed() should be fallible (range +/- 246060)
• Offset::from(&str) should be fallible
• Offset::from(&str) tests should also test out of bound Regexs (e.g. +HH:MMM, HHMM, /HH-MM, +HHH:MM, +HH::MM)
• Offset::from(&str) should not accept +0060, +2400 timezones. (they are out of range).
  - [ ] Attempt to unify the errors across chrono and tz-rs since things like ComponentOutOfRangeError might be sharable now (This makes more sense to try after we align some errors)
• Use the more idiomatic TimeError instead of TimeErr

[b-n]

@lopopolo I've done some digging, and it doesn't look like there are any "good" options for doing checked add on the datetimes. I was somewhat surprise there wasn't something like std::ops::CheckedAdd or anything similar. We can just add the Add trait, but something doesn't feel super right about this:

impl Add<T> for Time {
  type Output = Result<Self, Error>;
  fn try_from(value: T) -> Self::Output { }
}

This would end us in situations of

let now = Time::now();
let next = now + 1; // <-- next is a Result<Time, Error>, not a Time;

And I didn't try it until just now, but I see rust doesn't like function overloading (thank goodness), so this means just impl functions won't cut it.

My thoughts is implementing a trait:

pub trait CheckedAdd<T>: Sized {
    type Error;
    fn checked_add(value: T) -> Result<Self, Self::Error>;
}

Chrono currently uses num-traits for some of their additions of two times, however unfortuantely the implementation can only add the same type to the same type where as we are wanting to add a different type to another type.

Do you have any other bright[er] ideas than implementing this trait?

[lopopolo]

My thoughts is implementing a trait:

For these cases, I think we can just implement Time::checked_add and Time::checked_sub that take Time as regular methods. (and maybe similar methods that take f64, i64, and Duration?)

I don't think the complexity of a trait is worth it here, especially because in the trampolines for Time#add and similar, we do the unboxing to specific values, so if we are in the branch that unboxes a Float, we know we need to call Time::checked_add_f64, or something like that.

Then if we still want to implement std::ops::Add, the body of the trait impl can be time.checked_add(other).unwrap().

[b-n]

we can just implement Time::checked_add and Time::checked_sub that take Time

Funnily enough, it's not supported (in ruby 2.6.3, and also in 3.1.2):

irb(main):004:0> Time.at(1000) + Time.at(1000)
Traceback (most recent call last):
        6: from /home/ben/.rbenv/versions/2.6.3/bin/irb:23:in `<main>'
        5: from /home/ben/.rbenv/versions/2.6.3/bin/irb:23:in `load'
        4: from /home/ben/.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        3: from (irb):4
        2: from (irb):4:in `rescue in irb_binding'
        1: from (irb):4:in `+'
TypeError (time + time?)

A couple of options:

1. checked_add(value: Duration) - this is the type we can use for addition because it supports both seconds and nanos without float problems
2. checked_add_duration(value: Duration) - slighly more explicit version of the above

I'm in favour of checked_add(value: Duration) - e.g. we force rust people to think of adding durations by default. Do you have a pref?

Either way, documenting here for posterity, but i'll add checked_[add|sub]_[i8|i16|i32|i64|u8|u16|u32|u64] like we currently are.

[lopopolo]

yay table-driven tests.

[b-n]

hah yes! I was going to ask whether there is a style preference here. The same thing could be done with other tests and cheap hashmap tables (or a tuple lists i guess).

[lopopolo]

irb(main):004:0> Time.at(1000) + Time.at(1000)

Add but Time subtraction is supported!

[3.1.2] > Time.now - Time.now
=> -2.0e-06

A couple of options:

1. checked_add(value: Duration) - this is the type we can use for addition because it supports both seconds and nanos without float problems
2. checked_add_duration(value: Duration) - slighly more explicit version of the above

I'm in favor of this, but with the caveat that spinoso-time should probably provide some free functions which convert i64 and f64 to core::time::Duration. It would be not fun to implement all of that conversion code in the trampolines.

[lopopolo]

Either way, documenting here for posterity, but i'll add checked_[add|sub]_[i8|i16|i32|i64|u8|u16|u32|u64] like we currently are.

Let's impl these APIs in terms of Duration like you suggested, but make it easy for glue code in the trampolines to get a Duration from the primitive types. I guess that means these converter APIs would return Option<Duration> in case a negative value is passed?

So something like:

let float = ...;
let duration = spinoso_time::duration_from_f64(float)
    .or_else(|| {
        float.checked_neg().and_then(spinoso_time::duration_from_f64)
    })
    .ok_or_else(|| FloatDomainError::with_message("out of range")?;

Or maybe spinoso_time::duration_from_f64 returns an enum so callers can disambiguate when they need to call checked_add or checked_sub?

[b-n]

Let's impl these APIs in terms of Duration like you suggested, but make it easy for glue code in the trampolines to get a Duration from the primitive types. I guess that means these converter APIs would return Option in case a negative value is passed?

I've got one better I think (psuedo codeish):

fn checked_add(&self, value: Duration) -> Self {
  //magic
}

fn checked_add_f64(&self, value: f64) -> Self {
  let nanos = nanos_from_float(value);
  let duration = Duration::new(value.abs().floor(), nanos); 
  if value > 0 {
    self.checked_add(duration)
  } else {
    self.checked_sub(duration)
  }
}

[lopopolo]

this looks mostly good so far and I think you've got a solution to the ops problem in progress. I'm gonna step back on this PR. Once it's no longer draft, ping me and I'll take another look.

[b-n]

@lopopolo How do you feel about 58d30dc ?

(IMO, it needs a bit of work on the wording, but my brain is not turning up anything else noteworthy today)

[lopopolo]

can this enum derive Clone?

[b-n]

Not unless we make some PRs into tz-rs. the wrapped tz-rs errors don't implement it at present unfortunately.

[lopopolo]

is this expect guaranteed to not panic?

The docs for DateTime::find and its return type FoundDateTimeList indicate that the returned result may be empty:

https://docs.rs/tz-rs/latest/tz/datetime/struct.FoundDateTimeList.html

[b-n]

Yes, I think that's true only when using .unique after having a look at the underlying implementation. .latest will always return a result that we're able to work with luckily enough!

[lopopolo]

    /// Returns the latest found date time if existing
    pub fn latest(&self) -> Option<DateTime> {
        // Found date times are computed in ascending order of Unix times
        match *self.0.last()? {
            FoundDateTimeKind::Normal(date_time) => Some(date_time),
            FoundDateTimeKind::Skipped { after_transition, .. } => Some(after_transition),
        }
    }

that self.0.last()? means that if the vec in self.0 is empty, None is returned. We can leave this for now, but I'd love to maybe find a test case for this empty vec scenario.

[b-n]

Hah, yes, good point!

I'll do some more digging through the generation and the tests there, there must be a use case where it does indeed fail. If it exists, I will find it 😄

[lopopolo]

Two missing . in some doc comments but this is good to go! Thanks for pushing this through. This is a final approval, merge whenever you feel comfortable (you don't need to kick it back to me if you fix the typos).

🎉