Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ISO weekday calculations in negative years #4894

Merged
merged 2 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- `icu_calendar`
- New `DateTime::local_unix_epoch()` convenience constructor (https://github.com/unicode-org/icu4x/pull/4479)
- Improved approximation for Persian calendrical calculations (https://github.com/unicode-org/icu4x/issues/4713)
- Fix weekday calculations in negative ISO years (https://github.com/unicode-org/icu4x/pull/4894)
- `icu_datetime`
- `FormattedDateTime` and `FormattedZonedDateTime` now implement `Clone` and `Copy` (https://github.com/unicode-org/icu4x/pull/4476)
- `icu_locid`
Expand Down
15 changes: 14 additions & 1 deletion components/calendar/src/hebrew.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ impl<A: AsCalendar<Calendar = Hebrew>> DateTime<A> {
mod tests {

use super::*;
use crate::types::MonthCode;
use crate::types::{Era, MonthCode};
use calendrical_calculations::hebrew_keviyah::*;

// Sentinel value for Adar I
Expand Down Expand Up @@ -587,4 +587,17 @@ mod tests {
let yi = YearInfo::compute_for(88369);
assert_eq!(yi.keviyah.year_length(), 383);
}

#[test]
fn test_weekdays() {
// https://github.com/unicode-org/icu4x/issues/4893
let cal = Hebrew::new();
let era = "am".parse::<Era>().unwrap();
let month_code = "M01".parse::<MonthCode>().unwrap();
let dt = cal.date_from_codes(era, 3760, month_code, 1).unwrap();

// Should be Saturday per:
// https://www.hebcal.com/converter?hd=1&hm=Tishrei&hy=3760&h2g=1
assert_eq!(6, cal.day_of_week(&dt) as usize);
}
}
51 changes: 49 additions & 2 deletions components/calendar/src/iso.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ impl Calendar for Iso {

// The days of the week are the same every 400 years
// so we normalize to the nearest multiple of 400
let years_since_400 = date.0.year % 400;
let years_since_400 = date.0.year.rem_euclid(400);
debug_assert!(years_since_400 >= 0); // rem_euclid returns positive numbers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does, let's not assert std guarantees.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspected you might leave a comment like this :) ... I still have a slight preference for assertions on code invariants since as casts can be error-prone

let years_since_400 = years_since_400 as u32;
let leap_years_since_400 = years_since_400 / 4 - years_since_400 / 100;
// The number of days to the current year
// Can never cause an overflow because years_since_400 has a maximum value of 399.
Expand Down Expand Up @@ -169,7 +171,7 @@ impl Calendar for Iso {
}
};
let january_1_2000 = 5; // Saturday
let day_offset = (january_1_2000 + year_offset + month_offset + date.0.day as i32) % 7;
let day_offset = (january_1_2000 + year_offset + month_offset + date.0.day as u32) % 7;

// We calculated in a zero-indexed fashion, but ISO specifies one-indexed
types::IsoWeekday::from((day_offset + 1) as usize)
Expand Down Expand Up @@ -832,4 +834,49 @@ mod test {
check(-1439, 1969, 12, 31, 0, 1);
check(-2879, 1969, 12, 30, 0, 1);
}

#[test]
fn test_continuity_near_year_zero() {
// https://github.com/unicode-org/icu4x/issues/4893
const ONE_DAY_DURATION: DateDuration<Iso> = DateDuration {
years: 0,
months: 0,
weeks: 0,
days: 1,
marker: core::marker::PhantomData,
};

let mut iso_date = Date::try_new_iso_date(-10, 1, 1).unwrap();
let mut rata_die = iso_date.to_fixed();
let mut weekday = iso_date.day_of_week();

for _ in 0..(366 * 20) {
let next_iso_date = iso_date.added(ONE_DAY_DURATION);
let next_rata_die = next_iso_date.to_fixed();
assert_eq!(
next_rata_die,
rata_die + 1,
"{iso_date:?}..{next_iso_date:?}"
);
let next_weekday = next_iso_date.day_of_week();
assert_eq!(
(next_weekday as usize) % 7,
(weekday as usize + 1) % 7,
"{iso_date:?}..{next_iso_date:?}"
);
if iso_date.month().code.parsed().unwrap().0 == 2 && iso_date.day_of_month().0 == 28 {
assert_eq!(next_iso_date.is_in_leap_year(), iso_date.is_in_leap_year());
if iso_date.is_in_leap_year() {
assert_eq!(next_iso_date.month().code.parsed().unwrap().0, 2);
assert_eq!(next_iso_date.day_of_month().0, 29);
} else {
assert_eq!(next_iso_date.month().code.parsed().unwrap().0, 3);
assert_eq!(next_iso_date.day_of_month().0, 1);
}
}
iso_date = next_iso_date;
rata_die = next_rata_die;
weekday = next_weekday;
}
}
}
Loading