-
Notifications
You must be signed in to change notification settings - Fork 156
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
Implement IETF SEDATE conclusions #2397
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2397 +/- ##
=======================================
Coverage 94.74% 94.75%
=======================================
Files 20 20
Lines 10971 10990 +19
Branches 1947 1958 +11
=======================================
+ Hits 10395 10414 +19
+ Misses 532 531 -1
- Partials 44 45 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
78e0b17
to
e8d8cd6
Compare
(The ecmarkup linter error is because I've removed |CalendarName| in favour of |AnnotationValue|; but there's one reference to |CalendarName| remaining that is changed signifcantly by #2394. To fix this, this PR should really be combined with #2394, but for now I've left it separate for ease of review.) |
In keeping with the IXDTF format which extends the grammar of RFC 3339 with any number of annotations, we should allow annotations (time zone, calendar, and unknown after #2397 lands) after the short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms. If we were to allow UTC offsets ±UU[:UU] alongside the time zone annotation, that would be ambiguous in one case: YYYY-MM-UU would be ambiguous with YYYY-MM-DD. This PR makes the following choices: - UTC offset is allowed after MM-DD - UTC offset is disallowed after YYYY-MM (even if it is positive, or contains a minute component, which would not be ambiguous) Other choices are certainly possible. Closes: #2379
In keeping with the IXDTF format which extends the grammar of RFC 3339 with any number of annotations, we should allow annotations (time zone, calendar, and unknown after #2397 lands) after the short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms. If we were to allow UTC offsets ±UU[:UU] alongside the time zone annotation, that would be ambiguous in one case: YYYY-MM-UU would be ambiguous with YYYY-MM-DD. This PR makes the following choices: - UTC offset is allowed after MM-DD - UTC offset is disallowed after YYYY-MM (even if it is positive, or contains a minute component, which would not be ambiguous) Other choices are certainly possible. Closes: #2379
In keeping with the IXDTF format which extends the grammar of RFC 3339 with any number of annotations, we should allow annotations (time zone, calendar, and unknown after #2397 lands) after the short YYYY-MM PlainYearMonth and MM-DD PlainMonthDay forms. If we were to allow UTC offsets ±UU[:UU] alongside the time zone annotation, that would be ambiguous in one case: YYYY-MM-UU would be ambiguous with YYYY-MM-DD. This PR makes the following choices: - UTC offset is allowed after MM-DD - UTC offset is disallowed after YYYY-MM (even if it is positive, or contains a minute component, which would not be ambiguous) Other choices are certainly possible. Closes: #2379
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.
LGTM! We should probably also add a section to strings.md
that discusses the critical flag, and we'd probably want to point to it from the docs that mention 'critical'
instead of the vague "interoperation use cases". But if you want to defer that docs work that's OK with me!
Also, some failing tests to fix. |
Thanks for the review. I think I'd rather not go into detail in |
e8d8cd6
to
046208c
Compare
As of tc39/proposal-temporal#2397 which reached consensus in the August 2022 TC39 meeting, a date-time + Z with no bracket annotation is no longer accepted as a relativeTo parameter; either the Z should be removed or a bracket annotation should be added. This requires adjusting a few existing tests, but doesn't require any new ones.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with named and numeric offset time zone annotations, with and without the critical flag, with various combinations of Z and offset in front of the annotation.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with calendar annotations, with and without the critical flag, and also a check that the second calendar annotation is disregarded, as per the IETF draft.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with unrecognized annotations, (i.e., neither time zone nor calendar), in various combinations with recognized annotations.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with unrecognized annotations with the critical flag. These strings should all be rejected.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with more than one time zone annotation. These are not syntactically correct according to the grammar and should be rejected.
See tc39/proposal-temporal#2397 This normative change adds a new accepted value for the calendarName option in various toString() methods.
See tc39/proposal-temporal#2397 This normative change adds a new accepted value for the timeZoneName option in ZonedDateTime.prototype.toString().
As of tc39/proposal-temporal#2397 which reached consensus in the August 2022 TC39 meeting, a date-time + Z with no bracket annotation is no longer accepted as a relativeTo parameter; either the Z should be removed or a bracket annotation should be added. This requires adjusting a few existing tests, but doesn't require any new ones.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with named and numeric offset time zone annotations, with and without the critical flag, with various combinations of Z and offset in front of the annotation.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with calendar annotations, with and without the critical flag, and also a check that the second calendar annotation is disregarded, as per the IETF draft.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with unrecognized annotations, (i.e., neither time zone nor calendar), in various combinations with recognized annotations.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with unrecognized annotations with the critical flag. These strings should all be rejected.
See tc39/proposal-temporal#2397 Adds tests for ISO strings with more than one time zone annotation. These are not syntactically correct according to the grammar and should be rejected.
See tc39/proposal-temporal#2397 This normative change adds a new accepted value for the calendarName option in various toString() methods.
See tc39/proposal-temporal#2397 This normative change adds a new accepted value for the timeZoneName option in ZonedDateTime.prototype.toString().
Tests are in tc39/test262#3683. |
`relativeTo` strings must be either valid as PlainDate strings or as ZonedDateTime strings. A string with a Z but no bracketed time zone annotation is neither. We probably missed this when forbidding Z designators for Plain types, so it was an oversight.
The IETF SEDATE Working Group Internet-Draft, "Date and Time on the Internet: Timestamps with additional information" has reached agreement on all the open issues. This implements the conclusions of that draft RFC, which defines a date-time format called Internet Extended Date-Time Format (abbreviated IXDTF). IXDTF defines a grammar and semantics for annotations that can be appended to RFC 3339 strings. We were already using these annotations informally in Temporal for time zones and calendars. The main things that have to change are that annotations can have a "critical" flag ("!") which in Temporal has no effect on time zone and calendar annotations; and that multiple annotations are possible, where unknown ones are ignored unless they are marked critical. See: #1450
This adds a new recognized value "critical" to the calendarName option of the toString() methods of PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, and ZonedDateTime. calendarName: "critical" behaves like calendarName: "always", but it also outputs a "!" flag in the calendar annotation. This flag is never used by Temporal, but could be consumed by other programs. See: #1450
…ing() This adds a new recognized value "critical" to the timeZoneName option of ZonedDateTime.prototype.toString(). timeZoneName: "critical" behaves like timeZoneName: "always", but it also outputs a "!" flag in the time zone annotation. This flag is never used by Temporal, but could be consumed by other programs. See: #1450
046208c
to
7fd3af4
Compare
This reached consensus at the September 2022 TC39 meeting. I've rebased it and updated it to pull in the test262 tests now. |
https://bugs.webkit.org/show_bug.cgi?id=277942 Reviewed by NOBODY (OOPS!). RFC 3339[1] and RFC 9557[2] allows that bracketed timezone annotations include a critical flag(!)[3]. It indicate that the parsing should be rejected if there is a conflict between the offset and the timezone annotation. Temporal API has added this feature into the spec[4]. While the critical flag does not affect the behavior of Temporal, parsing must still succeed. Currently, JSC throws an error when creating a Temporal object from a string that includes a critical flag. This patch changes to allow creating Temporal objects from strings that include a critical flag. [1]: https://www.rfc-editor.org/rfc/rfc3339 [2]: https://www.rfc-editor.org/rfc/rfc9557 [3]: https://www.rfc-editor.org/rfc/rfc9557.html#name-inconsistent-time-offset-an [4]: tc39/proposal-temporal#2397 * JSTests/stress/temporal-instant-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaindate-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaindatetime-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaintime-tz-critical-flags.js: Added. * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::parseTimeZoneBracketedAnnotation):
https://bugs.webkit.org/show_bug.cgi?id=277942 Reviewed by Yusuke Suzuki. RFC 3339[1] and RFC 9557[2] allows that bracketed timezone annotations include a critical flag(!)[3]. It indicate that the parsing should be rejected if there is a conflict between the offset and the timezone annotation. Temporal API has added this feature into the spec[4]. While the critical flag does not affect the behavior of Temporal, parsing must still succeed. Currently, JSC throws an error when creating a Temporal object from a string that includes a critical flag. This patch changes to allow creating Temporal objects from strings that include a critical flag. [1]: https://www.rfc-editor.org/rfc/rfc3339 [2]: https://www.rfc-editor.org/rfc/rfc9557 [3]: https://www.rfc-editor.org/rfc/rfc9557.html#name-inconsistent-time-offset-an [4]: tc39/proposal-temporal#2397 * JSTests/stress/temporal-instant-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaindate-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaindatetime-tz-critical-flags.js: Added. * JSTests/stress/temporal-plaintime-tz-critical-flags.js: Added. * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::parseTimeZoneBracketedAnnotation): Canonical link: https://commits.webkit.org/282211@main
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):
The IETF SEDATE Working Group Internet-Draft, "Date and Time on the Internet: Timestamps with additional information" has reached agreement on all the open issues. This implements the conclusions of that draft RFC, which defines a date-time format called Internet Extended Date-Time Format (abbreviated IXDTF).
IXDTF defines a grammar and semantics for annotations that can be appended to RFC 3339 strings. We were already using these annotations informally in Temporal for time zones and calendars. The main things that have to change are that annotations can have a "critical" flag ("!") which in Temporal has no effect on time zone and calendar annotations; and that multiple annotations are possible, where unknown ones are ignored unless they are marked critical.
As part of this, the last two commits add
"critical"
as a recognized value for thetimeZoneName
andcalendarName
options in varioustoString()
methods, so that the critical flag can be output for interoperability purposes.Also fixes an oversight in parsing of
relativeTo
strings that I noticed while implementing this.See: #1450
(does not close the issue; it should be closed when the final administrative blockers to publishing the draft are cleared)