From dc2c760cd2ac94c823d1495cd6f3f388ad48f516 Mon Sep 17 00:00:00 2001 From: Paul Dicker Date: Thu, 28 Sep 2023 18:16:47 +0200 Subject: [PATCH 1/4] Use `overflowing_naive_local` in `checked_(add|sub)_months` --- src/datetime/mod.rs | 24 ++++++++++++++++-------- src/datetime/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/datetime/mod.rs b/src/datetime/mod.rs index 059f97d816..bb0f2e3871 100644 --- a/src/datetime/mod.rs +++ b/src/datetime/mod.rs @@ -436,13 +436,17 @@ impl DateTime { /// # Errors /// /// Returns `None` if: - /// - The resulting date would be out of range. /// - The local time at the resulting date does not exist or is ambiguous, for example during a /// daylight saving time transition. + /// - The resulting UTC datetime would be out of range. + /// - The resulting local datetime would be out of range (unless `months` is zero). #[must_use] - pub fn checked_add_months(self, rhs: Months) -> Option> { - self.naive_local() - .checked_add_months(rhs)? + pub fn checked_add_months(self, months: Months) -> Option> { + // `NaiveDate::checked_add_months` has a fast path for `Months(0)` that does not validate + // the resulting date, with which we can return `Some` even for an out of range local + // datetime. + self.overflowing_naive_local() + .checked_add_months(months)? .and_local_timezone(Tz::from_offset(&self.offset)) .single() } @@ -469,13 +473,17 @@ impl DateTime { /// # Errors /// /// Returns `None` if: - /// - The resulting date would be out of range. /// - The local time at the resulting date does not exist or is ambiguous, for example during a /// daylight saving time transition. + /// - The resulting UTC datetime would be out of range. + /// - The resulting local datetime would be out of range (unless `months` is zero). #[must_use] - pub fn checked_sub_months(self, rhs: Months) -> Option> { - self.naive_local() - .checked_sub_months(rhs)? + pub fn checked_sub_months(self, months: Months) -> Option> { + // `NaiveDate::checked_sub_months` has a fast path for `Months(0)` that does not validate + // the resulting date, with which we can return `Some` even for an out of range local + // datetime. + self.overflowing_naive_local() + .checked_sub_months(months)? .and_local_timezone(Tz::from_offset(&self.offset)) .single() } diff --git a/src/datetime/tests.rs b/src/datetime/tests.rs index be83955ee7..fcf6f6c404 100644 --- a/src/datetime/tests.rs +++ b/src/datetime/tests.rs @@ -1461,6 +1461,31 @@ fn test_min_max_setters() { assert_eq!(beyond_max.with_nanosecond(beyond_max.nanosecond()), Some(beyond_max)); } +#[test] +fn test_min_max_add_months() { + let offset_min = FixedOffset::west_opt(2 * 60 * 60).unwrap(); + let beyond_min = offset_min.from_utc_datetime(&NaiveDateTime::MIN); + let offset_max = FixedOffset::east_opt(2 * 60 * 60).unwrap(); + let beyond_max = offset_max.from_utc_datetime(&NaiveDateTime::MAX); + let max_time = NaiveTime::from_hms_nano_opt(23, 59, 59, 999_999_999).unwrap(); + + assert_eq!(beyond_min.checked_add_months(Months::new(0)), Some(beyond_min)); + assert_eq!( + beyond_min.checked_add_months(Months::new(1)), + Some(offset_min.from_utc_datetime(&(NaiveDate::MIN + Months(1)).and_time(NaiveTime::MIN))) + ); + assert_eq!(beyond_min.checked_sub_months(Months::new(0)), Some(beyond_min)); + assert_eq!(beyond_min.checked_sub_months(Months::new(1)), None); + + assert_eq!(beyond_max.checked_add_months(Months::new(0)), Some(beyond_max)); + assert_eq!(beyond_max.checked_add_months(Months::new(1)), None); + assert_eq!(beyond_max.checked_sub_months(Months::new(0)), Some(beyond_max)); + assert_eq!( + beyond_max.checked_sub_months(Months::new(1)), + Some(offset_max.from_utc_datetime(&(NaiveDate::MAX - Months(1)).and_time(max_time))) + ); +} + #[test] #[should_panic] fn test_local_beyond_min_datetime() { From 6f74488f2e504b1995acdc3cb16e3587193fdad9 Mon Sep 17 00:00:00 2001 From: Paul Dicker Date: Tue, 26 Sep 2023 12:36:55 +0200 Subject: [PATCH 2/4] Extend fast path in `NaiveDate::add_days` --- src/naive/date/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/naive/date/mod.rs b/src/naive/date/mod.rs index f26dba9e6e..f249baea91 100644 --- a/src/naive/date/mod.rs +++ b/src/naive/date/mod.rs @@ -699,10 +699,14 @@ impl NaiveDate { /// Add a duration of `i32` days to the date. pub(crate) const fn add_days(self, days: i32) -> Option { - // fast path if the result is within the same year + // Fast path if the result is within the same year. + // Also `DateTime::checked_(add|sub)_days` relies on this path, because if the value remains + // within the year it doesn't do a check if the year is in range. + // This way `DateTime:checked_(add|sub)_days(Days::new(0))` can be a no-op on dates were the + // local datetime is beyond `NaiveDate::{MIN, MAX}. const ORDINAL_MASK: i32 = 0b1_1111_1111_0000; if let Some(ordinal) = ((self.yof() & ORDINAL_MASK) >> 4).checked_add(days) { - if ordinal > 0 && ordinal <= 365 { + if ordinal > 0 && ordinal <= (365 + self.leap_year() as i32) { let year_and_flags = self.yof() & !ORDINAL_MASK; return Some(NaiveDate::from_yof(year_and_flags | (ordinal << 4))); } From 72e7df3b96def263e8deeabd3ff786c6c5fb20c1 Mon Sep 17 00:00:00 2001 From: Paul Dicker Date: Thu, 28 Sep 2023 17:45:13 +0200 Subject: [PATCH 3/4] Use `overflowing_naive_local` in `checked_(add|sub)_days` --- src/datetime/mod.rs | 31 +++++++++++++++++++++---------- src/datetime/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/datetime/mod.rs b/src/datetime/mod.rs index bb0f2e3871..8f8b8f3561 100644 --- a/src/datetime/mod.rs +++ b/src/datetime/mod.rs @@ -493,15 +493,22 @@ impl DateTime { /// # Errors /// /// Returns `None` if: - /// - The resulting date would be out of range. /// - The local time at the resulting date does not exist or is ambiguous, for example during a /// daylight saving time transition. + /// - The resulting UTC datetime would be out of range. + /// - The resulting local datetime would be out of range (unless `days` is zero). #[must_use] pub fn checked_add_days(self, days: Days) -> Option { - self.naive_local() - .checked_add_days(days)? - .and_local_timezone(TimeZone::from_offset(&self.offset)) - .single() + if days == Days::new(0) { + return Some(self); + } + // `NaiveDate::add_days` has a fast path if the result remains within the same year, that + // does not validate the resulting date. This allows us to return `Some` even for an out of + // range local datetime when adding `Days(0)`. + self.overflowing_naive_local() + .checked_add_days(days) + .and_then(|dt| self.timezone().from_local_datetime(&dt).single()) + .filter(|dt| dt <= &DateTime::::MAX_UTC) } /// Subtract a duration in [`Days`] from the date part of the `DateTime`. @@ -509,15 +516,19 @@ impl DateTime { /// # Errors /// /// Returns `None` if: - /// - The resulting date would be out of range. /// - The local time at the resulting date does not exist or is ambiguous, for example during a /// daylight saving time transition. + /// - The resulting UTC datetime would be out of range. + /// - The resulting local datetime would be out of range (unless `days` is zero). #[must_use] pub fn checked_sub_days(self, days: Days) -> Option { - self.naive_local() - .checked_sub_days(days)? - .and_local_timezone(TimeZone::from_offset(&self.offset)) - .single() + // `NaiveDate::add_days` has a fast path if the result remains within the same year, that + // does not validate the resulting date. This allows us to return `Some` even for an out of + // range local datetime when adding `Days(0)`. + self.overflowing_naive_local() + .checked_sub_days(days) + .and_then(|dt| self.timezone().from_local_datetime(&dt).single()) + .filter(|dt| dt >= &DateTime::::MIN_UTC) } /// Subtracts another `DateTime` from the current date and time. diff --git a/src/datetime/tests.rs b/src/datetime/tests.rs index fcf6f6c404..d465797329 100644 --- a/src/datetime/tests.rs +++ b/src/datetime/tests.rs @@ -1461,6 +1461,31 @@ fn test_min_max_setters() { assert_eq!(beyond_max.with_nanosecond(beyond_max.nanosecond()), Some(beyond_max)); } +#[test] +fn test_min_max_add_days() { + let offset_min = FixedOffset::west_opt(2 * 60 * 60).unwrap(); + let beyond_min = offset_min.from_utc_datetime(&NaiveDateTime::MIN); + let offset_max = FixedOffset::east_opt(2 * 60 * 60).unwrap(); + let beyond_max = offset_max.from_utc_datetime(&NaiveDateTime::MAX); + let max_time = NaiveTime::from_hms_nano_opt(23, 59, 59, 999_999_999).unwrap(); + + assert_eq!(beyond_min.checked_add_days(Days::new(0)), Some(beyond_min)); + assert_eq!( + beyond_min.checked_add_days(Days::new(1)), + Some(offset_min.from_utc_datetime(&(NaiveDate::MIN + Days(1)).and_time(NaiveTime::MIN))) + ); + assert_eq!(beyond_min.checked_sub_days(Days::new(0)), Some(beyond_min)); + assert_eq!(beyond_min.checked_sub_days(Days::new(1)), None); + + assert_eq!(beyond_max.checked_add_days(Days::new(0)), Some(beyond_max)); + assert_eq!(beyond_max.checked_add_days(Days::new(1)), None); + assert_eq!(beyond_max.checked_sub_days(Days::new(0)), Some(beyond_max)); + assert_eq!( + beyond_max.checked_sub_days(Days::new(1)), + Some(offset_max.from_utc_datetime(&(NaiveDate::MAX - Days(1)).and_time(max_time))) + ); +} + #[test] fn test_min_max_add_months() { let offset_min = FixedOffset::west_opt(2 * 60 * 60).unwrap(); From f23dcba0f5e6d15ef3e8a70d23ae842b1ca66a75 Mon Sep 17 00:00:00 2001 From: Paul Dicker Date: Thu, 28 Sep 2023 18:25:37 +0200 Subject: [PATCH 4/4] Make `with_year` return `Some` if it is a no-op --- src/datetime/mod.rs | 9 ++++++--- src/datetime/tests.rs | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/datetime/mod.rs b/src/datetime/mod.rs index 8f8b8f3561..5ade1fca34 100644 --- a/src/datetime/mod.rs +++ b/src/datetime/mod.rs @@ -1099,12 +1099,15 @@ impl Datelike for DateTime { /// # Errors /// /// Returns `None` if: - /// - The resulting date does not exist. - /// - When the `NaiveDateTime` would be out of range. /// - The local time at the resulting date does not exist or is ambiguous, for example during a /// daylight saving time transition. + /// - The resulting UTC datetime would be out of range. + /// - The resulting local datetime would be out of range (unless the year remains the same). fn with_year(&self, year: i32) -> Option> { - map_local(self, |datetime| datetime.with_year(year)) + map_local(self, |dt| match dt.year() == year { + true => Some(dt), + false => dt.with_year(year), + }) } /// Makes a new `DateTime` with the month number (starting from 1) changed. diff --git a/src/datetime/tests.rs b/src/datetime/tests.rs index d465797329..425618de04 100644 --- a/src/datetime/tests.rs +++ b/src/datetime/tests.rs @@ -1421,6 +1421,7 @@ fn test_min_max_setters() { let beyond_max = offset_max.from_utc_datetime(&NaiveDateTime::MAX); assert_eq!(beyond_min.with_year(2020).unwrap().year(), 2020); + assert_eq!(beyond_min.with_year(beyond_min.year()), Some(beyond_min)); assert_eq!(beyond_min.with_month(beyond_min.month()), Some(beyond_min)); assert_eq!(beyond_min.with_month(3), None); assert_eq!(beyond_min.with_month0(beyond_min.month0()), Some(beyond_min)); @@ -1441,6 +1442,7 @@ fn test_min_max_setters() { assert_eq!(beyond_min.with_nanosecond(0), Some(beyond_min)); assert_eq!(beyond_max.with_year(2020).unwrap().year(), 2020); + assert_eq!(beyond_max.with_year(beyond_max.year()), Some(beyond_max)); assert_eq!(beyond_max.with_month(beyond_max.month()), Some(beyond_max)); assert_eq!(beyond_max.with_month(3), None); assert_eq!(beyond_max.with_month0(beyond_max.month0()), Some(beyond_max));