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

|DateSpecYearMonth| and |DateSpecMonthDay| don't allow a calendar suffix #2379

Closed
gibson042 opened this issue Aug 16, 2022 · 11 comments · Fixed by #2428
Closed

|DateSpecYearMonth| and |DateSpecMonthDay| don't allow a calendar suffix #2379

gibson042 opened this issue Aug 16, 2022 · 11 comments · Fixed by #2428
Assignees
Labels
behavior Relating to behavior defined in the proposal has-consensus iso8601-grammar ISO 8061 grammar normative Would be a normative change to the proposal

Comments

@gibson042
Copy link
Collaborator

(spun off from #2285 (comment) )

It is negatively surprising that a calendar suffix is accepted when parsing a time (e.g., Temporal.PlainTime.from("0000[u-ca=iso8601]")) but not when parsing a year–month or month–day (e.g., Temporal.PlainYearMonth.from("202208[u-ca=iso8601]") or Temporal.PlainMonthDay.from("0816[u-ca=iso8601]")). There is good reason for those functions to be restricted to the ISO 8601 calendar because months and days don't align across calendars (e.g., August 2022 spans Hebrew months Av and Elul, and August 16 can fall on a wide variety of month–day combinations across different Hebrew years), but that should be a semantic restriction rather than a syntactic one—if their parsing utilizes the ISO 8601 calendar, then they should not reject input in which that is made explicit given that time parsing (in which calendar is irrelevant) does support the suffix.

It's fine to reject e.g. Temporal.PlainYearMonth.from("202208[u-ca=hebrew]"), but that should be done by u-ca value rather than presence.

@gibson042 gibson042 added behavior Relating to behavior defined in the proposal normative Would be a normative change to the proposal iso8601-grammar ISO 8061 grammar labels Aug 16, 2022
@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

I wasn't in favour of this originally, but coincidentally I spent some of today investigating what needs to change in the Temporal proposal due to the publication of the IETF draft (#1450) and I think supporting this fits with the idea that any RFC 3339 string should be able to be followed by any annotations.

So, I agree, it makes sense to allow Temporal.PlainYearMonth.from('202208[u-ca=iso8601]').

@ptomato ptomato self-assigned this Aug 31, 2022
ptomato added a commit that referenced this issue Sep 1, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations (time zone,
calendar, and unknown after #2397 lands) after the short YYYY-MM
PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This PR makes the following choices:

- UTC offset is allowed after MM-DD
- UTC offset is disallowed after YYYY-MM (even if it is positive, or
  contains a minute component, which would not be ambiguous)

Other choices are certainly possible.

Closes: #2379
ptomato added a commit that referenced this issue Sep 1, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations (time zone,
calendar, and unknown after #2397 lands) after the short YYYY-MM
PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This PR makes the following choices:

- UTC offset is allowed after MM-DD
- UTC offset is disallowed after YYYY-MM (even if it is positive, or
  contains a minute component, which would not be ambiguous)

Other choices are certainly possible.

Closes: #2379
ptomato added a commit that referenced this issue Sep 1, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations (time zone,
calendar, and unknown after #2397 lands) after the short YYYY-MM
PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This PR makes the following choices:

- UTC offset is allowed after MM-DD
- UTC offset is disallowed after YYYY-MM (even if it is positive, or
  contains a minute component, which would not be ambiguous)

Other choices are certainly possible.

Closes: #2379
@sffc
Copy link
Collaborator

sffc commented Sep 2, 2022

YearMonth and MonthDay have always been the weirdest parts of the Temporal data model.

@ptomato found in #2398 that there is ambiguity in the grammar, e.g. between YYYY-MM-UU (UTC offset) and YYYY-MM-DD.

It remains perfectly valid to parse YYYY-MM-DD into a YearMonth or a MonthDay.

I think it's therefore totally reasonable to classify YYYY-MM and MM-DD as extraordinarily special cases. They should be accepted only in their respective parsing functions, and they don't allow any extra ignorable information.

@justingrant
Copy link
Collaborator

I assume that it's a common use case for someone to specify a birthday or holiday along with the calendar of that month/day. Seems like a bad idea to prohibit that case.

Similarly, year/month combos with calendar info attached seem to be fairly common use cases, e.g. for displaying months in a calendar UI for a non-ISO calendar.

Can we solve this problem by restricting use of offsets to strings where there's a full YMD date present? So 2020-01-01-07 would be interpreted as 2020-01-01T00:00-07:00 but when parsing 2020-01-07 the 07 would always be interpreted as a day, not an offset, because an offset requires a full date to be present.

@ptomato
Copy link
Collaborator

ptomato commented Sep 6, 2022

The actual numbers in the strings can only be ISO years, months, and days, so a string like 04-04[u-ca=chinese] would be prohibited anyway. So I'm not sure the birthday use case would apply.

Disallowing an offset after YYYY-MM was the approach I took in the linked PR. I don't really have a strong opinion though. I would be fine with disallowing offsets after YYYY-MM and MM-DD or disallowing all annotations after YYYY-MM and MM-DD.

@justingrant
Copy link
Collaborator

a string like 04-04[u-ca=chinese] would be prohibited anyway. So I'm not sure the birthday use case would apply.

Oh, good point, you're correct. I forgot that we required a full ISO date for YM and MD cases because we need the full ISO date for those calendars.

So disallowing calendar annotations for YYYY-MM and MM-DD seems like a good idea to prevent bugs, unless we restrict acceptance of the calendar annotation to only the ISO calendar.

@sffc was that the point you were making above?

@ptomato
Copy link
Collaborator

ptomato commented Sep 6, 2022

Richard's proposal was that we allow annotation(s) to be present after YYYY-MM and MM-DD, but throw if the calendar is anything other than iso8601. (In terms of real changes, this would allow something likeTemporal.PlainMonthDay.from('0816+00:00[UTC][u-ca=iso8601]'), and disallow Temporal.PlainTime.from('0816+00:00[UTC][u-ca=iso8601]') because it would then require a T prefix in order to be a time.) Shane's proposal is that we don't allow annotations at all there, which is the status quo.

@sffc
Copy link
Collaborator

sffc commented Sep 7, 2022

My proposal is that we don't allow any annotations, not calendar, offset, or time zone, and not a T either. I don't know whether this is the status quo.

@ptomato
Copy link
Collaborator

ptomato commented Sep 7, 2022

OK, let's see if we can reach a conclusion online before next week's plenary, otherwise I will take this PR off the agenda and we'll discuss it again in a future champions meeting.

@sffc The status quo is indeed what your proposal is — currently the only accepted forms are YYYY-MM, YYYYMM, ±YYYYYY-MM, ±YYYYYYMM, MM-DD, MMDD, --MM-DD, --MMDD, nothing else.

@ptomato
Copy link
Collaborator

ptomato commented Sep 12, 2022

Our presentation is scheduled for today and we are no closer to a conclusion, so I'll take this PR off the docket for this plenary. Please be prepared to discuss it at an upcoming champions meeting.

@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2022

We discussed this in the champions meeting of 2022-09-15. The problem of YYYY-MM + UTC offset being ambiguous with days vanishes if we disallow UTC offset after YYYY-MM, and the problem of that being inconsistent vanishes if we just disallow UTC offset altogether if there is no time component.

I had previously thought that ISO 8601 allowed YYYY-MM-DDZ, but it actually doesn't define it; it doesn't forbid it either, but Z or UTC offset are not included in the "complete representation" of date-only representations. So, it is actually bad for Temporal to accept this unofficial variant of ISO 8601, especially if we're not doing anything with it. If in the future we wanted to add a ZonedDate type, we'd need to consider this syntax, but that is definitely not on the table for the current proposal.

The plan that we agreed on is now:

  • Disallow Z and ±HH:MM (UTC offset) unless directly following a HH:MM[:SS.SSS...] time component.
  • Allow time zone and calendar annotations after any ISO strings, including YYYY-MM, MM-DD, and any other places where Z and UTC offset are not allowed.

This would change a few things that might not be obvious, so I'll recap here with specific examples:

PlainYearMonth

  • 2022-09[Africa/Abidjan][u-ca=iso8601] - previously rejected, now accepted
  • 2022-09[u-ca=hebrew] - now syntactically valid, but still rejected because YYYY-MM syntax cannot be converted between calendars, so practically speaking no change
  • 2022-09+01:00, 2022-09Z - continues to be rejected

PlainMonthDay

  • 09-15[Asia/Katmandu][u-ca=iso8601] - previously rejected, now accepted
  • 09-15[u-ca=chinese] - now syntactically valid, but still rejected because MM-DD syntax cannot be converted between calendars, so practically speaking no change
  • 09-15+01:00, 09-15Z - continues to be rejected

PlainDate and PlainDateTime

  • 2022-09-15Z[Europe/Vienna] - becomes syntactically invalid, but was already rejected because Plain types are forbidden to take Z
  • 2022-09-15+01:00[Europe/Vienna] - previously accepted, now rejected

Instant

  • 2022-09-15Z, 2022-09-15-02:30 - previously accepted (time would default to midnight), now rejected
  • 2022-09-15T00Z, 2022-09-15T00-02:30 - now you have to use these instead of the above

ZonedDateTime

  • 2022-09-15Z[America/Whitehorse], 2022-09-15-02:30[America/St_Johns] - previously accepted (time would default to midnight), now rejected
  • 2022-09-15T00Z[America/Whitehorse], 2022-09-15T00-02:30[America/St_Johns] - now you have to use these instead of the above

@ptomato ptomato removed their assignment Sep 15, 2022
@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2022

I'll unassign myself as I won't be working on it immediately; I will pick it up again when I have time, but anyone else is welcome to take #2398 as a starting point in the meantime.

@ptomato ptomato self-assigned this Oct 21, 2022
ptomato added a commit that referenced this issue Oct 24, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations after the
short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This forced us to take another look at ISO 8601
and realize that YYYY-MM-DD-UU and YYYY-MM-DDZ are actually not defined by
ISO 8601. Although they are not forbidden either, but we don't have any
reason to introduce this notation given the set of Temporal types.
Therefore, UTC offsets are disallowed after any date-only notation (that
is, YYYY-MM-DD, YYYY-MM, and MM-DD.)

Closes: #2379
ptomato added a commit to ptomato/test262 that referenced this issue Oct 24, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to ptomato/test262 that referenced this issue Nov 29, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to ptomato/test262 that referenced this issue Nov 30, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to tc39/test262 that referenced this issue Nov 30, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit that referenced this issue Dec 1, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations after the
short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This forced us to take another look at ISO 8601
and realize that YYYY-MM-DD-UU and YYYY-MM-DDZ are actually not defined by
ISO 8601. Although they are not forbidden either, but we don't have any
reason to introduce this notation given the set of Temporal types.
Therefore, UTC offsets are disallowed after any date-only notation (that
is, YYYY-MM-DD, YYYY-MM, and MM-DD.)

Closes: #2379
ptomato added a commit that referenced this issue Dec 1, 2022
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations after the
short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This forced us to take another look at ISO 8601
and realize that YYYY-MM-DD-UU and YYYY-MM-DDZ are actually not defined by
ISO 8601. Although they are not forbidden either, but we don't have any
reason to introduce this notation given the set of Temporal types.
Therefore, UTC offsets are disallowed after any date-only notation (that
is, YYYY-MM-DD, YYYY-MM, and MM-DD.)

Closes: #2379
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue Jan 9, 2023
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations after the
short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This forced us to take another look at ISO 8601
and realize that YYYY-MM-DD-UU and YYYY-MM-DDZ are actually not defined by
ISO 8601. Although they are not forbidden either, but we don't have any
reason to introduce this notation given the set of Temporal types.
Therefore, UTC offsets are disallowed after any date-only notation (that
is, YYYY-MM-DD, YYYY-MM, and MM-DD.)

Closes: tc39#2379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal has-consensus iso8601-grammar ISO 8061 grammar normative Would be a normative change to the proposal
Projects
None yet
4 participants