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

PrepareCalendarFields: Replace TO-PRIMITIVE-AND-REQUIRE-STRING conversion #2962

Closed
anba opened this issue Sep 25, 2024 · 6 comments · Fixed by #2995
Closed

PrepareCalendarFields: Replace TO-PRIMITIVE-AND-REQUIRE-STRING conversion #2962

anba opened this issue Sep 25, 2024 · 6 comments · Fixed by #2995
Assignees
Labels
spec-text Specification text involved
Milestone

Comments

@anba
Copy link
Contributor

anba commented Sep 25, 2024

Just like "timeZone" is now eagerly validated using TO-TEMPORAL-TIME-ZONE-IDENTIFIER, also replace the TO-PRIMITIVE-AND-REQUIRE-STRING conversion with stricter checks:

This is a normative change but should fall into the "Remove calendars and time zone" bucket, which enabled further simplifications to PrepareCalendarFields.

@ptomato
Copy link
Collaborator

ptomato commented Sep 25, 2024

I'd prefer not to add more special treatments in PrepareCalendarFields, but we can put it on the agenda for the next Temporal champions call.

@anba
Copy link
Contributor Author

anba commented Sep 26, 2024

TO-PRIMITIVE-AND-REQUIRE-STRING is used for exactly two fields: "monthCode" and "offset". Replacing it with two new conversion methods doesn't really make things more complicated, IMO.

Early conversion also ensures bad inputs are caught early and more consistent. (More consistent because CalendarResolveFields doesn't specify in which order TypeError and RangeError should be thrown.) Implementation-wise it also seems better, because parsed "monthCode" and "offset" values can be stored as plain integers, whereas storing unparsed values as strings requires a bit more overhead.

@gibson042
Copy link
Collaborator

"M00L" is apparently valid per https://tc39.es/proposal-temporal/#table-temporal-calendar-date-record-fields?

Correct—«In a calendar with a leap month at the start of some years, the month code of that month would be "M00L"».

CalendarResolveFields doesn't specify in which order TypeError and RangeError should be thrown

That's a good point... we probably should specify that it generalizes the pattern of ISOResolveMonth (first checking to see if unset field(s) warrant a TypeError, then checking values to maybe throw a RangeError).

@anba
Copy link
Contributor Author

anba commented Sep 30, 2024

Correct—«In a calendar with a leap month at the start of some years, the month code of that month would be "M00L"».

Do you happen to know a calendar where a leap month can occur at the start of the year? It looks like the Hindu calendar can have leap months at the start of the year based on this (for the amanta system). But it's not clear (to me) that the current month code scheme actually works for the Hindu calendar, cf. tc39/proposal-intl-era-monthcode#18. Are there any other calendar systems with leap months at the start of the year?

@gibson042
Copy link
Collaborator

None that I know of (and particularly not in the Chinese calendar AFAIK), but the Temporal champions felt it wise to avoid a syntactic restriction because it is logically possible, and attested in at least some Hindu calendars.

@ptomato
Copy link
Collaborator

ptomato commented Oct 3, 2024

Meeting, 2024-10-03: We're OK to do this as long as it falls into the "Remove calendars and time zones" bucket. If anyone disagrees with that, please holler.

@ptomato ptomato added spec-text Specification text involved and removed meeting-agenda labels Oct 3, 2024
@ptomato ptomato added this to the Stage "3.5" milestone Oct 3, 2024
@ptomato ptomato self-assigned this Oct 3, 2024
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the calendar validate whether the month
code actually exists in the year.

See tc39/proposal-temporal#2962
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
…lity

Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the time zone validate whether the UTC
offset is correct for that exact time in the time zone.

See tc39/proposal-temporal#2962
ptomato added a commit that referenced this issue Oct 4, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
ptomato added a commit that referenced this issue Oct 4, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
Ms2ger pushed a commit to ptomato/test262 that referenced this issue Oct 7, 2024
Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the calendar validate whether the month
code actually exists in the year.

See tc39/proposal-temporal#2962
Ms2ger pushed a commit to ptomato/test262 that referenced this issue Oct 7, 2024
…lity

Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the time zone validate whether the UTC
offset is correct for that exact time in the time zone.

See tc39/proposal-temporal#2962
Ms2ger pushed a commit to tc39/test262 that referenced this issue Oct 7, 2024
Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the calendar validate whether the month
code actually exists in the year.

See tc39/proposal-temporal#2962
Ms2ger pushed a commit to tc39/test262 that referenced this issue Oct 7, 2024
…lity

Syntax is validated first. Only after the property bag is fully converted
into a Calendar Fields Record does the time zone validate whether the UTC
offset is correct for that exact time in the time zone.

See tc39/proposal-temporal#2962
ptomato added a commit that referenced this issue Oct 7, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
ptomato added a commit that referenced this issue Oct 7, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
This is a follow-up request from Anba, to the normative change in #2925.
It moves syntactic validation of month codes and offset strings into
PrepareCalendarFields. This allows implementations to store month codes
and offset strings as integers in their equivalents of Calendar Fields
Records, instead of allocated strings.

Closes: #2962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants