Bug(s) introduced by "Editorial: Combine TemporalTimeZoneIdentifier and TimeZoneBracketedName"

[linusg]

https://github.com/tc39/proposal-temporal/commit/6db76f4808100e2bd92e3aa379ac55b88dad4d91 looks harmless on the surface, but broke 10 tests in test262 after I implemented it. A somehow related implementation bug causing that is of course possible, but I found at least one legitimate issue caused by this commit:

• ParseTemporalTimeZoneString still refers to a TimeZoneNumericUTCOffset parse node contained within the result of parsing TemporalTimeZoneString, even though the latter no longer contains the former. TemporalTimeZoneIdentifier used to contain TimeZoneNumericUTCOffset, but TimeZoneIdentifier only contains TimeZoneUTCOffsetName.

When replacing this outdated reference and using TimeZoneUTCOffsetName for offsetString, those broken tests are fixed again, but a handful of other tests starts failing, so something else might be amiss - I haven't investigated further.

[ptomato]

Is it possible that these underlying bugs are the ones fixed by #2200?

[linusg]

That doesn't seem to be the case - I compared three different points in the implementation timeline: baseline (before the commit linked above), editorial (baseline + the commit above implemented), and normative (editorial + the commit you mentioned implemented).

My diffs for them are below, hopefully showing an exact translation of the spec changes into implementation code:

editorial.patch

```diff diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp index 4154c00ea6..836e90e62c 100644 --- a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp +++ b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp @@ -950,10 +950,10 @@ bool ISO8601Parser::parse_time_zone_iana_name() return true; } -// https://tc39.es/proposal-temporal/#prod-TimeZoneBracketedName -bool ISO8601Parser::parse_time_zone_bracketed_name() +// https://tc39.es/proposal-temporal/#prod-TimeZoneIdentifier +bool ISO8601Parser::parse_time_zone_identifier() { - // TimeZoneBracketedName : + // TimeZoneIdentifier : // TimeZoneIANAName // TimeZoneUTCOffsetName StateTransaction transaction { *this }; @@ -970,11 +970,11 @@ bool ISO8601Parser::parse_time_zone_bracketed_name() bool ISO8601Parser::parse_time_zone_bracketed_annotation() { // TimeZoneBracketedAnnotation : - // [ TimeZoneBracketedName ] + // [ TimeZoneIdentifier ] StateTransaction transaction { *this }; if (!m_state.lexer.consume_specific('[')) return false; - if (!parse_time_zone_bracketed_name()) + if (!parse_time_zone_identifier()) return false; if (!m_state.lexer.consume_specific(']')) return false; @@ -1650,24 +1650,14 @@ bool ISO8601Parser::parse_temporal_time_string() || parse_calendar_time(); } -// https://tc39.es/proposal-temporal/#prod-TemporalTimeZoneIdentifier -bool ISO8601Parser::parse_temporal_time_zone_identifier() -{ - // TemporalTimeZoneIdentifier : - // TimeZoneNumericUTCOffset - // TimeZoneIANAName - return parse_time_zone_numeric_utc_offset() - || parse_time_zone_iana_name(); -} - // https://tc39.es/proposal-temporal/#prod-TemporalTimeZoneString bool ISO8601Parser::parse_temporal_time_zone_string() { // TemporalTimeZoneString : - // TemporalTimeZoneIdentifier + // TimeZoneIdentifier // Date TimeSpecSeparator[opt] TimeZone Calendar[opt] StateTransaction transaction { *this }; - if (!parse_temporal_time_zone_identifier()) { + if (!parse_time_zone_identifier()) { if (!parse_date()) return false; (void)parse_time_spec_separator(); diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h index fdd7a9a3ba..954b304437 100644 --- a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h +++ b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h @@ -136,7 +136,7 @@ public: [[nodiscard]] bool parse_time_zone_iana_component(); [[nodiscard]] bool parse_time_zone_iana_name_tail(); [[nodiscard]] bool parse_time_zone_iana_name(); - [[nodiscard]] bool parse_time_zone_bracketed_name(); + [[nodiscard]] bool parse_time_zone_identifier(); [[nodiscard]] bool parse_time_zone_bracketed_annotation(); [[nodiscard]] bool parse_time_zone_offset_required(); [[nodiscard]] bool parse_time_zone_name_required(); @@ -176,7 +176,6 @@ public: [[nodiscard]] bool parse_temporal_duration_string(); [[nodiscard]] bool parse_temporal_month_day_string(); [[nodiscard]] bool parse_temporal_time_string(); - [[nodiscard]] bool parse_temporal_time_zone_identifier(); [[nodiscard]] bool parse_temporal_time_zone_string(); [[nodiscard]] bool parse_temporal_year_month_string(); [[nodiscard]] bool parse_temporal_zoned_date_time_string(); ```

normative.patch

```diff diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp index a4d8ad4765..989e4d9577 100644 --- a/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp +++ b/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp @@ -1595,7 +1595,7 @@ ThrowCompletionOr parse_temporal_time_zone_string(GlobalObject // 4. Let each of z, offsetString, and name be the source text matched by the respective UTCDesignator, TimeZoneNumericUTCOffset, and TimeZoneIANAName Parse Node contained within parseResult, or an empty sequence of code points if not present. auto z = parse_result->utc_designator; auto offset_string = parse_result->time_zone_numeric_utc_offset; - auto name = parse_result->time_zone_iana_name; + auto name = parse_result->time_zone_identifier; // 5. If name is empty, then // a. Set name to undefined. diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp index 836e90e62c..2ae84f80e1 100644 --- a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp +++ b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.cpp @@ -945,7 +945,6 @@ bool ISO8601Parser::parse_time_zone_iana_name() } else if (!parse_time_zone_iana_name_tail()) { return false; } - m_state.parse_result.time_zone_iana_name = transaction.parsed_string_view(); transaction.commit(); return true; } @@ -963,6 +962,7 @@ bool ISO8601Parser::parse_time_zone_identifier() return false; } transaction.commit(); + m_state.parse_result.time_zone_identifier = transaction.parsed_string_view(); return true; } diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h index 954b304437..71dfe0b460 100644 --- a/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h +++ b/Userland/Libraries/LibJS/Runtime/Temporal/ISO8601.h @@ -37,7 +37,7 @@ struct ParseResult { Optional time_zone_utc_offset_minute; Optional time_zone_utc_offset_second; Optional time_zone_utc_offset_fractional_part; - Optional time_zone_iana_name; + Optional time_zone_identifier; Optional duration_years; Optional duration_months; Optional duration_weeks; ```

