Changes to Islamic calculations:

- Fix continuity by allowing 353-length years and shifting excess days
- Change debug assertions to warnings
- Refactor for better debuggability

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"]

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<IB: IslamicBasedMarker>(extended_year: i32) -> Self {
         let ny = IB::fixed_from_islamic(extended_year, 1, 1);
         let packed_data = PackedIslamicYearInfo::compute_with_ny::<IB>(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<IB: IslamicBasedMarker>(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
         }
     }
 

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,31 +104,25 @@ 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,
             ));
         }
         let next_packed = self.data.get(delta + 1)?;
         let next_ny = next_packed.ny::<IB>(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.
 /// </div>
-#[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;

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);

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"]

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

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]

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)
     }