icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

Fix negative numbers across icu_calendar

Open sffc opened this issue 3 years ago • 11 comments
trafficstars

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

sffc avatar Oct 03 '22 02:10 sffc

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

Manishearth avatar Oct 03 '22 17:10 Manishearth

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 avatar Oct 03 '22 18:10 Manishearth

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

Manishearth avatar Oct 03 '22 18:10 Manishearth

Nope, we're both 1-indexed.

Manishearth avatar Oct 03 '22 18:10 Manishearth

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

sffc avatar Oct 03 '22 18:10 sffc

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 avatar Oct 03 '22 18:10 Manishearth

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.

Manishearth avatar Oct 03 '22 18:10 Manishearth

#3477 handles Julian

atcupps avatar Aug 01 '23 16:08 atcupps

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

Manishearth avatar Aug 01 '23 16:08 Manishearth

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.

atcupps avatar Aug 01 '23 16:08 atcupps

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.

sffc avatar May 14 '24 15:05 sffc