Normative: Fix production for time zone name in time zone strings by ptomato · Pull Request #2200 · tc39/proposal-temporal

ptomato · 2022-05-14T00:43:09Z

ParseTemporalTimeZoneString erroneously assumed that any bare time zone
identifier or bracketed name would be an IANA name. That is not the case,
it can be a UTC offset name as well.

Closes: #1805

ptomato · 2022-05-14T00:43:26Z

Draft until presented to TC39.

codecov · 2022-05-14T00:44:00Z

Codecov Report

Merging #2200 (0c657e5) into main (78abbb8) will increase coverage by 7.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2200      +/-   ##
==========================================
+ Coverage   84.01%   91.09%   +7.08%     
==========================================
  Files          18       19       +1     
  Lines        4766    10577    +5811     
  Branches     1088     1696     +608     
==========================================
+ Hits         4004     9635    +5631     
- Misses        594      932     +338     
+ Partials      168       10     -158

Flag	Coverage Δ
test262	`84.01% <ø> (ø)`
tests	`81.85% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/plaintime.mjs	`69.20% <0.00%> (-30.80%)`	⬇️
polyfill/lib/plaindatetime.mjs	`78.73% <0.00%> (-20.28%)`	⬇️
polyfill/lib/plainyearmonth.mjs	`77.04% <0.00%> (-16.42%)`	⬇️
polyfill/lib/plaindate.mjs	`82.95% <0.00%> (-15.77%)`	⬇️
polyfill/lib/duration.mjs	`80.61% <0.00%> (-15.51%)`	⬇️
polyfill/lib/plainmonthday.mjs	`82.26% <0.00%> (-11.33%)`	⬇️
polyfill/lib/intrinsicclass.mjs	`51.35% <0.00%> (-4.21%)`	⬇️
polyfill/lib/instant.mjs	`90.54% <0.00%> (-3.15%)`	⬇️
polyfill/lib/ecmascript.mjs	`93.34% <0.00%> (-0.92%)`	⬇️
polyfill/lib/now.mjs	`100.00% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78abbb8...0c657e5. Read the comment docs.

FrankYFTang · 2022-05-14T02:06:38Z

So... TimeZoneNumericUTCOffset will be the value for both offsetString and name if it present in the string? [from what I can read from your PR)

Could ou list some example to illustrate the problem and expectation. Thanks

ptomato · 2022-05-16T18:57:19Z

@FrankYFTang Here are the cases I'm thinking of:

Parse a string that is a bare IANA identifier: Europe/London
- name: matches Europe/London via TemporalTimeZoneIdentifier / TimeZoneIANAName
- offsetString: empty
- z: empty
Parse a string that is a bare UTC offset time zone identifier: -07:00
- name: matches -07:00 via TemporalTimeZoneIdentifier / TimeZoneNumericUTCOffset
- offsetString: matches -07:00 via TemporalTimeZoneIdentifier / TimeZoneNumericUTCOffset
- z: empty
Parse an ISO string with UTC designator: 2022-05-16T11:40Z
- name: empty
- offsetString: empty
- z: matches Z via TimeZone / TimeZoneUTCOffset / UTCDesignator
Parse an ISO string with numeric UTC offset: 2022-05-16T11:40-07:00
- name: empty
- offsetString: matches -07:00 via TimeZone / TimeZoneUTCOffset / TimeZoneNumericUTCOffset
- z: empty
Parse an ISO string with bracketed IANA identifier: 2022-05-16T11:40[Europe/London]
- name: matches Europe/London via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: empty
- z: empty
Parse an ISO string with UTC designator and bracketed IANA identifier: 2022-05-16T11:40Z[Europe/London]
- name: matches Europe/London via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: empty
- z: matches Z via TimeZone / TimeZoneUTCOffset / UTCDesignator
Parse an ISO string with numeric UTC offset and bracketed IANA identifier: 2022-05-16T11:40-07:00[Europe/London]
- name: matches Europe/London via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: matches -07:00 via TimeZone / TimeZoneUTCOffset / TimeZoneNumericUTCOffset
- z: empty
Parse an ISO string with bracketed UTC offset time zone identifier: 2022-05-16T11:40[-07:00]
- name: matches -07:00 via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: empty (does not match TimeZoneUTCOffsetName)
- z: empty
Parse an ISO string with UTC designator and bracketed UTC offset time zone identifier: 2022-05-16T11:40Z[-07:00]
- name: matches -07:00 via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: empty (does not match TimeZoneUTCOffsetName)
- z: matches Z via TimeZone / TimeZoneUTCOffset / UTCDesignator
Parse an ISO string with numeric UTC offset and bracketed UTC offset time zone identifier: 2022-05-16T11:40+02:00[-07:00]
- name: matches -07:00 via TimeZone / TimeZoneBracketedAnnotation / TimeZoneBracketedName
- offsetString: matches +02:00 via TimeZone / TimeZoneUTCOffset / TimeZoneNumericUTCOffset (does not match TimeZoneUTCOffsetName)
- z: empty

