Skip to content

Commit 40b36de

Browse files
Fix Tabular behavior when estimating boundary years (#7057)
Fixes #7056 We have a case where the estimated year is two years off. I'm not sure if there's an underlying bug in the logic here. Perhaps the +1 in `extended_year` estimation code is wrong? --------- Co-authored-by: Robert Bastian <[email protected]>
1 parent 8f1579a commit 40b36de

File tree

1 file changed

+35
-17
lines changed

1 file changed

+35
-17
lines changed

components/calendar/src/cal/hijri.rs

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,7 @@ impl HijriYearData {
525525

526526
fn md_from_rd(self, rd: RataDie) -> (u8, u8) {
527527
let day_of_year = (rd - self.start_day()) as u16;
528+
528529
debug_assert!(day_of_year < 360 || !WELL_BEHAVED_ASTRONOMICAL_RANGE.contains(&rd));
529530
// We divide by 30, not 29, to account for the case where all months before this
530531
// were length 30 (possible near the beginning of the year)
@@ -732,28 +733,33 @@ impl<R: Rules> Calendar for Hijri<R> {
732733
}
733734

734735
fn from_rata_die(&self, rd: RataDie) -> Self::DateInner {
736+
// (354 * 30 + 11) / 30 is the mean year length for a tabular year
737+
// This is slightly different from the `calendrical_calculations::islamic::MEAN_YEAR_LENGTH`, which is based on
738+
// the (current) synodic month length.
739+
// This does not need to be accurate, it's just a performance optimisation
740+
// to avoid a long linear search. `Rules` also don't need to use this
741+
// epoch, it'll just search longer if the epoch is very different.
742+
//
735743
// +1 because the epoch is new year of year 1
736-
// truncating instead of flooring does not matter, as this is well-defined for
737-
// positive years only
738-
let extended_year = ((rd - calendrical_calculations::islamic::ISLAMIC_EPOCH_FRIDAY) as f64
739-
/ calendrical_calculations::islamic::MEAN_YEAR_LENGTH)
740-
as i32
741-
+ 1;
744+
// Before the epoch the division will round up (towards 0), so we need to
745+
// subtract 1, which is the same as not adding the 1.
746+
let extended_year = (rd - calendrical_calculations::islamic::ISLAMIC_EPOCH_FRIDAY) * 30
747+
/ (354 * 30 + 11)
748+
+ (rd > calendrical_calculations::islamic::ISLAMIC_EPOCH_FRIDAY) as i64;
742749

743-
let year = self.0.year_data(extended_year);
750+
let mut year = self.0.year_data(extended_year as i32);
744751

745-
let y = if rd < year.start_day() {
746-
self.0.year_data(extended_year - 1)
752+
if rd < year.start_day() {
753+
while rd < year.start_day() {
754+
year = self.0.year_data(year.extended_year - 1);
755+
}
747756
} else {
748-
let next_year = self.0.year_data(extended_year + 1);
749-
if rd < next_year.start_day() {
750-
year
751-
} else {
752-
next_year
757+
while rd >= year.start_day() + year.last_day_of_month(12) as i64 {
758+
year = self.0.year_data(year.extended_year + 1)
753759
}
754-
};
755-
let (m, d) = y.md_from_rd(rd);
756-
HijriDateInner(ArithmeticDate::new_unchecked(y, m, d))
760+
}
761+
let (m, d) = year.md_from_rd(rd);
762+
HijriDateInner(ArithmeticDate::new_unchecked(year, m, d))
757763
}
758764

759765
fn to_rata_die(&self, date: &Self::DateInner) -> RataDie {
@@ -1761,6 +1767,18 @@ mod test {
17611767
assert_eq!(dt.0.year.extended_year, -6823);
17621768
}
17631769

1770+
#[test]
1771+
fn test_regression_7056() {
1772+
// https://github.com/unicode-org/icu4x/issues/7056
1773+
let calendar = Hijri::new_tabular(
1774+
TabularAlgorithmLeapYears::TypeII,
1775+
TabularAlgorithmEpoch::Friday,
1776+
);
1777+
let iso = Date::try_new_iso(-62971, 3, 19).unwrap();
1778+
let _dt = iso.to_calendar(calendar);
1779+
let _dt = iso.to_calendar(Hijri::new_umm_al_qura());
1780+
}
1781+
17641782
#[test]
17651783
fn test_regression_5069_uaq() {
17661784
let calendar = Hijri::new_umm_al_qura();

0 commit comments

Comments
 (0)