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

Normative: Improve the "NotAmbiguous" grammar #2284

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

gibson042
Copy link
Collaborator

Technically normative in adding support for edge case like the following:

  • HHMMSS[timeZoneName] where HHMMSS on its own is ambiguous with YYYYMM
    (e.g. "112312[America/New_York]")
  • HHMM-HH[timeZoneName] where HHMM-HH on its own is ambiguous with YYYY-MM
    (e.g., "1001-12[-00:01]")

...but the polyfill already works like that.

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #2284 (4af2c4a) into main (a4728e3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2284   +/-   ##
=======================================
  Coverage   95.03%   95.03%           
=======================================
  Files          20       20           
  Lines       10812    10812           
  Branches     1927     1927           
=======================================
  Hits        10275    10275           
  Misses        503      503           
  Partials       34       34           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gibson042 gibson042 force-pushed the 2022-06-improve-notambiguous branch from 633a520 to bc2853d Compare June 10, 2022 20:35
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes:

TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour TimeZoneUTCOffsetMinute
TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour `:` TimeZoneUTCOffsetMinute `:` TimeZoneUTCOffsetSecond TimeZoneUTCOffsetFraction?
TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour TimeZoneUTCOffsetMinute TimeZoneUTCOffsetSecond TimeZoneUTCOffsetFraction?
TimeZoneUTCOffset but not `-` TimeZoneUTCOffsetHour
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a valid way of expressing this? I don't think I saw any existing examples of "but not" followed by a sequence; only single literals, nonterminals, or "one of" (e.g., "but not one of \, ', or LineTerminator")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so based on Grammar Notationusing the phrase “but not” and then indicating the expansions to be excluded»), but if not then I think it could instead use a negative lookahead like |ForStatement|:

Suggested change
TimeZoneUTCOffset but not `-` TimeZoneUTCOffsetHour
[lookahead != `-` TimeZoneUTCOffsetHour] TimeZoneUTCOffset

@ptomato ptomato marked this pull request as draft June 26, 2022 05:43
@ptomato ptomato added normative Would be a normative change to the proposal and removed editorial labels Jun 26, 2022
@anba
Copy link
Contributor

anba commented Jun 27, 2022

I think this normative change subsumes #2279 and #2280, so we could close those. @anba do you agree?

Yes, agreed.

@gibson042
Copy link
Collaborator Author

I think it makes sense to split the initial editorial commits out into a separate PR, to avoid rebase trouble as much as possible.

Done: #2337 (and rebased this PR on top of it)

At some point in the past you suggested using early errors to simplify this part of the grammar, for which I opened #1984. Should that still apply?

Quite possibly. I'm in favor of any approach that gets this down to a small and comprehensible state.

@FrankYFTang
Copy link
Contributor

when do you plan to push this to the spec? I was working on a cl to sync the v8 impl to the spec text in https://chromium-review.googlesource.com/c/v8/v8/+/3822342 but then syg asked me to hold till you land the changes to grammar and sync with that. So it will be nice if you can land this (and other parser changes already agreeed in 2022-07 meeting) ASAP

@ptomato
Copy link
Collaborator

ptomato commented Aug 15, 2022

These issues are waiting for corresponding test262 tests. For now, I've prioritized the PRs affecting the grammar above the other ones.

Technically normative in adding support for edge case like the following:
* HHMMSS[timeZoneName] where HHMMSS on its own is ambiguous with YYYYMM
  (e.g. "112312[America/New_York]")
* HHMM-HH[timeZoneName] where HHMM-HH on its own is ambiguous with YYYY-MM
  (e.g., "1001-12[-00:01]")

...but the polyfill already works like that.
ptomato added a commit to ptomato/test262 that referenced this pull request Aug 15, 2022
This implements the normative change in
tc39/proposal-temporal#2284 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure strings like HHMM-UU[TZ] and HHMMSS[TZ] do not
require a disambiguating T separator, even if HHMM-UU and HHMMSS would by
themselves.
@ptomato ptomato force-pushed the 2022-06-improve-notambiguous branch from 1187cf9 to 4af2c4a Compare August 15, 2022 23:37
@ptomato
Copy link
Collaborator

ptomato commented Aug 15, 2022

This achieved consensus in July 2022. Tests are in tc39/test262#3641, so I think this is ready to go in.

@ptomato ptomato marked this pull request as ready for review August 15, 2022 23:41
@ptomato ptomato merged commit 5a84124 into tc39:main Aug 15, 2022
@FrankYFTang
Copy link
Contributor

@ptomato is there any other pending PR to merge which will change the ISO8601 Grammar after this before the next TC39?

@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

Yes, #2287 and #2345 are also awaiting tests.

Ms2ger pushed a commit to ptomato/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2284 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure strings like HHMM-UU[TZ] and HHMMSS[TZ] do not
require a disambiguating T separator, even if HHMM-UU and HHMMSS would by
themselves.
ptomato added a commit to ptomato/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2284 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure strings like HHMM-UU[TZ] and HHMMSS[TZ] do not
require a disambiguating T separator, even if HHMM-UU and HHMMSS would by
themselves.
ptomato added a commit to tc39/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2284 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure strings like HHMM-UU[TZ] and HHMMSS[TZ] do not
require a disambiguating T separator, even if HHMM-UU and HHMMSS would by
themselves.
pull bot pushed a commit to jamlee-t/v8 that referenced this pull request Sep 10, 2022
Add TimeHourMinuteBasicFormatNotAmbiguousWithMonthDay
TimeZoneNumericUTCOffsetNotAmbiguousWithDayOfMonth
TimeZoneNumericUTCOffsetNotAmbiguousWithMonth
TimeZoneIdentifier, UnpaddedHour, TimeZoneIANALegacyName productions.

Sync the spec of TemporalInstantString, TemporalTimeString
TimeZone, TimeZoneBracketedAnnotation, TemporalTimeZoneString,
ToTemporalTimeZone, TimeZoneIANAName productions.

Fix bug in ScanCalendarDateTimeTimeRequired, ToTemporalTimeZone

Change name from Handle<String> to Handle<Object> to hold undefined

Update parser tests accordingly.

Spec Text:
https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar
https://tc39.es/proposal-temporal/#sec-temporal-totemporaltimezone


Related PR changes:
tc39/proposal-temporal#2284
tc39/proposal-temporal#2287
tc39/proposal-temporal#2345


Bug: v8:11544
Change-Id: I6f1a5e5dedba461db9f36abe76fa97119c1f8c2c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3822342
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83123}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants