[JSC] Update implementation of Temporal.Instant.from according to spec#36361
[JSC] Update implementation of Temporal.Instant.from according to spec#36361catamorphism wants to merge 13 commits into
Conversation
|
EWS run on previous version of this PR (hash 44fce7a) Details |
44fce7a to
b8ed26b
Compare
|
EWS run on previous version of this PR (hash b8ed26b) Details |
b8ed26b to
659d18a
Compare
|
EWS run on previous version of this PR (hash 659d18a) Details |
https://bugs.webkit.org/show_bug.cgi?id=223166 Reviewed by NOBODY (OOPS!). Implement critical flag, multiple calendar annotations, and unknown annotations so that the remaining Temporal.Instant.From test262 tests pass (except for ones using ZonedDateTime) See tc39/proposal-temporal#2397 for the origin of these changes. * JSTests/stress/temporal-instant.js: * JSTests/test262/config.yaml: * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::canBeCalendar): (JSC::ISO8601::parseOneCalendar): (JSC::ISO8601::parseCalendar): (JSC::ISO8601::parseCalendarTime): (JSC::ISO8601::parseCalendarDateTime): (JSC::ISO8601::parseInstant): * Source/JavaScriptCore/runtime/ISO8601.h: * Source/JavaScriptCore/runtime/TemporalPlainDate.cpp: (JSC::TemporalPlainDate::from): * Source/JavaScriptCore/runtime/TemporalPlainDateTime.cpp: (JSC::TemporalPlainDateTime::from): * Source/JavaScriptCore/runtime/TemporalPlainTime.cpp: (JSC::TemporalPlainTime::from):
659d18a to
22596e4
Compare
|
EWS run on previous version of this PR (hash 22596e4) Details |
| shouldBe(`${Temporal.Instant.from('1976-11-18T15Z')}`, '1976-11-18T15:00:00Z'); | ||
| // ignores any specified calendar | ||
| shouldBe(`${Temporal.Instant.from('1976-11-18T15:23:30.123456789Z[u-ca=discord]')}`, '1976-11-18T15:23:30.123456789Z'); | ||
| // unknown annotations are ignored |
There was a problem hiding this comment.
Are there already equivalent tests in test262? I think so, but if not, we should add them
There was a problem hiding this comment.
Yes, there are equivalent tests:
- test/built-ins/Temporal/Instant/from/argument-string-calendar-annotation.js
- test/built-ins/Temporal/Instant/from/argument-string-time-zone-annotation.js
- test/built-ins/Temporal/Instant/from/argument-string-unknown-annotation.js
| if (i < len) { | ||
| if (buffer[i] != '[') | ||
| return false; | ||
| i++; | ||
| } | ||
| if (i < len) { | ||
| if (buffer[i] == '!') | ||
| i++; | ||
| } | ||
| // '_' allowed as first char | ||
| if (i < len) { | ||
| if (buffer[i] == '_') | ||
| i++; | ||
| } |
There was a problem hiding this comment.
These three characters are necessary, then, we should first check buffer.lengthRemaining is more than 3 and check each character with index.
There was a problem hiding this comment.
The first one is necessary, but the second two are optional.
| unsigned keyLength = 0; | ||
| while (buffer[keyLength] != '=') | ||
| keyLength++; | ||
| Vector<LChar, maxCalendarLength> key(buffer.consume(keyLength)); |
There was a problem hiding this comment.
Why can't we just have span from buffer?
|
EWS run on previous version of this PR (hash bf03a26) Details
|
|
EWS run on previous version of this PR (hash ffbda15) Details |
| int32_t len = buffer.lengthRemaining(); | ||
| // Check for [! or [, followed by any number of 'a'-'z' or '-', followed by '=' |
There was a problem hiding this comment.
Why is it int32_t? This looks not correct since it should be size_t.
And we should early return here when the length is 0.
Also, let's use length instead of len.
| // Calendar : | ||
| // [u-ca= CalendarName] | ||
| return buffer.lengthRemaining() >= 6 && buffer[0] == '[' && buffer[1] == 'u' && buffer[2] == '-' && buffer[3] == 'c' && buffer[4] == 'a' && buffer[5] == '='; | ||
| // https://tc39.es/proposal-temporal/#sec-temporal-parseisodatetime |
There was a problem hiding this comment.
This is not corresponding to the syntax definition section. Please use a link to that.
Also, please describe the BNF form of this as the same to the other function (like, parseTimeSpec for example).
There was a problem hiding this comment.
Please check parseTimeSpec comment carefully. We do not need to have many URLs, but we need documentation comment for parsing logic.
| static std::tuple<std::optional<CalendarRecord>, std::optional<CalendarRecord>> | ||
| parseCalendar(StringParsingBuffer<CharacterType>& buffer) | ||
| { | ||
| std::optional<CalendarRecord> first = parseOneCalendar(buffer); |
There was a problem hiding this comment.
Ditto. Please add link and add BNF.
|
EWS run on previous version of this PR (hash 19227db) Details |
|
EWS run on previous version of this PR (hash 3a906bd) Details |
| // Parse AKeyLeadingChar | ||
| // '_' allowed as first char |
| // https://tc39.es/proposal-temporal/#prod-Annotation | ||
| // Annotation ::: | ||
| // [ AnnotationCriticalFlag_opt AnnotationKey = AnnotationValue ] | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AnnotationCriticalFlag | ||
| // AnnotationCriticalFlag ::: | ||
| // ! | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AnnotationKey | ||
| // AnnotationKey ::: | ||
| // AKeyLeadingChar | ||
| // AnnotationKey AKeyChar | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AKeyLeadingChar | ||
| // AKeyLeadingChar ::: | ||
| // LowercaseAlpha | ||
| // _ | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AKeyChar | ||
| // AKeyChar ::: | ||
| // AKeyLeadingChar | ||
| // DecimalDigit | ||
| // - |
There was a problem hiding this comment.
Please follow to the other comment's BNF to make it readable well. (See parseUTCOffsetInMinutes for example)
// https://tc39.es/proposal-temporal/#prod-Annotation
//
// Annotation :::
// [ AnnotationCriticalFlag_opt AnnotationKey = AnnotationValue ]
//
// AnnotationCriticalFlag :::
// !
//
// AnnotationKey :::
// AKeyLeadingChar
// AnnotationKey AKeyChar
//
// AKeyLeadingChar :::
// LowercaseAlpha
// _
//
// AKeyChar :::
// AKeyLeadingChar
// DecimalDigit
// -
Also, I cannot see AnnotationValue definition.
There was a problem hiding this comment.
I think it's not necessary to include AnnotationValue in the comment because this code doesn't parse the value; it just looks ahead to see if a possible prefix of an Annotation is present (up to the =). What do you think? (The parsing is done by parseOneCalendar(). )
There was a problem hiding this comment.
Please use the above comment's content as is. The main purpose of this comment is understanding what is parsed in this function by checking this comment. So, we need necessary information (We do not want to check the spec to see whether this function is implemented correctly).
There was a problem hiding this comment.
Added AnnotationValue to comment.
|
EWS run on previous version of this PR (hash b882dd8) Details |
|
|
||
| template<typename CharacterType> | ||
| static std::tuple<std::optional<CalendarRecord>, std::optional<CalendarRecord>> | ||
| parseCalendar(StringParsingBuffer<CharacterType>& buffer) |
There was a problem hiding this comment.
Why is it only returning up to two calendars?
From the definition of https://tc39.es/proposal-temporal/#prod-Annotations, it can be a vector of annotations.
If so, then it should be std::optional<Vector<CalendarRecord>>
| // Annotation ::: | ||
| // [ AnnotationCriticalFlag_opt AnnotationKey = AnnotationValue ] | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AnnotationCriticalFlag |
There was a problem hiding this comment.
But we do not need this URL.
| // AnnotationCriticalFlag ::: | ||
| // ! | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AnnotationKey |
There was a problem hiding this comment.
But we do not need this URL.
| // AKeyLeadingChar | ||
| // AnnotationKey AKeyChar | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AKeyLeadingChar |
There was a problem hiding this comment.
But we do not need this URL.
| // LowercaseAlpha | ||
| // _ | ||
|
|
||
| // https://tc39.es/proposal-temporal/#prod-AKeyChar |
There was a problem hiding this comment.
But we do not need this URL.
|
EWS run on previous version of this PR (hash 9ac33a5) Details |
|
EWS run on previous version of this PR (hash e010f87) Details |
|
EWS run on previous version of this PR (hash f662acf) Details |
|
EWS run on previous version of this PR (hash ce8556a) Details |
| // | ||
| // AnnotationValueComponent ::: | ||
| // Alpha AnnotationValueComponent[opt] | ||
| // DecimalDigit AnnotationValueComponent[opt] |
There was a problem hiding this comment.
The format of these syntax is not aligned to the other code. Please check parseTimeSpec etc. very carefully and align it to them.
| while (canBeCalendar(buffer)) { | ||
| auto calendarRecord = parseOneCalendar(buffer); | ||
| if (!calendarRecord) | ||
| return std::nullopt; |
There was a problem hiding this comment.
Is it right? If we already parsed one calendar for example, and if the second one failed, I don't think returning nullopt here is the right thing.
There was a problem hiding this comment.
I'm not sure if there's a string that would cause this path to be taken. It seems like canBeCalendar() implies that parseOneCalendar() will succeed. Maybe it should be an assertion.
There was a problem hiding this comment.
(well, maybe not. After thinking about it some more, I think canBeCalendar() does not imply that parseOneCalendar() will succeed. The heuristic in canBeCalendar() would return true for a remaining buffer of [!=..
There was a problem hiding this comment.
So then yes, I think this code is correct. A string such as 2025-02-19T13:28[u-ca=gregory][!=.. should return nullopt here, because the [!=.. is "junk at the end of the string" which is not allowed and should cause ToTemporalPlainDateTime etc. to throw a RangeError.
| // Check for multiple calendars and critical flag | ||
| // step 4(a)(ii)(2)(c)(ii) | ||
| else if (calendarRecord->m_critical || calendarWasCritical) | ||
| return std::nullopt; |
There was a problem hiding this comment.
I'm pretty sure this code is correct. According to the spec ParseISODateTime, if there is more than one calendar annotation and any of them have the critical flag, the string should be rejected. See also https://www.rfc-editor.org/rfc/rfc9557.html#name-optional-generation-and-ele
| // Check for unknown annotations with critical flag | ||
| // step 4(a)(ii)(2)(d)(i) | ||
| if (calendarRecord->m_unknown && calendarRecord->m_critical) | ||
| return std::nullopt; |
There was a problem hiding this comment.
Likewise, I'm pretty sure this is correct. If there is an unrecognized annotation and it is marked critical (e.g. [!foo=bar]), it must be rejected.
| enum class ValidateTimeZoneID : bool { No, Yes }; | ||
| std::optional<std::tuple<PlainTime, std::optional<TimeZoneRecord>>> parseTime(StringView); | ||
| std::optional<std::tuple<PlainTime, std::optional<TimeZoneRecord>, std::optional<CalendarRecord>>> parseCalendarTime(StringView); | ||
| std::optional<std::tuple<PlainTime, std::optional<TimeZoneRecord>, std::optional<Vector<CalendarRecord>>>> parseCalendarTime(StringView); |
There was a problem hiding this comment.
Most common case is one-calendar when exists. So use Vector<CalendarRecord, 1> throughout the code.
There was a problem hiding this comment.
I'm actually thinking it'd be better not to return a vector here. If there are two calendar annotations and neither are critical, the spec says the first one must be chosen: https://www.rfc-editor.org/rfc/rfc9557.html#section-3.3-16
There was a problem hiding this comment.
...but we still want to allow zero calendars. So maybe std::optional<Vector<CalendarRecord, 1>> is okay.
(Unless you prefer std::optional<std::optional<CalendarRecord>>. Also, in a future revision, you may want to refactor this into something like a (fictional) std::expected<std::optional<CalendarRecord>, CalendarParseErrorEnum> in order to be able to generate better error messages.)
There was a problem hiding this comment.
Fixed in 51b4125. I used Vector<CalendarRecord, 1> in this function, but switched back to std::optional<CalendarRecord> at callers, because if we parse ≥2 calendars we should ignore all subsequent ones.
sosukesuzuki
left a comment
There was a problem hiding this comment.
I'm not a WebKit Reviewer, so this is informal review.
| Vector<CalendarRecord> result; | ||
| // https://tc39.es/proposal-temporal/#sec-temporal-parseisodatetime | ||
| bool calendarWasCritical = false; | ||
| while (canBeCalendar(buffer)) { |
There was a problem hiding this comment.
If canBeCalendar(buffer) is false, it should return std::nullopt instead of an empty vector? That way we can remove the size check on the caller
There was a problem hiding this comment.
I think that would mix two cases that need to be distinguished — std::nullopt means parsing error, empty vector means valid string without a calendar annotation.
| // Digit | ||
| // For BNF, see comment in canBeCalendar() | ||
|
|
||
| if (!canBeCalendar(buffer)) |
There was a problem hiding this comment.
This should be assertion? Because canBeCalendar is called on caller side.
| // can't have multiple annotations if one is critical | ||
| shouldThrow(() => Temporal.Instant.from('1976-11-18T15:23:30.123456789Z[!u-ca=hebrew][u-ca=discord]'), RangeError); | ||
| // can't have an unknown critical annotation | ||
| shouldThrow(() => Temporal.Instant.from('1976-11-18T15:23:30.123456789Z[!foo=bar]'), RangeError); |
There was a problem hiding this comment.
We should throw RangeError for Temporal.Instant.from('1976-11-18T15:23:30.123456789Z[======]')?
There was a problem hiding this comment.
Good point. I added this test and a similar one with zero-length annotation value in 51b4125.
|
EWS run on previous version of this PR (hash 51b4125) Details |
|
EWS run on previous version of this PR (hash afaf5fc) Details |
🛠 mac|
@Constellation I'm attempting to move this PR forward while Tim is out this week. I think the comments have been addressed now. |
Constellation
left a comment
There was a problem hiding this comment.
Looks like comments are not resolved yet.
|
EWS run on current version of this PR (hash d4dc3b9) Details |
It looks like resolving #36361 (comment) unresolved those other ones. I've pushed an update removing all subsequent links to grammar productions after the first one in |
https://bugs.webkit.org/show_bug.cgi?id=223166 Reviewed by Justin Michaud and Sosuke Suzuki. This does not change anything currently, but is groundwork for implementing a normative change to the Temporal proposal: tc39/proposal-temporal#2397 It changes parseCalendar() to handle unknown annotation keys and known annotation keys with the `!` critical flag. (Although those are still disallowed by canBeCalendar() at this time, I will send a follow-up PR for that.) Note this code was already submitted as part of WebKit#36361 which I'm trying to split up into smaller, more easily reviewable parts. * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::parseCalendar): Canonical link: https://commits.webkit.org/293522@main
https://bugs.webkit.org/show_bug.cgi?id=223166 Reviewed by Yusuke Suzuki. This is part of a normative change to the Temporal proposal: tc39/proposal-temporal#2397 It adds support for the `!` critical flag to calendar annotations in parsed strings. The flag is ignored in all normal cases, but we still store it as a boolean in CalendarRecord, because when we start handling unknown annotation keys, we'll need to treat those differently depending on whether they were flagged or not. Note this code was already submitted as part of WebKit#36361 which I'm trying to split up into smaller, more easily reviewable parts. * JSTests/stress/temporal-instant.js: * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::canBeCalendar): (JSC::ISO8601::parseCalendar): * Source/JavaScriptCore/runtime/ISO8601.h: Canonical link: https://commits.webkit.org/293612@main
|
This has been split up into smaller PRs and landed piecewise. It can be closed. |
https://bugs.webkit.org/show_bug.cgi?id=223166
Reviewed by NOBODY (OOPS!).
Implement critical flag, multiple calendar annotations, and unknown annotations so that the remaining Temporal.Instant.From test262 tests pass (except for ones using ZonedDateTime)
See tc39/proposal-temporal#2397 for the origin of these changes.
(JSC::ISO8601::parseOneCalendar):
(JSC::ISO8601::parseCalendar):
(JSC::ISO8601::parseCalendarTime):
(JSC::ISO8601::parseCalendarDateTime):
(JSC::ISO8601::parseInstant):
d4dc3b9
🛠 playstation