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

|TemporalTimeString| accepts ambiguous input with a calendar #2285

Closed
gibson042 opened this issue Jun 10, 2022 · 5 comments · Fixed by #2287
Closed

|TemporalTimeString| accepts ambiguous input with a calendar #2285

gibson042 opened this issue Jun 10, 2022 · 5 comments · Fixed by #2287
Assignees
Labels
behavior Relating to behavior defined in the proposal iso8601-grammar ISO 8061 grammar normative Would be a normative change to the proposal

Comments

@gibson042
Copy link
Collaborator

|TemporalTimeString| expands into either |CalendarTime| or |CalendarDateTimeTimeRequired|. |CalendarDateTimeTimeRequired| includes a mandatory date, separator, and time, and is therefore unambiguous. |CalendarTime| expands into one of three productions:

  1. The first starts with a mandatory |TimeDesignator| and is therefore unambiguous with PlainYearMonth and PlainMonthDay.
  2. The second starts with a mandatory |TimeSpec| and ends with a mandatory |Calendar|.
  3. The third expands to |TimeSpecWithOptionalTimeZoneNotAmbiguous|, which has constraints to ensure lack of ambiguity.

The issue is the second expansion of |CalendarTime|. Input to Temporal.PlainTime.from like "112312" fails to match any of the expansions and is rejected (presumably for ambiguity vs. a basic-format PlainYearMonth), as is input like "1123" (presumably for ambiguity vs. a basic-format PlainMonthDay). Suffixing either with a bracketed time zone or an unambiguous offset (e.g., "112312[+00:00]" or "1123-13") results in the third production matching and therefore acceptance, which is expected. But suffixing either with a calendar such as "[u-ca=iso8601]" results in the second production matching and therefore acceptance, which is negatively surprising because calendars are relevant to dates rather than times and fail to resolve the logical ambiguity (e.g., "112312[u-ca=iso8601]" is at least as plausibly a PlainYearMonth<1123-12> as it is a PlainTime<11:23:12>, and probably even more likely to be the former).

|CalendarTime| should be updated to require lack of ambiguity in all expansions.

@gibson042 gibson042 added the behavior Relating to behavior defined in the proposal label Jun 10, 2022
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Jun 10, 2022
@ptomato ptomato added the normative Would be a normative change to the proposal label Jun 24, 2022
@ptomato ptomato added the iso8601-grammar ISO 8061 grammar label Aug 11, 2022
@ptomato
Copy link
Collaborator

ptomato commented Aug 15, 2022

@gibson042 I went to write test262 tests for this, and on second thought I'm not sure anymore that it's correct.

PlainYearMonth and PlainMonthDay strings in the form of YYYY-MM and MM-DD are ISO-calendar only, so they couldn't have calendar annotations in the first place. For PlainYearMonth and PlainMonthDay with non-ISO calendars, you can't use the short form, and require the full ISO date with reference ISO day or reference ISO year, respectively. So, I now believe that HHMMSS[u-ca=CAL] and similar cannot be ambiguous with YYYYMM[u-ca=CAL] because the latter isn't accepted to begin with.

I think #2287 should be closed, but I could be getting something wrong in this analysis, what do you think?

@gibson042
Copy link
Collaborator Author

gibson042 commented Aug 16, 2022

#2287 fixes the absurdity of adding a calendar suffix (which is intended to provide supplemental information regarding days and larger units) resulting in otherwise invalid time strings being accepted—such a suffix could plausibly be rejected, but definitely shouldn't be treated as a signal that resolves potential ambiguity in favor of interpretation as a time.

And I would consider acceptance of "HHMMSS[u-ca=CAL]" as a valid time but rejection of "YYYYMM[u-ca=CAL]" as a valid year-month to be a separate absurdity meriting its own ticket—if a calendar suffix is valid when parsing a time, then it should definitely be valid when parsing a year-month or month-day (even if the identity of that calendar is required to be "iso8601" in those cases).

@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

In #1971 we decided never to emit MM-DD[u-ca=CAL] and YYYY-MM[u-ca=CAL] because they were not round-trippable — that we didn't resolve that question in favour of making them round-trippable implies that at least some of the champions didn't feel it was useful or necessary to accept those formats. Including me, so I'm not sure I agree that it's an absurdity.

But as for the main point, am I correctly paraphrasing you if I say that your point is that if the T is required in THHMMSS due to ambiguity, it should not be optional in THHMMSS[u-ca=CAL] despite that string not being valid for anything else, because it could still result in users misunderstanding what is being passed in as YYYYMM[u-ca=CAL]? I can get behind that.

@gibson042
Copy link
Collaborator Author

In #1971 we decided never to emit MM-DD[u-ca=CAL] and YYYY-MM[u-ca=CAL] because they were not round-trippable — that we didn't resolve that question in favour of making them round-trippable implies that at least some of the champions didn't feel it was useful or necessary to accept those formats. Including me, so I'm not sure I agree that it's an absurdity.

The absurdity is some inputs being accepted as a time only when followed by a date-relevant component. That's just not a good disambiguation signal, and furthermore is hostile to any future attempt to allow parsing year–month or month–day with explicit calendar (e.g., Temporal.PlainMonthDay.from("1123[u-ca=iso8601]")).

But as for the main point, am I correctly paraphrasing you if I say that your point is that if the T is required in THHMMSS due to ambiguity, it should not be optional in THHMMSS[u-ca=CAL] despite that string not being valid for anything else, because it could still result in users misunderstanding what is being passed in as YYYYMM[u-ca=CAL]? I can get behind that.

Yes, that is a correct logical implication of my position. 💯

@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

OK, thanks. I'll reply further on #2379.

ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Aug 16, 2022
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 iso8601-grammar ISO 8061 grammar normative Would be a normative change to the proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants