Editorial: Simpler time zone parsing in ToTemporalTimeZoneSlotValue

[anba]

When directly calling ParseTimeZoneIdentifier in ParseTemporalTimeZoneString, the time zone parsing in ToTemporalTimeZoneSlotValue can be simplified.

Also fixes #2639.

And fixes an incorrect ? ParseTimeZoneIdentifier call in ToTemporalTimeZoneSlotValue, which is actually infallible and therefore should have been ! ParseTimeZoneIdentifier.

And fixes an incorrect offsetParseResult.[[OffsetMinutes]] access in ToTemporalTimeZoneSlotValue, which should have been offsetParseResult.[[OffsetNanoseconds]] / (60 * 10^9).

[codecov]

Codecov Report

Merging #2641 (63dbee9) into main (768bc99) will decrease coverage by 0.01%.
The diff coverage is 98.85%.

❗ Current head 63dbee9 differs from pull request most recent head d69c59f. Consider uploading reports for the commit d69c59f to get more accurate results

@@            Coverage Diff             @@
##             main    #2641      +/-   ##
==========================================
- Coverage   96.06%   96.05%   -0.01%     
==========================================
  Files          20       20              
  Lines       11553    11594      +41     
  Branches     2195     2193       -2     
==========================================
+ Hits        11098    11137      +39     
- Misses        391      393       +2     
  Partials       64       64              

Files Changed	Coverage Δ
polyfill/lib/ecmascript.mjs	98.41% <98.80%> (-0.03%)	⬇️
polyfill/lib/regex.mjs	100.00% <100.00%> (ø)
polyfill/lib/zoneddatetime.mjs	100.00% <100.00%> (ø)

[justingrant]

I had a few minor comments, otherwise looks good to me. Nice simplification.

Before merging, I'd like to get either @ptomato and/or @gibson042 to take a look too.

We should probably also update the polyfill to keep it in sync with the changed spec. @anba if you don't want to do this, let me know and I can add a polyfill commit to this PR.

Note that this PR overlaps with #2633 so whichever one is merged second will need to be rebased.

[anba]

We should probably also update the polyfill to keep it in sync with the changed spec. @anba if you don't want to do this, let me know and I can add a polyfill commit to this PR.

I don't have anything set-up to build and test the polyfill, so it'd be better if someone else can update the polyfill code. (And maybe also fix the polyfill to correctly reject Temporal.TimeZone.from("0000-01-01T00:00+00:00:00"). 😄)

[ptomato]

Seems like good improvements to me.

[justingrant]

@anba - could you rebase this PR now that #2633 has been merged? I suspect that the only conflict is the change from [[PrimaryIdentifier]] to [[Identifier]] in ToTemporalTimeZoneSlotValue. Thanks!

I don't have anything set-up to build and test the polyfill, so it'd be better if someone else can update the polyfill code.

OK I'll add a polyfill commit to this PR later this week.

(And maybe also fix the polyfill to correctly reject Temporal.TimeZone.from("0000-01-01T00:00+00:00:00"). 😄)

Sure, I can fix this too! Thanks for finding this bug.

[justingrant]

(And maybe also fix the polyfill to correctly reject Temporal.TimeZone.from("0000-01-01T00:00+00:00:00"). 😄)

It turns out that this bug was fixed already in #2635. As soon as you rebase then it will be gone. See #2635 (comment).

But the tests for #2635 didn't actually test strings with trailing zeroes, which was a mistake. (My mistake!). Fixed here: tc39/test262#3892.

[justingrant]

Polyfill changes are done. @ptomato, would you mind taking a quick look at them?

As part of doing the polyfill changes, I also found some Test262 bugs and gaps, and fixed a polyfill bug (in a separate commit). Updated tests are in tc39/test262#3893 and tc39/test262#3892. This PR should not be merged until those tests are merged.

could you rebase this PR now that #2633 has been merged?

@anba, I ended up needing to rebase to make the polyfill changes anyway, so I just pushed the rebase and fixed the one-line merge conflict.

[justingrant]