I do see one oddity in this list, that's parsing -07:00 gets a value in _offsetString_ even though there is technically no offset string in that string. I think this is harmless though, since name is preferred in ToTemporalTimeZone, and this branch is not taken in any of the other callers of ParseTemporalTimeZoneString because the string is already determined to be an ISO string, so a bare UTC offset would be rejected earlier.

Just for avoidance of any doubt, I think it's enough to have TemporalTimeZoneIdentifier enclose TimeZoneUTCOffsetName instead of TimeZoneNumericUTCOffset. This would be an editorial change because I don't think it's possible to see any observable semantics from changing it.

ptomato · 2022-05-17T17:00:58Z

I rebased this on the editorial change that I mentioned. Now it is a simpler change, and the oddity with -07:00 is gone.

anba · 2022-06-10T14:38:18Z

The following tests will no longer be valid with this change:

test/built-ins/Temporal/Now/zonedDateTimeISO/timezone-string-multiple-offsets.js
test/built-ins/Temporal/Now/zonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/from/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/prototype/equals/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/prototype/equals/timezone-string-datetime.js
test/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/prototype/since/timezone-string-multiple-offsets.js
test/built-ins/Temporal/ZonedDateTime/prototype/until/timezone-string-multiple-offsets.js
test/built-ins/Temporal/PlainDate/prototype/toZonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/TimeZone/from/timezone-string-multiple-offsets.js
test/built-ins/Temporal/PlainTime/prototype/toZonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/PlainDateTime/prototype/toZonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/Instant/prototype/toZonedDateTimeISO/timezone-string-multiple-offsets.js
test/built-ins/Temporal/Instant/prototype/toZonedDateTime/timezone-string-multiple-offsets.js
test/built-ins/Temporal/Instant/prototype/toString/timezone-string-multiple-offsets.js

The tests use time zone strings like "2021-08-19T17:30:45.123456789+01:46[+01:45:30.987654321]". When processed in ToTemporalTimeZone, step 4.a.i will throw a RangeError, because the offset strings don't match.

Ms2ger · 2022-06-13T07:35:56Z

The following tests will no longer be valid with this change:

Hmm, I'm not sure that's intentional. We'll take another look.

ParseTemporalTimeZoneString erroneously assumed that any bare time zone identifier or bracketed name would be an IANA name. That is not the case, it can be a UTC offset name as well. Closes: #1805

ptomato · 2022-06-23T10:45:08Z

If I understand the situation correctly it's because +01:45:30.987654321 previously did not match, so the [[Name]] slot of the result would be undefined? But then I think those tests were already invalid, because e.g. in Temporal/Now/zonedDateTimeISO/timezone-string-multiple-offsets.js, the resulting time zone would have been constructed from the offset string, not the bracketed name. However, this I believe is due to an unintentional change in one of the refactors to make string parsing go through ParseText.

I believe the solution is to add the "match minutes" behaviour to ToTemporalTimeZone, just like it is in ToTemporalZonedDateTime and ToRelativeTemporalObject. I think it should be OK to tack another commit on to this PR to restore the original situation we had. I'll do this today.

ptomato · 2022-06-23T10:48:44Z

...or maybe we were originally intending to discard the offset string if a bracketed name was present, when parsing a time zone string? I'll dig into the discussion archives.

ptomato · 2022-06-23T11:07:46Z

I believe I inadvertently changed this in 2a81fbc, and the offset string should be discarded if there is a bracketed name. This seems to be confirmed by #1696.

In [2a81fbc] I inadvertently added this validation that the offset string is equal to a bracketed offset string in TimeZone.from(). Instead, the offset string should be ignored when there is a bracketed name. The accidental change invalidated several test262 tests, which should now be valid once more.

ptomato · 2022-06-23T11:19:02Z

@anba How does this look to you?

anba · 2022-06-24T14:31:59Z

@anba How does this look to you?

I think that should work.

Just to double check, we'll end up with the following behaviour:

