BurntSushi / jiff

A date-time library for Rust that encourages you to jump into the pit of success.
The Unlicense
1.74k stars 33 forks source link

Timestamp adds 1 day panic #90

Closed tisonkun closed 2 months ago

tisonkun commented 2 months ago
let current_date = Timestamp::now();
current_date + 1.day()

panic:

adding span to timestamp overflowed: Error { inner: ErrorInner { kind: Range(RangeError(Negative { what: "days", given: 1, min: -7304484, max: 7304484 })), cause: None } }
thread 'append::rolling_file::rotation::tests::test_next_date_timestamp' panicked at /Users/tison/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jiff-0.1.5/src/timestamp.rs:2233:31:
adding span to timestamp overflowed: Error { inner: ErrorInner { kind: Range(RangeError(Negative { what: "days", given: 1, min: -7304484, max: 7304484 })), cause: None } }
stack backtrace:

Code:

// Span
    #[inline(always)]
    pub(crate) fn smallest_non_time_non_zero_unit_error(
        &self,
    ) -> Option<Error> {
        if self.days != 0 {
            Some(t::SpanDays::error("days", self.days.get()))
        } else if self.weeks != 0 {
            Some(t::SpanWeeks::error("weeks", self.weeks.get()))
        } else if self.months != 0 {
            Some(t::SpanMonths::error("months", self.months.get()))
        } else if self.years != 0 {
            Some(t::SpanYears::error("years", self.years.get()))
        } else {
            None
        }
    }
tisonkun commented 2 months ago

Not sure why we have this check ..

BurntSushi commented 2 months ago

The panic is occurring here because you're using the + operator, which specifically panics when checked_add fails. So if you want to deal with error conditions gracefully, then you should use checked_add. And checked_add documents this error condition. You can't add days or greater units to a Timestamp because the meaning of "day" is ambiguous in this context.

BurntSushi commented 2 months ago

The error message here is pretty abysmal though. So we should improve that.