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: Align ISO-8601 and non-ISO calendar behaviour when "year" is present #2863

Open
anba opened this issue May 27, 2024 · 5 comments

Comments

@anba
Copy link
Contributor

anba commented May 27, 2024

ISO-8601 calendar when year field is present:

js> Temporal.PlainMonthDay.from({calendar: "iso8601", year: 2023, month: 2, day: 30}).toString()
"02-28"
js> Temporal.PlainMonthDay.from({calendar: "iso8601", year: 2023, monthCode: "M02", day: 30}).toString()
"02-28"

day is constrained to 28, because the year 2023 is not a leap year.

Gregorian calendar as implemented in the spec polyfill:

js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, month: 2, day: 30}).toString()
"1972-02-28[u-ca=gregory]"
js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, monthCode: "M02", day: 30}).toString()
"1972-02-29[u-ca=gregory]"

Notice that when monthCode is present, year is ignored because day isn't constrained to 28, but instead set to 29.

Relevant abstract operations:

Neither CalendarResolveFields nor CalendarMonthDayToISOReferenceDate seem to require that year is taken into account, so these are also valid results:

js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, month: 2, day: 30}).toString()
"1972-02-29[u-ca=gregory]"
js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, monthCode: "M02", day: 30}).toString()
"1972-02-29[u-ca=gregory]"

These results can happen when first resolving year + month to a month code (here: "M02") and then resolving the month code and day pair.

@gibson042
Copy link
Collaborator

gibson042 commented May 27, 2024

Neither CalendarResolveFields nor CalendarMonthDayToISOReferenceDate seem to require that year is taken into account

CalendarMonthDayToISOReferenceDate does require taking year into account, but it is only mentioned for overflow "reject":

Like RegulateISODate, the operation throws a RangeError exception if overflow is "reject" and the month and day described by fields does not exist.
For example, when calendar is "gregory" and overflow is "reject", fields values of { monthCode: "M01", day: "32" } and { "year": 2001, "month": 2, "day": 29 } would both cause a RangeError to be thrown.
In the latter case, even though February 29 is a date in leap years of the Gregorian calendar, 2001 was not a leap year and a month code cannot be determined from the nonexistent date 2001-02-29 with the specified month index.

That paragraph should probably be expanded to also explicitly align with RegulateISODate when overflow is "constrain" as well, which would affect both monthCode and month input (and necessitate corresponding polyfill updates).

@justingrant
Copy link
Collaborator

My understanding (@ptomato correct me if I'm wrong) is that removing custom calendars in #2854 will make this problem moot, because we can remove ISOYear (for PMD) and ISODay (for PYM) from being observable. So in that case the year or day, respectively, of the inputs should not matter.

Philip is this correct?

@anba
Copy link
Contributor Author

anba commented May 28, 2024

My understanding (@ptomato correct me if I'm wrong) is that removing custom calendars in #2854 will make this problem moot, because we can remove ISOYear (for PMD) and ISODay (for PYM) from being observable. So in that case the year or day, respectively, of the inputs should not matter.

This about the day in a PlainMonthDay. When implementing CalendarMonthDayToISOReferenceDate in https://phabricator.services.mozilla.com/D211772, I've started with the case when monthCode is available, because that doesn't require to process any other relevant fields (era, eraYear, year, or month):

js> Temporal.PlainMonthDay.from({calendar: "gregory", monthCode: "M02", day: 30}).toString()
"1972-02-29[u-ca=gregory]"

The spec text doesn't mention if year / eraYear has to be validated when monthCode is present, so for now I'm simply ignoring it:

js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, monthCode: "M02", day: 30}).toString()
"1972-02-29[u-ca=gregory]"

I've compared this against the spec polyfill and saw that ignoring year seems to be okay.

Then I've added support for the case when month is present. The current patch for SpiderMonkey takes month and year / eraYear, computes the monthCode, and then reuses the code to resolve day with the newly computed monthCode to get the final PlainMonthDay.

js> Temporal.PlainMonthDay.from({calendar: "gregory", year: 2023, month: 2, day: 30}).toString()
"1972-02-29[u-ca=gregory]"

Notice that in all cases the result is "1972-02-29[u-ca=gregory]".

When I then compared the result for the last case against the spec polyfill, I've noticed that the spec polyfill returns "1972-02-28[u-ca=gregory]". (The polyfill returns "1972-02-29[u-ca=gregory]" for the other two cases.)

So now I'm unsure which results are actually correct?

Input Output
{calendar: "iso8601", monthCode: "M02", day: 30} "1972-02-29[u-ca=iso8601]"
{calendar: "iso8601", year: 2023, monthCode: "M02", day: 30} "1972-02-28[u-ca=iso8601]"
{calendar: "iso8601", month: 2, day: 30} "1972-02-29[u-ca=iso8601]"
{calendar: "iso8601", year: 2023, month: 2, day: 30} "1972-02-28[u-ca=iso8601]"
{calendar: "gregory", monthCode: "M02", day: 30} "1972-02-29[u-ca=gregory]"
{calendar: "gregory", year: 2023, monthCode: "M02", day: 30} ???
{calendar: "gregory", year: 2023, month: 2, day: 30} ???

@anba
Copy link
Contributor Author

anba commented May 29, 2024

@justingrant
Copy link
Collaborator

Meeting 2024-05-30: @gibson042 will make a PR matching his comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants