-
Notifications
You must be signed in to change notification settings - Fork 159
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
Conversation
Codecov Report
@@ 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. |
633a520
to
bc2853d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes:
- I think it makes sense to split the initial editorial commits out into a separate PR, to avoid rebase trouble as much as possible.
- At some point in the past you suggested using early errors to simplify this part of the grammar, for which I opened Use early errors to simplify ISO 8601 grammar #1984. Should that still apply?
- I think this normative change subsumes Normative: Remove "60" from TimeMinuteNotValidDay #2279 and Normative: Add "22" to
TimeHourNotValidMonth
#2280, so we could close those. @anba do you agree?
spec/abstractops.html
Outdated
TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour TimeZoneUTCOffsetMinute | ||
TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour `:` TimeZoneUTCOffsetMinute `:` TimeZoneUTCOffsetSecond TimeZoneUTCOffsetFraction? | ||
TimeZoneUTCOffsetSign TimeZoneUTCOffsetHour TimeZoneUTCOffsetMinute TimeZoneUTCOffsetSecond TimeZoneUTCOffsetFraction? | ||
TimeZoneUTCOffset but not `-` TimeZoneUTCOffsetHour |
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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 Notation («using 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|:
TimeZoneUTCOffset but not `-` TimeZoneUTCOffsetHour | |
[lookahead != `-` TimeZoneUTCOffsetHour] TimeZoneUTCOffset |
bc2853d
to
0d464c5
Compare
Done: #2337 (and rebased this PR on top of it)
Quite possibly. I'm in favor of any approach that gets this down to a small and comprehensible state. |
0d464c5
to
1187cf9
Compare
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 |
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.
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.
1187cf9
to
4af2c4a
Compare
This achieved consensus in July 2022. Tests are in tc39/test262#3641, so I think this is ready to go in. |
@ptomato is there any other pending PR to merge which will change the ISO8601 Grammar after this before the next TC39? |
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.
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.
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.
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 <[email protected]> Commit-Queue: Frank Tang <[email protected]> Cr-Commit-Position: refs/heads/main@{#83123}
Technically normative in adding support for edge case like the following:
(e.g. "112312[America/New_York]")
(e.g., "1001-12[-00:01]")
...but the polyfill already works like that.