diff --git a/Cargo.lock b/Cargo.lock index 91019133658..f01e2050bee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -296,6 +296,7 @@ version = "0.1.0" dependencies = [ "core_maths", "displaydoc", + "log", ] [[package]] @@ -942,6 +943,7 @@ dependencies = [ "icu_provider", "serde", "serde_json", + "simple_logger", "tinystr", "writeable", "zerovec", diff --git a/components/calendar/Cargo.toml b/components/calendar/Cargo.toml index 623b47c684b..3c490afb19f 100644 --- a/components/calendar/Cargo.toml +++ b/components/calendar/Cargo.toml @@ -39,6 +39,7 @@ icu = { path = "../../components/icu", default-features = false } icu_benchmark_macros = { path = "../../tools/benchmark/macros" } serde = { workspace = true, features = ["derive", "alloc"] } serde_json = { workspace = true } +simple_logger = { workspace = true } [target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies] @@ -47,6 +48,7 @@ criterion = { workspace = true } [features] default = ["compiled_data"] +logging = ["calendrical_calculations/logging"] std = ["icu_provider/std", "icu_locid/std", "calendrical_calculations/std"] serde = ["dep:serde", "zerovec/serde", "tinystr/serde", "icu_provider/serde"] datagen = ["serde", "dep:databake", "zerovec/databake", "tinystr/databake"] diff --git a/components/calendar/src/islamic.rs b/components/calendar/src/islamic.rs index 9252af9c213..534c44d7d3e 100644 --- a/components/calendar/src/islamic.rs +++ b/components/calendar/src/islamic.rs @@ -221,31 +221,34 @@ impl IslamicTabular { #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] pub(crate) struct IslamicYearInfo { packed_data: PackedIslamicYearInfo, - /// Is the previous year 355 days (short = 354) - prev_year_long: bool, + prev_year_length: u16, } impl IslamicYearInfo { pub(crate) const LONG_YEAR_LEN: u16 = 355; const SHORT_YEAR_LEN: u16 = 354; - pub(crate) fn new(prev_year_long: bool, packed_data: PackedIslamicYearInfo) -> Self { - Self { - prev_year_long, - packed_data, - } + pub(crate) fn new( + prev_packed: PackedIslamicYearInfo, + this_packed: PackedIslamicYearInfo, + extended_year: i32, + ) -> (Self, i32) { + let days_in_year = prev_packed.days_in_year(); + let year_info = Self { + prev_year_length: days_in_year, + packed_data: this_packed, + }; + (year_info, extended_year) } fn compute(extended_year: i32) -> Self { let ny = IB::fixed_from_islamic(extended_year, 1, 1); let packed_data = PackedIslamicYearInfo::compute_with_ny::(extended_year, ny); let prev_ny = IB::fixed_from_islamic(extended_year - 1, 1, 1); - let diff = u16::try_from(ny - prev_ny).unwrap_or(0); - debug_assert!( - diff == Self::SHORT_YEAR_LEN || diff == Self::LONG_YEAR_LEN, - "Found wrong year length for Islamic year {}: Expected 355 or 354, got {diff}", - extended_year - 1 - ); - Self::new(diff == Self::LONG_YEAR_LEN, packed_data) + let rd_diff = u16::try_from(ny - prev_ny).unwrap_or(0); + Self { + prev_year_length: rd_diff, + packed_data, + } } /// Get the new year R.D. given the extended year that this yearinfo is for fn new_year(self, extended_year: i32) -> RataDie { @@ -266,11 +269,7 @@ impl IslamicYearInfo { #[inline] fn days_in_prev_year(self) -> u16 { - if self.prev_year_long { - Self::LONG_YEAR_LEN - } else { - Self::SHORT_YEAR_LEN - } + self.prev_year_length } } @@ -848,9 +847,9 @@ impl CalendarArithmetic for IslamicCivil { fn days_in_provided_year(year: i32, _data: ()) -> u16 { if Self::is_leap_year(year, ()) { - 355 + IslamicYearInfo::LONG_YEAR_LEN } else { - 354 + IslamicYearInfo::SHORT_YEAR_LEN } } @@ -1092,9 +1091,9 @@ impl CalendarArithmetic for IslamicTabular { fn days_in_provided_year(year: i32, _data: ()) -> u16 { if Self::is_leap_year(year, ()) { - 355 + IslamicYearInfo::LONG_YEAR_LEN } else { - 354 + IslamicYearInfo::SHORT_YEAR_LEN } } diff --git a/components/calendar/src/provider/islamic.rs b/components/calendar/src/provider/islamic.rs index 3715ab2e87f..85e70bacb74 100644 --- a/components/calendar/src/provider/islamic.rs +++ b/components/calendar/src/provider/islamic.rs @@ -15,6 +15,7 @@ use crate::islamic::IslamicYearInfo; use calendrical_calculations::islamic::IslamicBasedMarker; use calendrical_calculations::rata_die::RataDie; +use core::fmt; use icu_provider::prelude::*; use zerovec::ule::{AsULE, ULE}; use zerovec::ZeroVec; @@ -78,12 +79,7 @@ impl<'data> IslamicCacheV1<'data> { return None; }; - let days_in_prev_year = prev_packed.days_in_year(); - - Some(IslamicYearInfo::new( - days_in_prev_year == IslamicYearInfo::LONG_YEAR_LEN, - this_packed, - )) + Some(IslamicYearInfo::new(prev_packed, this_packed, extended_year).0) } /// Get the cached data for the Islamic Year corresponding to a given day. /// @@ -108,11 +104,9 @@ impl<'data> IslamicCacheV1<'data> { if fixed < this_ny { let prev2_packed = self.data.get(delta - 2)?; - return Some(( - IslamicYearInfo::new( - prev2_packed.days_in_year() == IslamicYearInfo::LONG_YEAR_LEN, - prev_packed, - ), + return Some(IslamicYearInfo::new( + prev2_packed, + prev_packed, extended_year - 1, )); } @@ -120,19 +114,15 @@ impl<'data> IslamicCacheV1<'data> { let next_ny = next_packed.ny::(extended_year + 1); if fixed >= next_ny { - Some(( - IslamicYearInfo::new( - this_packed.days_in_year() == IslamicYearInfo::LONG_YEAR_LEN, - next_packed, - ), + Some(IslamicYearInfo::new( + this_packed, + next_packed, extended_year + 1, )) } else { - Some(( - IslamicYearInfo::new( - prev_packed.days_in_year() == IslamicYearInfo::LONG_YEAR_LEN, - this_packed, - ), + Some(IslamicYearInfo::new( + prev_packed, + this_packed, extended_year, )) } @@ -159,7 +149,7 @@ impl<'data> IslamicCacheV1<'data> { /// including in SemVer minor releases. While the serde representation of data structs is guaranteed /// to be stable, their Rust representation might not be. Use with caution. /// -#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, ULE)] +#[derive(Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, ULE)] #[cfg_attr( feature = "datagen", derive(serde::Serialize, databake::Bake), @@ -169,6 +159,15 @@ impl<'data> IslamicCacheV1<'data> { #[repr(packed)] pub struct PackedIslamicYearInfo(pub u8, pub u8); +impl fmt::Debug for PackedIslamicYearInfo { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { + fmt.debug_struct("PackedIslamicYearInfo") + .field("ny_offset", &self.ny_offset()) + .field("month_lengths", &self.month_lengths()) + .finish() + } +} + impl PackedIslamicYearInfo { pub(crate) fn new(month_lengths: [bool; 12], ny_offset: i8) -> Self { debug_assert!( @@ -193,6 +192,11 @@ impl PackedIslamicYearInfo { Self(le[0], le[1]) } + fn month_lengths(self) -> [u8; 12] { + let months: [u8; 12] = core::array::from_fn(|i| 1 + i as u8); + months.map(|x| if self.month_has_30_days(x) { 30 } else { 29 }) + } + // Get the new year offset from the mean synodic new year pub(crate) fn ny_offset(self) -> i8 { let masked = (self.1 >> 5) as i8; diff --git a/components/calendar/src/tests/continuity_test.rs b/components/calendar/src/tests/continuity_test.rs index 202cd5cd53b..955954906da 100644 --- a/components/calendar/src/tests/continuity_test.rs +++ b/components/calendar/src/tests/continuity_test.rs @@ -157,6 +157,8 @@ fn test_islamic_civil_continuity() { #[test] fn test_islamic_observational_continuity() { + #[cfg(feature = "logging")] + let _ = simple_logger::SimpleLogger::new().env().init(); let cal = crate::islamic::IslamicObservational::new(); let cal = Ref(&cal); let date = Date::try_new_observational_islamic_date(-10, 1, 1, cal); @@ -177,6 +179,8 @@ fn test_islamic_tabular_continuity() { #[test] fn test_islamic_umm_al_qura_continuity() { + #[cfg(feature = "logging")] + let _ = simple_logger::SimpleLogger::new().env().init(); let cal = crate::islamic::IslamicUmmAlQura::new(); let cal = Ref(&cal); let date = Date::try_new_ummalqura_date(-10, 1, 1, cal); diff --git a/components/datetime/Cargo.toml b/components/datetime/Cargo.toml index a8ca363dc43..d44a1eba61b 100644 --- a/components/datetime/Cargo.toml +++ b/components/datetime/Cargo.toml @@ -88,7 +88,8 @@ datagen = [ "icu_timezone/datagen", "serde", "std", - ] +] +logging = ["icu_calendar/logging"] experimental = ["dep:litemap"] bench = ["serde"] compiled_data = ["dep:icu_datetime_data", "dep:icu_locid_transform", "icu_calendar/compiled_data", "icu_decimal/compiled_data", "icu_plurals/compiled_data", "icu_timezone/compiled_data"] diff --git a/components/icu/Cargo.toml b/components/icu/Cargo.toml index 655bbddd77e..2d004033ca7 100644 --- a/components/icu/Cargo.toml +++ b/components/icu/Cargo.toml @@ -125,7 +125,7 @@ experimental = [ "dep:icu_experimental", ] sync = ["icu_provider/sync"] -logging = ["icu_provider/logging"] +logging = ["icu_provider/logging", "icu_datetime/logging"] [package.metadata.cargo-all-features] # Components are tested individually, and there's no logic in this crate diff --git a/utils/calendrical_calculations/Cargo.toml b/utils/calendrical_calculations/Cargo.toml index 2da8ce53bc7..33ae9e001c4 100644 --- a/utils/calendrical_calculations/Cargo.toml +++ b/utils/calendrical_calculations/Cargo.toml @@ -34,8 +34,10 @@ all-features = true [dependencies] core_maths = { workspace = true } displaydoc = { workspace = true } +log = { workspace = true, optional = true } [features] +logging = ["dep:log"] std = [] [package.metadata.cargo-all-features] diff --git a/utils/calendrical_calculations/src/islamic.rs b/utils/calendrical_calculations/src/islamic.rs index 515f8e98899..fcfe39818f1 100644 --- a/utils/calendrical_calculations/src/islamic.rs +++ b/utils/calendrical_calculations/src/islamic.rs @@ -21,6 +21,8 @@ const CAIRO: Location = Location { pub trait IslamicBasedMarker { /// The epoch of the calendar. Different calendars use a different epoch (Thu or Fri) due to disagreement on the exact date of Mohammed's migration to Mecca. const EPOCH: RataDie; + /// The name of the calendar for debugging. + const DEBUG_NAME: &'static str; /// Given the extended year, calculate the approximate new year using the mean synodic month fn mean_synodic_ny(extended_year: i32) -> RataDie { Self::EPOCH + (f64::from((extended_year - 1) * 12) * MEAN_SYNODIC_MONTH).floor() as i64 @@ -39,33 +41,61 @@ pub trait IslamicBasedMarker { /// Given an extended year, calculate whether each month is 29 or 30 days long fn month_lengths_for_year(extended_year: i32, ny: RataDie) -> [bool; 12] { + let next_ny = Self::fixed_from_islamic(extended_year + 1, 1, 1); + #[cfg(feature = "logging")] + if !matches!(next_ny - ny, 354 | 355) { + log::warn!( + "({}) Found year {extended_year} AH with length {}", + Self::DEBUG_NAME, + next_ny - ny + ); + } let mut prev_rd = ny; - let mut lengths = [false; 12]; - - for month_idx in 0..11 { - let new_rd = Self::fixed_from_islamic(extended_year, month_idx + 2, 1); + let mut excess_days = 0; + let mut lengths = core::array::from_fn(|month_idx| { + let month_idx = month_idx as u8; + let new_rd = if month_idx < 11 { + Self::fixed_from_islamic(extended_year, month_idx + 2, 1) + } else { + next_ny + }; let diff = new_rd - prev_rd; - debug_assert!( - // TODO: year 1409 has a 31-length month 1 - diff == 29 || diff == 30 || diff == 31, - "Found extended year {extended_year} with month length {diff} for month {}", - month_idx + 1 - ); - if new_rd - prev_rd >= 30 { - if let Some(l) = lengths.get_mut(usize::from(month_idx)) { - *l = true; + prev_rd = new_rd; + match diff { + 29 => false, + 30 => true, + 31 => { + #[cfg(feature = "logging")] + log::warn!( + "({}) Found year {extended_year} AH with month length {diff} for month {}", + Self::DEBUG_NAME, + month_idx + 1 + ); + excess_days += 1; + true + } + _ => { + debug_assert!( + false, + "({}) Found year {extended_year} AH with month length {diff} for month {}", + Self::DEBUG_NAME, + month_idx + 1 + ); + false } } - prev_rd = new_rd - } - let new_rd = Self::fixed_from_islamic(extended_year + 1, 1, 1); - let diff = new_rd - prev_rd; - debug_assert!( - diff == 29 || diff == 30, - "Found extended year {extended_year} with month length {diff} for month 12" - ); - if new_rd - prev_rd == 30 { - lengths[11] = true; + }); + // To maintain invariants for calendar arithmetic, if astronomy finds + // a 31-day month, "move" the day to the first 29-day month in the + // same year to maintain all months at 29 or 30 days. + if excess_days != 0 { + debug_assert_eq!( + excess_days, + 1, + "({}) Found year {extended_year} AH with more than one excess day", + Self::DEBUG_NAME + ); + lengths.iter_mut().find(|l| **l == false).map(|l| *l = true); } lengths } @@ -93,6 +123,7 @@ pub struct TabularIslamicMarker; impl IslamicBasedMarker for ObservationalIslamicMarker { const EPOCH: RataDie = FIXED_ISLAMIC_EPOCH_FRIDAY; + const DEBUG_NAME: &'static str = "ObservationalIslamic"; fn fixed_from_islamic(year: i32, month: u8, day: u8) -> RataDie { fixed_from_islamic_observational(year, month, day) } @@ -103,6 +134,7 @@ impl IslamicBasedMarker for ObservationalIslamicMarker { impl IslamicBasedMarker for SaudiIslamicMarker { const EPOCH: RataDie = FIXED_ISLAMIC_EPOCH_FRIDAY; + const DEBUG_NAME: &'static str = "SaudiIslamic"; fn fixed_from_islamic(year: i32, month: u8, day: u8) -> RataDie { fixed_from_saudi_islamic(year, month, day) } @@ -113,6 +145,7 @@ impl IslamicBasedMarker for SaudiIslamicMarker { impl IslamicBasedMarker for CivilIslamicMarker { const EPOCH: RataDie = FIXED_ISLAMIC_EPOCH_FRIDAY; + const DEBUG_NAME: &'static str = "CivilIslamic"; fn fixed_from_islamic(year: i32, month: u8, day: u8) -> RataDie { fixed_from_islamic_civil(year, month, day) } @@ -123,6 +156,7 @@ impl IslamicBasedMarker for CivilIslamicMarker { impl IslamicBasedMarker for TabularIslamicMarker { const EPOCH: RataDie = FIXED_ISLAMIC_EPOCH_THURSDAY; + const DEBUG_NAME: &'static str = "TabularIslamic"; fn fixed_from_islamic(year: i32, month: u8, day: u8) -> RataDie { fixed_from_islamic_tabular(year, month, day) }