I also found some Test262 bugs and gaps, and fixed a polyfill bug (in a separate commit). Updated tests are in tc39/test262#3893 and tc39/test262#3892. This PR should not be merged until those tests are merged.

Test262 PRs are now both merged, so this PR should be ready to merge after the polyfill changes are reviewed.

[ptomato]

Looks good, but I think there must be a less complicated way to test the parser and I'd really prefer not to make that test any more complicated. Let me know if you'd like me to dig into this myself.

[justingrant]

Let me know if you'd like me to dig into this myself.

Yes, I would really like your help here! I spent many hours on this last night and still wasn't sure if I ended up with a good solution.

The core problem to solve is that the new ParseTemporalTimeZone AO returns a record that acts differently from other parsing methods.

Other ecmascript.mjs parsing methods for non-TimeZone types returns a record including a separate component for each different part of the string. For example, ParseTemporalZonedDateTimeString includes an offset and a tzAnnotation component, even if they both represent the same offset, and it returns a z component even though the resulting ZDT instance has no idea whether a Z designator was present in the original string.

However, the new ParseTemporalTimeZone returns a mutually-exclusive record that's has either an offsetMinutes (which in turn could have come from the ISO string offset or an offset in the annotation) or a tzName (that could come from a Z designator on an Instant string or from an IANA name in the annotation). This new ParseTemporalTimeZone makes it much clearer for callers and yields a simpler an non-DRY implementation, but it made it more confusing to integrate with validStrings.mjs's assumption that validating parsing involves comparing the parts injected into the fuzzed data to the results of the parsing method.

BTW, while working on the tests I realized that I'd earlier tried a similar (although not as elegant) refactoring as Anba's, but backed it out because I didn't want to invest the time required to figure out how to make it work with validStrings.

I'd be grateful if you have an idea how to rationalize this. I guess one naive way would to split TimeZone parsing into a lower-level parsing method that's validStrings-friendly that essentially just returns the z, offset, and tzAnnotation fields of ParseISODateTime, and a higher-level method that calls the lower-level one. I'm sure there must be other ways to solve this too.

Thanks!

[ptomato]

I'd be grateful if you have an idea how to rationalize this. I guess one naive way would to split TimeZone parsing into a lower-level parsing method that's validStrings-friendly that essentially just returns the z, offset, and tzAnnotation fields of ParseISODateTime, and a higher-level method that calls the lower-level one. I'm sure there must be other ways to solve this too.

I've added another commit doing basically this. Take a look when you have time and let me know what you think.

[justingrant]

one naive way would to split TimeZone parsing into a lower-level parsing method that's validStrings-friendly that essentially just returns the z, offset, and tzAnnotation fields of ParseISODateTime, and a higher-level method that calls the lower-level one. I'm sure there must be other ways to solve this too.

I've added another commit doing basically this. Take a look when you have time and let me know what you think.

I like it! I'd like to avoid deviating from the spec so much though. Ideally we'd add the low-level parsing method with a new name, and leave ParseTemporalTimeZone with the same shape as the spec. I have an idea for how to do this: add a ES.ParseTemporalTimeZoneRaw (bikeshed welcome!) method, and then change validStrings.mjs to try the "*Raw" variant of each mode. This avoids special-casing and would allow similar refactors later if we want to.

Also, your validStrings.mjs changes don't seem to be testing the Z case, so I'll try to fix that and you can review both changes tomorrow. Thanks for your help!

[justingrant]

I made the changes described in the comment above:

• Swapped names so that the outer parsing operation has the same name as the spec (ParseTemporalTimeZoneString) and the inner parsing operation (that doesn't exist in the spec) is called ParseTemporalTimeZoneStringRaw
• Added validStrings test coverage for parsing the z component for TimeZone, Instant, and ZonedDateTime

@ptomato Want to take a look?

[ptomato]

Perfect, thanks for the collaboration!

BTW, the name ParseTemporalTimeZoneStringRaw was also my first choice, great minds think alike! But I discarded it because I didn't want to change validStrings to try to call *Raw methods. However, now that you've done it that way, it looks fine.