unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.

https://icu4x.unicode.org

Other

1.37k stars 176 forks source link

Fix negative numbers across icu_calendar #2703

Closed sffc closed 5 months ago

sffc commented 2 years ago

Poking around, I keep finding places in icu_calendar where we do the wrong thing with negative numbers. We need to fix this, since one value proposition of ICU4X's calendar crate is that we generate correct results across the range required by Temporal.

I made a branch, iso-negative, with some test cases that are currently failing.

Actions on this ticket:

[x] Clone my branch and change the code so that the tests I added can all pass
[ ] Add tests to cover all operations that involve signed integers
[ ] Fix code that these tests reveal to be broken, perhaps by using the new div_rem_euclid helper function (#2704)

Related: #2151

CC @pt2121 @Manishearth @pandusonu2

Manishearth commented 2 years ago

I ended up fixing half of this whilst attempting to fix the coptic issue

Manishearth commented 2 years ago

Pretty sure that check(-1828, -5, 12, 31); is incorrect; I just ran the original lisp code and it wants it to be 30.

@sffc where did you get these fixed values from?

Manishearth commented 2 years ago

Oh, though it might be a zero-indexed vs 1-indexed difference, argh

Manishearth commented 2 years ago

Nope, we're both 1-indexed.

sffc commented 2 years ago

I calculated the fixed values manually.

Year 0: Fixed Days -1 through -366 (it is a leap year)
Year -1: Fixed Days -367 through -731
Year -2: Fixed Days -732 through -1096
Year -3: Fixed Days -1097 through -1461
Year -4: Fixed Days -1462 through -1827 (leap year)
Year -5: Fixed Days -1828 through -2192

Manishearth commented 2 years ago

Ah, there's your problem, fixed day 0 is not Jan 1 1 AD (that's fixed 1), it is Dec 31 1 BC

I fixed it in the tests

Manishearth commented 2 years ago

https://github.com/unicode-org/icu4x/issues/2703 fixes most of this, except for week_of.

I didn't really add many tests though. I think we ought to fuzz pre-https://github.com/unicode-org/icu4x/issues/2703 code and include those testcases.

atcupps commented 1 year ago

3477 handles Julian

Manishearth commented 1 year ago

@atcupps should this issue be closed now, or are there still bits left to be handled?

atcupps commented 1 year ago

Here are the calendars that have included negative date testing:

[x] Buddhist
[x] Chinese
[ ] Coptic
[x] Dangi
[ ] Ethiopian
[x] Gregorian
[x] Indian
[ ] Islamic
[x] Iso
[ ] Japanese
[x] Julian
[x] Persian

So there's still quite a few more to be done.

sffc commented 5 months ago

There are still bugs involving negative dates. See https://github.com/unicode-org/icu4x/pull/4894

A good next step would be to scrub the crate of any remaining % operations.