Going from baseline to editorial yields these test262 changes (✅ = test passing, ❌ = test failing, 💥️ = interpreter crashing because of failed assertion, unwrapping empty optional that is assumed to have a value, etc)

Diff Tests:
    test/built-ins/Temporal/PlainTime/prototype/toZonedDateTime/basic.js                       ✅ -> 💥️
    test/built-ins/Temporal/TimeZone/from/argument-valid.js                                    ✅ -> 💥️
    test/built-ins/Temporal/TimeZone/prototype/getPlainDateTimeFor/pre-epoch.js                ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/timezone-string-multiple-offsets.js ❌ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/largestunit.js                       ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/timezone-string-datetime.js          ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/timezone-string-multiple-offsets.js  ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/until/timezone-string-datetime.js          ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/until/timezone-string-multiple-offsets.js  ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone/subclassing-ignored.js        ✅ -> 💥️

Going from editorial to normative fixes a bunch of other crashes and failures (and one starting to fail :thinking:):

Diff Tests:
    test/built-ins/Temporal/Duration/compare/relativeto-sub-minute-offset.js                        ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/add/relativeto-sub-minute-offset.js                  ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/round/relativeto-sub-minute-offset.js                ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/subtract/relativeto-sub-minute-offset.js             ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/total/relativeto-sub-minute-offset.js                ❌ -> ✅
    test/built-ins/Temporal/Instant/prototype/toString/timezone-string-multiple-offsets.js          ✅ -> ❌
    test/built-ins/Temporal/ZonedDateTime/compare/zoneddatetime-string.js                           💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/from/zoneddatetime-string-multiple-offsets.js             💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/from/zoneddatetime-string.js                              💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/sub-minute-offset.js                     💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/zoneddatetime-string-multiple-offsets.js 💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/zoneddatetime-string.js                  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/sub-minute-offset.js                      💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/zoneddatetime-string-multiple-offsets.js  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/zoneddatetime-string.js                   💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/sub-minute-offset.js                      💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/zoneddatetime-string-multiple-offsets.js  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/zoneddatetime-string.js                   💥️ -> ✅

Both of them combined, e.g. comparing baseline against editorial + normative:

Diff Tests:
    test/built-ins/Temporal/Duration/compare/relativeto-sub-minute-offset.js                        ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/add/relativeto-sub-minute-offset.js                  ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/round/relativeto-sub-minute-offset.js                ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/subtract/relativeto-sub-minute-offset.js             ❌ -> ✅
    test/built-ins/Temporal/Duration/prototype/total/relativeto-sub-minute-offset.js                ❌ -> ✅
    test/built-ins/Temporal/Instant/prototype/toString/timezone-string-multiple-offsets.js          ✅ -> ❌
    test/built-ins/Temporal/PlainTime/prototype/toZonedDateTime/basic.js                            ✅ -> 💥️
    test/built-ins/Temporal/TimeZone/from/argument-valid.js                                         ✅ -> 💥️
    test/built-ins/Temporal/TimeZone/prototype/getPlainDateTimeFor/pre-epoch.js                     ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/compare/zoneddatetime-string.js                           💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/from/zoneddatetime-string-multiple-offsets.js             💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/from/zoneddatetime-string.js                              💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/sub-minute-offset.js                     💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/timezone-string-multiple-offsets.js      ❌ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/zoneddatetime-string-multiple-offsets.js 💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/equals/zoneddatetime-string.js                  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/largestunit.js                            ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/sub-minute-offset.js                      💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/timezone-string-datetime.js               ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/timezone-string-multiple-offsets.js       ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/since/zoneddatetime-string-multiple-offsets.js  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/since/zoneddatetime-string.js                   💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/sub-minute-offset.js                      💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/timezone-string-datetime.js               ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/until/timezone-string-multiple-offsets.js       ✅ -> 💥️
    test/built-ins/Temporal/ZonedDateTime/prototype/until/zoneddatetime-string-multiple-offsets.js  💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/until/zoneddatetime-string.js                   💥️ -> ✅
    test/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone/subclassing-ignored.js             ✅ -> 💥️

i.e. various crashes are fixed, and various previously passing tests start to crash. Take all of this with a grain of salt, there definitely are bugs, but I'm also convinced that there's a bug hiding somewhere in the spec, either caused or uncovered by these two changes.

FWIW, implementing my initially mentioned fix of replacing the reference to TimeZoneNumericUTCOffset on top of both of these causes absolute mayhem (-70 ✅ +17 ❌ +53 💥️), leaving me slightly confused. I'll continue to look into this tomorrow.

[ptomato]

@linusg Any further insights into what was going on?

[linusg]

Unfortunately not, no.

[ptomato]

Verify before closing.

[ptomato]

The issue with ParseTemporalTimeZone has been fixed in the meantime, and according to https://libjs.dev/test262/per-file/ all of the failing/crashing tests listed above are now passing on libjs. Closing.

[linusg]

I lost track of this, thanks for checking!