Normative: Do not validate time zone names when parsing Instant string

[ptomato]

This commit looks like it just moves a few things around, moving the
validation of time zone names from ParseTemporalTimeZoneString to its
callers. But the important part to note is that the validation no longer
takes place in ParseTemporalInstantString, one of the callers of
ParseTemporalTimeZoneString.

This is already covered by test262 tests.

Closes: #1897

[ptomato]

Draft until presented at TC39

[codecov]

Codecov Report

Merging #1953 (742471c) into main (69062d1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1953   +/-   ##
=======================================
  Coverage   95.00%   95.00%           
=======================================
  Files          19       19           
  Lines       10990    10990           
  Branches     1600     1600           
=======================================
  Hits        10441    10441           
  Misses        532      532           
  Partials       17       17           

Flag	Coverage Δ
test262	79.26% <ø> (ø)
tests	89.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 69062d1...742471c. Read the comment docs.

[ptomato]

cc @FrankYFTang

[ptomato]

Thanks for the reviews. @FrankYFTang good catch, I've fixed these copy-paste errors.

[FrankYFTang]

I think there are maybe other issues with your PR. I try to change my v8 prototype to this but then got ~700 test breakage in test262 due to throw in step 4 of ParseTimeZoneOffsetString. Still debugging to see where it go wrong (could be my code, not this PR)

[FrankYFTang]

ok, I notice one bug in my code, now I am down to about 30 tests in test262 changed.

[FrankYFTang]

In my prototype, I saw the following tests break after I change to sync with this PR. It might be because I have other bugs in my code. But maybe you would like to review these tests to see does it got changed.

=== test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer ===
=== test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-not-callable ===
=== test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-out-of-range ===
=== test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-wrong-type ===
=== test262/built-ins/Temporal/Duration/compare/timezone-getpossibleinstantsfor-iterable ===
=== test262/built-ins/Temporal/Duration/compare/timezone-string-datetime ===
=== test262/built-ins/Temporal/Duration/prototype/add/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer ===
=== test262/built-ins/Temporal/Duration/prototype/add/relativeto-propertybag-timezone-getoffsetnanosecondsfor-not-callable ===
=== test262/built-ins/Temporal/Duration/prototype/add/relativeto-propertybag-timezone-getoffsetnanosecondsfor-out-of-range ===
=== test262/built-ins/Temporal/Duration/prototype/add/relativeto-propertybag-timezone-getoffsetnanosecondsfor-wrong-type ===
=== test262/built-ins/Temporal/Duration/prototype/add/timezone-getpossibleinstantsfor-iterable ===
=== test262/built-ins/Temporal/Duration/prototype/add/timezone-string-datetime ===
=== test262/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer ===
=== test262/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-timezone-getoffsetnanosecondsfor-not-callable ===
=== test262/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-timezone-getoffsetnanosecondsfor-out-of-range ===
=== test262/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-timezone-getoffsetnanosecondsfor-wrong-type ===
=== test262/built-ins/Temporal/Duration/prototype/round/timezone-getpossibleinstantsfor-iterable ===
=== test262/built-ins/Temporal/Duration/prototype/round/timezone-string-datetime ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/relativeto-propertybag-timezone-getoffsetnanosecondsfor-not-callable ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/relativeto-propertybag-timezone-getoffsetnanosecondsfor-out-of-range ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/relativeto-propertybag-timezone-getoffsetnanosecondsfor-wrong-type ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/timezone-getpossibleinstantsfor-iterable ===
=== test262/built-ins/Temporal/Duration/prototype/subtract/timezone-string-datetime ===
=== test262/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer ===
=== test262/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-timezone-getoffsetnanosecondsfor-not-callable ===
=== test262/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-timezone-getoffsetnanosecondsfor-out-of-range ===
=== test262/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-timezone-getoffsetnanosecondsfor-wrong-type ===
=== test262/built-ins/Temporal/Duration/prototype/total/timezone-string-datetime ===

The following test change from failing to passed.

=== test262/built-ins/Temporal/Instant/from/timezone-custom ===

[ptomato]

@FrankYFTang I didn't check each of these tests, but at least in the first one on the list, test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer.js, no string parsing operations are called at all, so I'm skeptical that it is failing because of this change.

[FrankYFTang]

@FrankYFTang I didn't check each of these tests, but at least in the first one on the list, test262/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-getoffsetnanosecondsfor-non-integer.js, no string parsing operations are called at all, so I'm skeptical that it is failing because of this change.

for that particular one, what happen is the test has
relativeTo: { year: 2000, month: 5, day: 2, hour: 12, timeZone }

so in the code in ToRelativeTemporalObject step 6.j

j. Let timeZone be ? Get(value, "timeZone").

but in your PR that timeZone is ignored later since you are checking timeZoneName not timeZone and no other code read that timeZone . Therefore in the changed PR we no longer call to ToTemporalTimeZone() with that particular timeZone (read it from the object) and therefore no throw inside ToTemporalTimeZone caused by that timeZone will happen.

notice your change is

-      1. If _timeZone_ is not *undefined*, then
-        1. Set _timeZone_ to ? ToTemporalTimeZone(_timeZone_).
+      1. If _timeZoneName_ is not *undefined*, then
+        1. If ! IsValidTimeZoneName(_timeZoneName_) is *false*, throw a *RangeError* exception.
+        1. Set _timeZoneName_ to ! CanonicalizeTimeZoneName(_timeZoneName_).
+        1. Let _timeZone_ be ? ToTemporalTimeZone(_timeZoneName_).

[FrankYFTang]

no string parsing operations are called at all

be aware that your PR changes algorithm more than just "string parsing"

[ptomato]

no string parsing operations are called at all

be aware that your PR changes algorithm more than just "string parsing"

I see, thanks for pointing out how that change occurred. It was a mistake, I think I've corrected it now.

I've also noticed that in the string parsing validation we didn't consistently handle time zone bracketed names that are also offset strings, so I've special-cased that as well. Please let me know if you notice that the current PR changes anything other than the string parsing validation.

[FrankYFTang]

Could you help me out a little bit here. Since you collapsed your chage with the previous commit I have hard time to figure out what was your latest changed in this PR. Did you ONLY chnage the algorithm inside ToRelativeTemporalObject or there are additional changes?

[FrankYFTang]

This use of ParseText is very different than other part of the spec parsing the ISO 8601 grammar. Is there a reason for that? or could it be rewrite to use similar wording like other parsing part?
Is the line the same as
"If timeZoneName does satisfy the syntax of a TimeZoneNumericUTCOffset (see 13.33), then "
as similar (removed the "not") used in https://tc39.es/proposal-temporal/#sec-temporal-parsetimezoneoffsetstring

[ptomato]

@FrankYFTang I've uncollapsed it into the original changes and a second commit with the changes that I made yesterday, hopefully that helps.

The reason for the wording using ParseText is that @gibson042 pointed out that it is more in line with what is done elsewhere in ECMA-262, and that the "If timeZoneName satisfies the syntax of a TimeZoneNumericUTCOffset" language is not as well-defined as ParseText. See #1907 (comment) for more information. I haven't changed it everywhere it appears in the spec text yet, but I'm trying to avoid adding new instances of the old language that would later have to be changed to ParseText.

[ptomato]

This achieved consensus at the December 2021 TC39 meeting.

[ptomato]

@FrankYFTang did my answer address your concerns? If so I'd like to squash this back into one commit and merge the PR.

[ptomato]

@FrankYFTang This has been sitting around for a while, I'm assuming your question was answered.