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

CalendarMonthDayToISOReferenceDate: Allow to throw RangeError for too large inputs? #3002

Closed
anba opened this issue Oct 7, 2024 · 4 comments · Fixed by #3008
Closed

CalendarMonthDayToISOReferenceDate: Allow to throw RangeError for too large inputs? #3002

anba opened this issue Oct 7, 2024 · 4 comments · Fixed by #3008

Comments

@anba
Copy link
Contributor

anba commented Oct 7, 2024

Implementations should be allowed to throw a RangeError for too large [[Year]] / [[EraYear]] values in CalendarMonthDayToISOReferenceDate, because performing field validation with arbitrarily large years is impossible to implement.

Example: This case requires validating that the first month has thirty days in the year 999_999_999.

Temporal.PlainMonthDay.from({year: 999_999_999, month: 1, day: 30, calendar: "chinese"}, {overflow: "reject"});
@ptomato
Copy link
Collaborator

ptomato commented Oct 8, 2024

With #2997 I think an implementation would be free to throw a RangeError here if the year was too large to verify the month-day fields, because it would throw anyway in the ISODateWithinRange step. But, sounds fine nonetheless. (never mind)

I'm less sure about this because how do we define "too large" per calendar? 999999999-01-30 may be impossible to verify in the Chinese calendar, but you can easily verify whether an arbitrarily large year has an 02-29 in the Gregorian calendar, as long as it's a safe integer. We could alternatively say that the year can be disregarded if it corresponds to an ISO year that's outside the range [-271821, 275760]?

@justingrant
Copy link
Collaborator

We could alternatively say that the year can be disregarded if it corresponds to an ISO year that's outside the range [-271821, 275760]?

By "disregarded" do you mean throw a RangeError?

@ptomato
Copy link
Collaborator

ptomato commented Oct 8, 2024

No, just ignored. So Anba's original example, {year: 999_999_999, month: 1, day: 30} would be treated as {month: 1, day: 30} and therefore throw because you would need a monthCode in that case. But {year: 999_999_999, monthCode: 'M01', day: 30} would succeed and give a PlainMonthDay of M01-30 if it exists in any year.

FWIW, now that I think about this more, I like it less, because it'd actually be weirder for the Gregorian calendar; you could pass {year: 999_999_999, monthCode: 'M02', day: 29, calendar: 'gregory' } and unexpectedly get M02-29 despite the year not being a leap year. So maybe we should just throw if the ISO year is outside the range, even if it'd still be possible for the calendar to verify the month and day.

@ptomato
Copy link
Collaborator

ptomato commented Oct 8, 2024

I'll prepare a speculative PR for what I described above, with throwing.

I think it should not apply to the ISO calendar. That's similar to what we have in CalendarResolveFields (see the note at the bottom of the operation) where we allow {month, day} without year only for the ISO calendar because it is a permanently stable "machine" calendar. (Also, if we wanted to apply it to the ISO calendar, I think that would be a normative change, and I don't think we should do that given that the practical issue Anba mentioned doesn't apply to the ISO calendar either.)

This should probably also have test262 tests.

ptomato added a commit that referenced this issue Oct 8, 2024
@ptomato ptomato closed this as completed in c75c841 Oct 9, 2024
anba added a commit to anba/proposal-temporal that referenced this issue Dec 9, 2024
Follow-up to tc39#3008 (tc39#3002). Similar to CalendarMonthDayToISOReferenceDate, also
don't require support for mapping ISO dates up to ±999999-12-31 to
calendar dates. This restriction does not apply to the ISO 8601
calendar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants