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.

Sign up for GitHub

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

Jump to bottom

TimeFractionalPart in ParseISODateTime is ambigous #1794

Closed
FrankYFTang opened this issue Sep 2, 2021 · 1 comment · Fixed by #1957
Closed

TimeFractionalPart in ParseISODateTime is ambigous #1794

FrankYFTang opened this issue Sep 2, 2021 · 1 comment · Fixed by #1957
Assignees
@ptomato
Milestone
Next batch of nor...

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Sep 2, 2021 •
edited
Loading

In 13.34 ParseISODateTime ( isoString )
https://tc39.es/proposal-temporal/#sec-temporal-parseisodatetime

  1. Let year, month, day, hour, minute, second, fraction, and calendar be the parts of isoString produced respectively by the DateYear, DateMonth, DateDay, TimeHour, TimeMinute, TimeSecond, TimeFractionalPart, and CalendarName productions, or undefined if not present.

but the "TimeFractionalPart" is ambigous here.
Because TimeFractionalPart could be either from

  1. TemporalDateTimeString > CalendarDateTime > DateTime > TimeSpecSeparator > TimeSpec > TimeFraction > Fraction > TimeFractionalPart
    OR
  2. TemporalDateTimeString > CalendarDateTime > DateTime > TimeZone > TimeZoneOffsetRequired > TimeZoneUTCOffset > TimeZoneNuermicUTCOffset > TimeZoneUTCOffsetFraction > Fraction > TimeFractionalPart
    OR
  3. TemporalDateTimeString > CalendarDateTime > DateTime > TimeZone > TimeZoneOffsetRequired > TimeZoneBracketedAnnotation > TimeZoneBracketedName > TimeZoneUTCOffsetName > Fraction > TimeFractionalPart

For example, we may have the following string valid of TemporalDateTimeString syntax
"2021-09-01T02:03:04.56789+23:12:07.987654321[+11:22:33.444445555]"

notice "56789" , "987654321" , and "444445555" are all production of TimeFractionalPart in this string which is a valid TemporalDateTimeString so in ParseISODateTime it is ambigous which one of that three it is referring to in step 2. PR 1796 I push up only address case 1 and 2 but not yet case 3 yet.

@sffc @ptomato @justingrant @ljharb @Ms2ger @ryzokuken
This was referenced Sep 2, 2021
Closed
Merged
Closed
@ptomato
Copy link
Collaborator

ptomato commented Nov 4, 2021

Discussed in the champions meeting of 2021-11-04, this is a bug. Probably the way to fix it is to create three different named productions in the grammar, and only refer to one of them in this algorithm. (The correct one in Frank's example is the string 56789)

ptomato added a commit that referenced this issue Dec 3, 2021
@ptomato
Normative: Fix ambiguity in FractionalPart grammar
df6fb6e 
In ParseISODateTime, where the algorithm refers to "the part of
_isoString_ produced by the |TimeFractionalPart| production", it is
ambiguous whether this means the fractional part of the seconds in the
time representation, or the fractional part of the seconds in the
UTC-offset-named time zone.

That is, in the following string,
2021-09-01T02:03:04.56789+23:12:07.987654321[+11:22:33.444445555]
it could refer to "56789" or "444445555"

This fixes the ambiguity by making Fraction not include
TimeFractionalPart.

Closes: #1794
@ptomato ptomato mentioned this issue Dec 3, 2021
Merged
@ptomato ptomato self-assigned this Dec 3, 2021
ptomato added a commit that referenced this issue Dec 7, 2021
@ptomato
Normative: Fix ambiguity in FractionalPart grammar
8e0bae9 
In ParseISODateTime, where the algorithm refers to "the part of
_isoString_ produced by the |TimeFractionalPart| production", it is
ambiguous whether this means the fractional part of the seconds in the
time representation, or the fractional part of the seconds in the
UTC-offset-named time zone.

That is, in the following string,
2021-09-01T02:03:04.56789+23:12:07.987654321[+11:22:33.444445555]
it could refer to "56789" or "444445555"

This fixes the ambiguity by eliminating TimeFractionalPart and referencing
TimeFraction wherever TimeFractionalPart was previously referenced. Note
that TimeFraction includes the decimal separator as well, where
TimeFractionalPart did not, so string indices have to increase by one.

Closes: #1794
joyeecheung pushed a commit to joyeecheung/v8 that referenced this issue Dec 8, 2021
@FrankYFTang
[temporal] Sync parser change in PR1957
40deaa1 
tc39/proposal-temporal#1957
Resolve tc39/proposal-temporal#1794

Bug: v8:11544
Change-Id: I50d406848e815b400d6e0cd14dee95589aac0647
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3318718
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78263}
ptomato added a commit that referenced this issue Dec 17, 2021
@ptomato
Normative: Fix ambiguity in FractionalPart grammar
0f11bd6 
In ParseISODateTime, where the algorithm refers to "the part of
_isoString_ produced by the |TimeFractionalPart| production", it is
ambiguous whether this means the fractional part of the seconds in the
time representation, or the fractional part of the seconds in the
UTC-offset-named time zone.

That is, in the following string,
2021-09-01T02:03:04.56789+23:12:07.987654321[+11:22:33.444445555]
it could refer to "56789" or "444445555"

This fixes the ambiguity by eliminating TimeFractionalPart and referencing
TimeFraction wherever TimeFractionalPart was previously referenced. Note
that TimeFraction includes the decimal separator as well, where
TimeFractionalPart did not, so string indices have to increase by one.

Closes: #1794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ptomato ptomato

Labels
None yet
Projects
None yet
Milestone
Next batch of normative changes
Development

Successfully merging a pull request may close this issue.

Normative: Fix ambiguity in FractionalPart grammar tc39/proposal-temporal
2 participants
@ptomato @FrankYFTang