From c33971f8d8516eb9bd3cc12a229e815fb76c3af9 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 3 Oct 2022 14:32:04 -0700 Subject: [PATCH] Fix math in calendar (#2714) * Tests and initial work on ISO with negative years * Fix coptic division issue * fix test * add missing test Co-authored-by: Shane F. Carr --- components/calendar/Cargo.toml | 4 ++ components/calendar/benches/iso.rs | 20 ++++++ components/calendar/src/coptic.rs | 20 +++++- components/calendar/src/ethiopian.rs | 9 +++ components/calendar/src/helpers.rs | 14 ++++ components/calendar/src/iso.rs | 96 +++++++++++++++++++++++++--- components/calendar/src/julian.rs | 18 ++++-- 7 files changed, 164 insertions(+), 17 deletions(-) create mode 100644 components/calendar/benches/iso.rs diff --git a/components/calendar/Cargo.toml b/components/calendar/Cargo.toml index 8aa62fd9e56..9967975f005 100644 --- a/components/calendar/Cargo.toml +++ b/components/calendar/Cargo.toml @@ -63,6 +63,10 @@ harness = false name = "datetime" harness = false +[[bench]] +name = "iso" +harness = false + [[example]] name = "iso_date_manipulations" diff --git a/components/calendar/benches/iso.rs b/components/calendar/benches/iso.rs new file mode 100644 index 00000000000..184ba0ce2f6 --- /dev/null +++ b/components/calendar/benches/iso.rs @@ -0,0 +1,20 @@ +// This file is part of ICU4X. For terms of use, please see the file +// called LICENSE at the top level of the ICU4X source tree +// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). + +use criterion::{black_box, criterion_group, criterion_main, Criterion}; +use icu_calendar::{DateTime, Iso}; + +fn overview_bench(c: &mut Criterion) { + c.bench_function("iso/from_minutes_since_local_unix_epoch", |b| { + b.iter(|| { + for i in -250..250 { + let minutes = i * 10000; + DateTime::::from_minutes_since_local_unix_epoch(black_box(minutes)); + } + }); + }); +} + +criterion_group!(benches, overview_bench,); +criterion_main!(benches); diff --git a/components/calendar/src/coptic.rs b/components/calendar/src/coptic.rs index e616b3cc893..6facafae0ee 100644 --- a/components/calendar/src/coptic.rs +++ b/components/calendar/src/coptic.rs @@ -33,6 +33,7 @@ use crate::any_calendar::AnyCalendarKind; use crate::calendar_arithmetic::{ArithmeticDate, CalendarArithmetic}; +use crate::helpers::quotient; use crate::iso::Iso; use crate::julian::Julian; use crate::{types, Calendar, CalendarError, Date, DateDuration, DateDurationUnit, DateTime}; @@ -203,7 +204,7 @@ impl Coptic { fn fixed_from_coptic(date: ArithmeticDate) -> i32 { COPTIC_EPOCH - 1 + 365 * (date.year - 1) - + (date.year / 4) + + quotient(date.year, 4) + 30 * (date.month as i32 - 1) + date.day as i32 } @@ -219,8 +220,8 @@ impl Coptic { // Lisp code reference: https://github.com/EdReingold/calendar-code2/blob/1ee51ecfaae6f856b0d7de3e36e9042100b4f424/calendar.l#L1990 pub(crate) fn coptic_from_fixed(date: i32) -> CopticDateInner { - let year = (4 * (date - COPTIC_EPOCH) + 1463) / 1461; - let month = ((date - Self::fixed_from_coptic_integers(year, 1, 1)) / 30 + 1) as u8; // <= 12 < u8::MAX + let year = quotient(4 * (date - COPTIC_EPOCH) + 1463, 1461); + let month = (quotient(date - Self::fixed_from_coptic_integers(year, 1, 1), 30) + 1) as u8; // <= 12 < u8::MAX let day = (date + 1 - Self::fixed_from_coptic_integers(year, month, 1)) as u8; // <= days_in_month < u8::MAX #[allow(clippy::unwrap_used)] // day and month have the correct bounds @@ -321,3 +322,16 @@ fn year_as_coptic(year: i32) -> types::FormattableYear { } } } + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_coptic_regression() { + // https://github.com/unicode-org/icu4x/issues/2254 + let iso_date = Date::try_new_iso_date(-100, 3, 3).unwrap(); + let coptic = iso_date.to_calendar(Coptic); + let recovered_iso = coptic.to_iso(); + assert_eq!(iso_date, recovered_iso); + } +} diff --git a/components/calendar/src/ethiopian.rs b/components/calendar/src/ethiopian.rs index 503c9590de0..ff02b5515ab 100644 --- a/components/calendar/src/ethiopian.rs +++ b/components/calendar/src/ethiopian.rs @@ -425,4 +425,13 @@ mod test { Date::try_new_iso_date(1970, 1, 2).unwrap() ); } + + #[test] + fn test_roundtrip_negative() { + // https://github.com/unicode-org/icu4x/issues/2254 + let iso_date = Date::try_new_iso_date(-1000, 3, 3).unwrap(); + let ethiopian = iso_date.to_calendar(Ethiopian::new()); + let recovered_iso = ethiopian.to_iso(); + assert_eq!(iso_date, recovered_iso); + } } diff --git a/components/calendar/src/helpers.rs b/components/calendar/src/helpers.rs index 44e98d4a61a..0bf715ac396 100644 --- a/components/calendar/src/helpers.rs +++ b/components/calendar/src/helpers.rs @@ -13,6 +13,20 @@ pub fn div_rem_euclid(n: i32, d: i32) -> (i32, i32) { } } +/// Calculate `n / d` such that the remainder is always positive. +/// This is equivalent to quotient() in the Reingold/Dershowitz Lisp code +pub const fn quotient(n: i32, d: i32) -> i32 { + debug_assert!(d > 0); + // Code can use int_roundings once stabilized + // https://github.com/rust-lang/rust/issues/88581 + let (a, b) = (n / d, n % d); + if n >= 0 || b == 0 { + a + } else { + a - 1 + } +} + #[test] fn test_div_rem_euclid() { assert_eq!(div_rem_euclid(i32::MIN, 1), (-2147483648, 0)); diff --git a/components/calendar/src/iso.rs b/components/calendar/src/iso.rs index 08198fe7d29..d79f14bb844 100644 --- a/components/calendar/src/iso.rs +++ b/components/calendar/src/iso.rs @@ -31,6 +31,7 @@ use crate::any_calendar::AnyCalendarKind; use crate::calendar_arithmetic::{ArithmeticDate, CalendarArithmetic}; +use crate::helpers::{div_rem_euclid, quotient}; use crate::{types, Calendar, CalendarError, Date, DateDuration, DateDurationUnit, DateTime}; use tinystr::tinystr; @@ -313,9 +314,12 @@ impl DateTime { /// Convert minute count since 00:00:00 on Jan 1st, 1970 to ISO Date. /// + /// # Examples + /// /// ```rust /// use icu::calendar::DateTime; /// + /// // After Unix Epoch /// let today = DateTime::try_new_iso_datetime(2020, 2, 29, 0, 0, 0).unwrap(); /// /// assert_eq!(today.minutes_since_local_unix_epoch(), 26382240); @@ -324,10 +328,20 @@ impl DateTime { /// today /// ); /// + /// // Unix Epoch /// let today = DateTime::try_new_iso_datetime(1970, 1, 1, 0, 0, 0).unwrap(); /// /// assert_eq!(today.minutes_since_local_unix_epoch(), 0); /// assert_eq!(DateTime::from_minutes_since_local_unix_epoch(0), today); + /// + /// // Before Unix Epoch + /// let today = DateTime::try_new_iso_datetime(1967, 4, 6, 20, 40, 0).unwrap(); + /// + /// assert_eq!(today.minutes_since_local_unix_epoch(), -1440200); + /// assert_eq!( + /// DateTime::from_minutes_since_local_unix_epoch(-1440200), + /// today + /// ); /// ``` pub fn from_minutes_since_local_unix_epoch(minute: i32) -> DateTime { let (time, extra_days) = types::Time::from_minute_with_remainder_days(minute); @@ -371,9 +385,10 @@ impl Iso { // Calculate days per year let mut fixed: i32 = EPOCH - 1 + 365 * (date.0.year - 1); // Adjust for leap year logic - fixed += ((date.0.year - 1) / 4) - ((date.0.year - 1) / 100) + ((date.0.year - 1) / 400); + fixed += quotient(date.0.year - 1, 4) - quotient(date.0.year - 1, 100) + + quotient(date.0.year - 1, 400); // Days of current year - fixed += (367 * (date.0.month as i32) - 362) / 12; + fixed += quotient(367 * (date.0.month as i32) - 362, 12); // Leap year adjustment for the current year fixed += if date.0.month <= 2 { 0 @@ -414,19 +429,17 @@ impl Iso { // Lisp code reference: https://github.com/EdReingold/calendar-code2/blob/1ee51ecfaae6f856b0d7de3e36e9042100b4f424/calendar.l#L1191-L1217 fn iso_year_from_fixed(date: i32) -> i32 { + let date = date - EPOCH; // 400 year cycles have 146097 days - let n_400 = date / 146097; - let date = date % 146097; + let (n_400, date) = div_rem_euclid(date, 146097); // 100 year cycles have 36524 days - let n_100 = date / 36524; - let date = date % 36524; + let (n_100, date) = div_rem_euclid(date, 36524); // 4 year cycles have 1461 days - let n_4 = date / 1461; - let date = date % 1461; + let (n_4, date) = div_rem_euclid(date, 1461); - let n_1 = date / 365; + let n_1 = quotient(date, 365); let year = 400 * n_400 + 100 * n_100 + 4 * n_4 + n_1; @@ -454,7 +467,7 @@ impl Iso { } else { 2 }; - let month = ((12 * (prior_days + correction) + 373) / 367) as u8; // in 1..12 < u8::MAX + let month = quotient(12 * (prior_days + correction) + 373, 367) as u8; // in 1..12 < u8::MAX #[allow(clippy::unwrap_used)] // valid day and month let day = (date - Self::fixed_from_iso_integers(year, month, 1).unwrap() + 1) as u8; // <= days_in_month < u8::MAX #[allow(clippy::unwrap_used)] // valid day and month @@ -655,4 +668,67 @@ mod test { let offset = today.added(DateDuration::new(0, 1, 0, 1)); assert_eq!(offset, today_plus_1_month_1_day); } + + #[test] + fn test_iso_to_from_fixed() { + // Reminder: ISO year 0 is Gregorian year 1 BCE. + // Year 0 is a leap year due to the 400-year rule. + fn check(fixed: i32, year: i32, month: u8, day: u8) { + assert_eq!(Iso::iso_year_from_fixed(fixed), year, "fixed: {}", fixed); + assert_eq!( + Iso::iso_from_fixed(fixed), + Date::try_new_iso_date(year, month, day).unwrap(), + "fixed: {}", + fixed + ); + assert_eq!( + Iso::fixed_from_iso_integers(year, month, day), + Some(fixed), + "fixed: {}", + fixed + ); + } + check(-1828, -5, 12, 30); + check(-1827, -5, 12, 31); // leap year + check(-1826, -4, 1, 1); + check(-1462, -4, 12, 30); + check(-1461, -4, 12, 31); + check(-1460, -3, 1, 1); + check(-1459, -3, 1, 2); + check(-732, -2, 12, 30); + check(-731, -2, 12, 31); + check(-730, -1, 1, 1); + check(-367, -1, 12, 30); + check(-366, -1, 12, 31); + check(-365, 0, 1, 1); // leap year + check(-364, 0, 1, 2); + check(-1, 0, 12, 30); + check(0, 0, 12, 31); + check(1, 1, 1, 1); + check(2, 1, 1, 2); + check(364, 1, 12, 30); + check(365, 1, 12, 31); + check(366, 2, 1, 1); + check(1459, 4, 12, 29); + check(1460, 4, 12, 30); + check(1461, 4, 12, 31); // leap year + check(1462, 5, 1, 1); + } + + #[test] + fn test_from_minutes_since_local_unix_epoch() { + fn check(minutes: i32, year: i32, month: u8, day: u8, hour: u8, minute: u8) { + let today = DateTime::try_new_iso_datetime(year, month, day, hour, minute, 0).unwrap(); + assert_eq!(today.minutes_since_local_unix_epoch(), minutes); + assert_eq!( + DateTime::from_minutes_since_local_unix_epoch(minutes), + today + ); + } + + check(-1441, 1969, 12, 30, 23, 59); + check(-1440, 1969, 12, 31, 0, 0); + check(-1439, 1969, 12, 31, 0, 1); + check(-2879, 1969, 12, 30, 0, 1); + } } diff --git a/components/calendar/src/julian.rs b/components/calendar/src/julian.rs index 1f54039b812..8d599db05c4 100644 --- a/components/calendar/src/julian.rs +++ b/components/calendar/src/julian.rs @@ -33,6 +33,7 @@ use crate::any_calendar::AnyCalendarKind; use crate::calendar_arithmetic::{ArithmeticDate, CalendarArithmetic}; +use crate::helpers::quotient; use crate::iso::Iso; use crate::{types, Calendar, CalendarError, Date, DateDuration, DateDurationUnit, DateTime}; use core::marker::PhantomData; @@ -216,8 +217,8 @@ impl Julian { } else { date.year }; - let mut fixed: i32 = JULIAN_EPOCH - 1 + 365 * (year - 1) + (year - 1) / 4; - fixed += (367 * (date.month as i32) - 362) / 12; + let mut fixed: i32 = JULIAN_EPOCH - 1 + 365 * (year - 1) + quotient(year - 1, 4); + fixed += quotient(367 * (date.month as i32) - 362, 12); fixed += if date.month <= 2 { 0 } else if Self::is_leap_year_const(date.year) { @@ -250,7 +251,7 @@ impl Julian { // Lisp code reference: https://github.com/EdReingold/calendar-code2/blob/1ee51ecfaae6f856b0d7de3e36e9042100b4f424/calendar.l#L1711-L1738 fn julian_from_fixed(date: i32) -> JulianDateInner { - let approx = ((4 * date) + 1464) / 1461; + let approx = quotient((4 * date) + 1464, 1461); let year = if approx <= 0 { approx - 1 } else { approx }; let prior_days = date - Self::fixed_from_julian_integers(year, 1, 1); let correction = if date < Self::fixed_from_julian_integers(year, 3, 1) { @@ -260,7 +261,7 @@ impl Julian { } else { 2 }; - let month = ((12 * (prior_days + correction) + 373) / 367) as u8; // this expression is in 1..=12 + let month = quotient(12 * (prior_days + correction) + 373, 367) as u8; // this expression is in 1..=12 let day = (date - Self::fixed_from_julian_integers(year, month, 1) + 1) as u8; // as days_in_month is < u8::MAX #[allow(clippy::unwrap_used)] // day and month have the correct bounds @@ -407,4 +408,13 @@ mod test { let iso_expected_date = Date::try_new_iso_date(2022, 3, 1).unwrap(); assert_eq!(iso_date, iso_expected_date); } + + #[test] + fn test_roundtrip_negative() { + // https://github.com/unicode-org/icu4x/issues/2254 + let iso_date = Date::try_new_iso_date(-1000, 3, 3).unwrap(); + let julian = iso_date.to_calendar(Julian::new()); + let recovered_iso = julian.to_iso(); + assert_eq!(iso_date, recovered_iso); + } }