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

Missing calendar validation in ToRelativeTemporalObject and ToTemporalZonedDateTime #2546

Closed
anba opened this issue Apr 12, 2023 · 3 comments
Assignees

Comments

@anba
Copy link
Contributor

anba commented Apr 12, 2023

ToRelativeTemporalObject, step 7.d. should probably be:

7.d. Set calendar to ? ToTemporalCalendarSlotValue(calendar, "iso8601").

Otherwise the calendar String value isn't validated to be a known, built-in calendar.

Also applies to ToTemporalZonedDateTime, step 6.j.

@justingrant
Copy link
Collaborator

Meeting 2023-04-13: This was a bug in the spec text of #2482 that wasn't caught before that PR was merged a few days ago, so we'll treat it as if it were a review comment on that PR and fix it now.

@ptomato
Copy link
Collaborator

ptomato commented Apr 13, 2023

I don't think we can use ToTemporalCalendarSlotValue here, because we should not accept all valid calendar string inputs as a value for a calendar annotation. For example, this proposed change would make the following code work:

Temporal.ZonedDateTime.from('2023-04-13T00[UTC][u-ca=2023-04-13]')

I think we can use IsBuiltinCalendar here instead, because result.[[Calendar]] should already be an identifier — we just need to make sure it's a known builtin one.

According to the current spec text, this already works for the other types, e.g.:

Temporal.PlainDate.from('2023-04-13[u-ca=2023-04-13]')

This is also a regression from #2482. So, the bug that needs to be fixed here is slightly more extensive than originally reported, but still doable.

@ptomato
Copy link
Collaborator

ptomato commented Apr 13, 2023

(The time zone annotation doesn't have this problem because an ISO string isn't a syntactically valid time zone identifier.)

@ptomato ptomato self-assigned this Apr 13, 2023
ptomato added a commit that referenced this issue Apr 14, 2023
In ToRelativeTemporalObject, ToTemporalDateTime, ToTemporalMonthDay,
ToTemporalYearMonth, and ToTemporalZonedDateTime, PR #2482 mistakenly left
out the validation of the calendar name when parsing a string.

The validation in ToTemporalDate was incorrect, because it should not have
accepted ISO strings as calendar identifiers, which
ToTemporalCalendarSlotValue would have, e.g.:

    Temporal.PlainDate.from('2023-04-13[u-ca=2023-04-13]')

(would return a PlainDate with the iso8601 calendar, instead of throwing)

Closes: #2546
ptomato added a commit that referenced this issue Apr 18, 2023
In ToRelativeTemporalObject, ToTemporalDateTime, ToTemporalMonthDay,
ToTemporalYearMonth, and ToTemporalZonedDateTime, PR #2482 mistakenly left
out the validation of the calendar name when parsing a string.

The validation in ToTemporalDate was incorrect, because it should not have
accepted ISO strings as calendar identifiers, which
ToTemporalCalendarSlotValue would have, e.g.:

    Temporal.PlainDate.from('2023-04-13[u-ca=2023-04-13]')

(would return a PlainDate with the iso8601 calendar, instead of throwing)

Closes: #2546
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