Polyfill: Fix extreme PlainYearMonth crash in non-ISO calendars#3279
Polyfill: Fix extreme PlainYearMonth crash in non-ISO calendars#3279
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3279 +/- ##
==========================================
- Coverage 97.96% 97.93% -0.04%
==========================================
Files 22 22
Lines 10519 10586 +67
Branches 1814 1827 +13
==========================================
+ Hits 10305 10367 +62
- Misses 197 201 +4
- Partials 17 18 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5be3fd4 to
875ce03
Compare
875ce03 to
74d2751
Compare
ptomato
left a comment
There was a problem hiding this comment.
This seems reasonable. But the approximate nature of the shifts for some calendars makes me uneasy, and I find it hard to reason about.
One alternative idea that I had, would be to extend the range beyond the legacy Date range more conservatively, staying only within the same month. e.g., if new Temporal.PlainDate(-271821, 4, 19, 'islamic-civil') gives { year: -280804, month: 3, day: 21 }, then we can manually calculate the calendar date for ISO dates between -271821-03-30 and -271821-04-18.
No idea if this would be sufficient to not crash in the bisection loop; maybe we need to bisect with dates outside of that, in which case we should probably go with the solution currently in this PR.
Replace isOutOfLegacyDateRange() with compareISODateToLegacyDateRange(), a three-way comparison returning -1/0/1. Rewrite clampISODate() to use it, removing the duplicated boundary conditions. Thanks to ptomato for the suggestion.
ptomato
left a comment
There was a problem hiding this comment.
I'd still like to try the approach I suggested, but given that we don't have a lot of time, better to merge this since it works and iterate later. Thanks again!
calendarToIsoDate()'s bisection loop can produce intermediate ISO dates beyond the legacyDateboundary, causingisoToCalendarDate()to fail viaIntl.DateTimeFormat.formatToParts().The fix is to override
isoToCalendarDatein affected calendar helpers to (1) shift out-of-range ISO dates by a calendar-appropriate cycle, (2) convert, then (3) adjust the year back. Another wrinkle is that Coptic/Ethiopian and Islamic calendars use day-based shifting with exact cycle lengths to avoid Julian–Gregorian drift; Indian uses year-based shifting since its months start at fixed Gregorian dates; Hebrew doesn't need an override since its extreme dates stay within range during bisection, though I'm not fully sure this holds in all cases.Fixes #3251