time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.09k stars 273 forks source link

time panicking with `local datetime out of valid range` from a `to_offset` #517

Closed mariari closed 1 year ago

mariari commented 1 year ago

I have some rust code that reliably produces local datetime out of valid range error

It seems it may be related to #384 and #514. This is reproducible for versions 0.3.13 all the way up to 0.3.16 oddly enough 0.3.11 is fine.

Code that Reproduced the error

the following code was made to create the error

#[test]
fn test() {
   let val = datetime!(9999-12-31 23:59:59 -00:00:01);
   let res: Result<TimeAgain,_> = val.try_into();
   std::println!("{:?}", val);
   assert_eq!(1,1)
}

#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
#[serde(try_from = "Timestamp", into = "Timestamp")]
pub struct TimeAgain(PrimitiveDateTime);

impl From<TimeAgain> for Timestamp {
    fn from(value: TimeAgain) -> Self {
        let t = value.0.assume_utc();
        let seconds = t.unix_timestamp();
        // Safe to convert to i32 because .nanosecond()
        // is guaranteed to return a value in 0..1_000_000_000 range.
        let nanos = t.nanosecond() as i32;
        Timestamp { seconds, nanos }
    }
}

impl TryFrom<OffsetDateTime> for TimeAgain {
    type Error = Error;

    fn try_from(t: OffsetDateTime) -> Result<TimeAgain, Error> {
        Self::from_utc(t.to_offset(offset!(UTC)))
    }
}

impl TryFrom<Timestamp> for TimeAgain {
    type Error = Error;

    fn try_from(value: Timestamp) -> Result<Self, Error> {
        let nanos = value
            .nanos
            .try_into()
            .map_err(|_| Error::timestamp_nanos_out_of_range())?;
        Self::from_unix_timestamp(value.seconds, nanos)
    }
}

impl TimeAgain {
    #[cfg(any(feature = "clock"))]
    pub fn now() -> Time {
        OffsetDateTime::now_utc().try_into().unwrap()
    }

    // Internal helper to produce a `Time` value validated with regard to
    // the date range allowed in protobuf timestamps.
    // The source `OffsetDateTime` value must have the zero UTC offset.
    fn from_utc(t: OffsetDateTime) -> Result<Self, Error> {
        debug_assert_eq!(t.offset(), offset!(UTC));
        match t.year() {
            1..=9999 => Ok(Self(PrimitiveDateTime::new(t.date(), t.time()))),
            _ => Err(Error::date_out_of_range()),
        }
    }
    pub fn from_unix_timestamp(secs: i64, nanos: u32) -> Result<Self, Error> {
        if nanos > 999_999_999 {
            return Err(Error::timestamp_nanos_out_of_range());
        }
        let total_nanos = secs as i128 * 1_000_000_000 + nanos as i128;
        match OffsetDateTime::from_unix_timestamp_nanos(total_nanos) {
            Ok(odt) => Self::from_utc(odt),
            _ => Err(Error::timestamp_conversion()),
        }
    }

}

The error causing lines

It seems the call in try_from, namely t.to_offset caused the error.

impl TryFrom<OffsetDateTime> for TimeAgain {
    type Error = Error;

    fn try_from(t: OffsetDateTime) -> Result<TimeAgain, Error> {
        t.to_offset(offset!(UTC));
        Self::from_utc(t.to_offset(offset!(UTC)))
    }
}

Error Message

thread 'time::tests::test' panicked at 'local datetime out of valid range', /home/taichi/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.16/src/offset_date_time.rs:128:13
jhpratt commented 1 year ago

Without looking over your code (it appears to be far from a minimal example), I am certain that this is the intended behavior. This is wholly unrelated to #514, which was actually filed in error, and #384 is somewhat along the right track.

Ultimately this traces back quite a ways to #307. If you look there, I initially resolved in favor of option 4, as I was unaware of a manner to resolve the potential conflict once tzdb is supported. However, a few months ago I ran across a solution (not relevant here), which led me to reconsider that decision. As such I changed the internal representation to option 3 in that issue. There are significant advantages. For end users, everything is significantly faster. For me, it will allow me (in a few weeks when MSRV permits) merge the internal representation of PrimitiveDateTime and OffsetDateTime, reducing the duplication between them by a fair amount.

Generally speaking, if you're up against the bounds of what is considered valid, panics aren't unexpected.