time-rs / time

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

Panic when `nth_next_occurrence` and `nth_prev_occurrence` get `n = 0` #629

Closed phisch closed 9 months ago

phisch commented 9 months ago

Both nth_next_occurrence and nth_prev_occurrence return a time::Date, but internally use a function that returns an Option<Date>.

This example causes a panic:

use time::{Date, Month};

fn main() {
    let date = Date::from_calendar_date(2023, Month::October, 16).unwrap();
    let nth_next = date.nth_next_occurrence(time::Weekday::Friday, 0);
}

Why?

This is because nth_next_occurrence returns Self instead of Option<Self>:

pub const fn nth_next_occurrence(self, weekday: Weekday, n: u8) -> Self {
    expect_opt!(
        self.checked_nth_next_occurrence(weekday, n),
        "overflow calculating the next occurrence of a weekday"
    )
}

while using checked_nth_next_occurence which returns Option<Self>:

pub(crate) const fn checked_nth_next_occurrence(self, weekday: Weekday, n: u8) -> Option<Self> {
    if n == 0 {
        return None;
    }

    const_try_opt!(self.checked_next_occurrence(weekday))
        .checked_add(Duration::weeks(n as i64 - 1))
}

Both functions should return Option<Self>.

phisch commented 9 months ago

I just saw that there is a test that expects this case to panic:

#[test]
#[should_panic]
fn nth_next_occurence_zeroth_occurence_test() {
    date!(2023 - 06 - 25).nth_next_occurrence(Weekday::Monday, 0);
}

So this is not a bug but expected behavior. Though I think it would be better to return None as the zeroth next occurrence instead.

jhpratt commented 9 months ago

The panicking behavior is documented, and changing the return type is a breaking change.