Temporal.PlainDateTime.from ignores any time zone parts
Temporal.ZonedDateTime.from rejects mismatched time zone information
Temporal.Instant.from ignores mismatched time zone information and always uses the offset
Temporal.TimeZone.from ignores mismatched time zone information and always uses the bracketed part

string	operation	output
`"1970-01-01T12:00:00+01:00[+02:00]"`	`Temporal.PlainDateTime.from`	`1970-01-01T12:00:00`
`"1970-01-01T12:00:00+01:00[+02:00]"`	`Temporal.ZonedDateTime.from`	`RangeError`
`"1970-01-01T12:00:00+01:00[+02:00]"`	`Temporal.Instant.from`	`1970-01-01T11:00:00Z`
`"1970-01-01T12:00:00+01:00[+02:00]"`	`Temporal.TimeZone.from`	`+02:00`
`"1970-01-01T12:00:00+01:00[America/New_York]"`	`Temporal.PlainDateTime.from`	`1970-01-01T12:00:00`
`"1970-01-01T12:00:00+01:00[America/New_York]"`	`Temporal.ZonedDateTime.from`	`RangeError`
`"1970-01-01T12:00:00+01:00[America/New_York]"`	`Temporal.Instant.from`	`1970-01-01T11:00:00Z`
`"1970-01-01T12:00:00+01:00[America/New_York]"`	`Temporal.TimeZone.from`	`America/New_York`

ptomato · 2022-06-25T17:52:31Z

That is correct, I believe. The principle is that each from() method only pays attention to the relevant parts of the string, as long as it is syntactically correct.

(ZonedDateTime.from() can also accept mismatched time zone information if you give it the offset option, but I'm leaving that out of consideration here)

ptomato · 2022-06-26T07:25:05Z

This change achieved consensus in the June 2022 TC39 plenary meeting, and the fix for anba's issue does not change the intention of the normative change. I'll go ahead and merge this. It is already covered by test262 tests.

ptomato requested review from Ms2ger and gibson042 May 14, 2022 00:43

ptomato marked this pull request as draft May 14, 2022 00:43

This was referenced May 14, 2022

TimeZoneUTCOffsetName and Etc/GMT are ignored in ParseTemporalTimeZoneString #1805

Closed

Editorial: name = TimeZoneBracketedName <= TimeZoneIANAName #1941

Closed

ptomato added the spec-text Specification text involved label May 14, 2022

Ms2ger approved these changes May 16, 2022

View reviewed changes

ptomato mentioned this pull request May 17, 2022

Various editorial changes #2204

Merged

ptomato force-pushed the 1805-time-zone-identifier branch from 0e7bb9a to 2900ad7 Compare May 17, 2022 17:00

ptomato mentioned this pull request May 17, 2022

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

Closed

gibson042 approved these changes May 20, 2022

View reviewed changes

ptomato added this to the Next batch of normative changes milestone May 23, 2022

Normative: Fix production for time zone name in time zone strings

cf7b9f1

ParseTemporalTimeZoneString erroneously assumed that any bare time zone identifier or bracketed name would be an IANA name. That is not the case, it can be a UTC offset name as well. Closes: #1805

ptomato force-pushed the 1805-time-zone-identifier branch from 2900ad7 to 0c657e5 Compare June 23, 2022 11:12

ptomato requested review from Ms2ger and gibson042 June 23, 2022 11:18

ptomato marked this pull request as ready for review June 26, 2022 07:21

ptomato merged commit fb7cfa6 into main Jun 26, 2022

ptomato deleted the 1805-time-zone-identifier branch June 26, 2022 07:25

ptomato mentioned this pull request Jan 20, 2025

undefined TimeZoneIANAName should not throw RangeError in *relativeto-sub-minute-offset tests. tc39/test262#3319

Closed

Conversation

ptomato commented May 14, 2022

Uh oh!

ptomato commented May 14, 2022

Uh oh!

codecov bot commented May 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FrankYFTang commented May 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptomato commented May 16, 2022

Uh oh!

ptomato commented May 17, 2022

Uh oh!

anba commented Jun 10, 2022

Uh oh!

Ms2ger commented Jun 13, 2022

Uh oh!

ptomato commented Jun 23, 2022

Uh oh!

ptomato commented Jun 23, 2022

Uh oh!

ptomato commented Jun 23, 2022

Uh oh!

ptomato commented Jun 23, 2022

Uh oh!

anba commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptomato commented Jun 25, 2022

Uh oh!

ptomato commented Jun 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented May 14, 2022 •

edited

Loading

FrankYFTang commented May 14, 2022 •

edited

Loading

anba commented Jun 24, 2022 •

edited

Loading