Normative: Limit offset time zones to minutes precision by justingrant · Pull Request #2607 · tc39/proposal-temporal

justingrant · 2023-06-14T10:06:07Z

This PR limits offset time zones to minutes precision to address implementer feedback that the storage requirements of Temporal.TimeZone are too large. This PR reduces Temporal.TimeZone storage requirements from 49 bits to 12-13 bits.

The first normative commit changes |TimeZoneUTCOffsetName| to minutes precision and moves sub-minute offset formatting from FormatOffsetTimeZoneIdentifier to GetOffsetStringFor.

The second normative commit ensures that SystemTimeZoneIdentifier cannot return offsets greater than minute precision, and ensures that any offsets are returned in normalized form (e.g. -05:00 not -05 nor -0500). Note that no current implementation returns offsets from SystemTimeZoneIdentifier, so although this commit is a normative change to an existing ECMA-262 abstract operation, it won't require changes to existing implementation nor will it break existing ECMAScript programs.

This PR fixes #2593 and fixes #2584, and is marked as a draft until it can be presented at the July 2023 TC39 plenary meeting.

Tests are at tc39/test262#3862.

gibson042

I got a little in, but honestly am feeling overwhelmed by the plethora of abstract operations. For me at least, I think the splits hinder comprehension.

codecov · 2023-06-15T04:17:00Z

Codecov Report

Merging #2607 (fd7fccd) into main (9253db8) will decrease coverage by 0.03%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##             main    #2607      +/-   ##
==========================================
- Coverage   95.96%   95.94%   -0.03%     
==========================================
  Files          20       20              
  Lines       11536    11550      +14     
  Branches     2191     2197       +6     
==========================================
+ Hits        11071    11082      +11     
- Misses        401      404       +3     
  Partials       64       64

Impacted Files	Coverage Δ
polyfill/lib/ecmascript.mjs	`98.21% <91.17%> (-0.05%)`	⬇️
polyfill/lib/regex.mjs	`100.00% <100.00%> (ø)`

justingrant · 2023-06-16T10:26:53Z

V2 is available. It's a refactor based on @gibson042's advice around parsing and grammar.

Let me know if you see problems with this version. If not then I'll move onto Test262.

justingrant · 2023-06-16T10:27:31Z

Also, I updated the OP so you can see a summary of the latest design.

justingrant · 2023-06-18T01:01:27Z

@gibson042 @ptomato PR is now ready for review. I split it up into one editorial commit that refactors offset handling, and two normative commits:

A small one that ensures that SystemTimeZoneIdentifier only returns offset strings in normal form
A big one that makes the nanoseconds=>minutes change for offset time zones

I'll work on Test262 next.

gibson042

There's a lot of good work here, but I think a little more is still warranted.

tc39/proposal-temporal#2607 was approved in the July 2023 TC39 meeting, which removes the need for this proposal to change SystemTimeZoneIdentifier.

This editorial commit refactors FormatSecondsStringPart to include similar editorial changes made to GetOffsetStringFor in tc39#2607.

justingrant · 2023-07-16T01:10:59Z

All review comments are addressed and PR is rebased to main.

Tests over in tc39/test262#3862 are I think also ready to merge if we're OK merging them despite concerns in https://github.com/tc39/test262/pull/3862/files#r1259590953 that I'd prefer to defer for now.

This editorial commit refactors FormatSecondsStringPart to include similar editorial changes made to GetOffsetStringFor in tc39#2607.

This editorial commit removes redundant spec text dealing with time formatting: * Adds a new AO FormatTimeString, and calls it in TemporalTimeToString, TemporalDateTimeToString, and TimeString (the legacy date AO in 262). * Removes FormatSecondsStringPart and replaces it with a new AO FormatFractionalSeconds, and call it in FormatTimeString and TemporalDurationToString. The text of this new AO is aligned with similar text in GetOffsetStringFor in tc39#2607. * Replaces sub-second formatting text in TemporalDurationToString with a call to FormatFractionalSeconds. * Aligns polyfill code to these spec changes. * Adjusts polyfill code in a few places to better match the spec. Note that this commit doesn't touch spec text for formatting time zone offsets because there are several in-flight PRs dealing with offsets and I wanted to keep this PR merge-conflict-free. But once those PRs land and the dust settles, then I'll send another editorial PR to DRY offset string formatting too, by using FormatTimeString to replace bespoke formatting text for offsets.

This editorial commit removes redundant spec text dealing with time formatting: * Adds a new AO FormatTimeString, and calls it in TemporalTimeToString, TemporalDateTimeToString, and TimeString (the legacy date AO in 262). * Removes FormatSecondsStringPart and replaces it with a new AO FormatFractionalSeconds, and call it in FormatTimeString and TemporalDurationToString. The text of this new AO is aligned with similar text in GetOffsetStringFor in #2607. * Replaces sub-second formatting text in TemporalDurationToString with a call to FormatFractionalSeconds. * Aligns polyfill code to these spec changes. * Adjusts polyfill code in a few places to better match the spec. Note that this commit doesn't touch spec text for formatting time zone offsets because there are several in-flight PRs dealing with offsets and I wanted to keep this PR merge-conflict-free. But once those PRs land and the dust settles, then I'll send another editorial PR to DRY offset string formatting too, by using FormatTimeString to replace bespoke formatting text for offsets.

Modify/add tests for tc39/proposal-temporal#2607

justingrant · 2023-07-17T19:50:05Z

I think it's fine to make those minor editorial changes in the coming days. Let me know when everyone is happy about it, and I'll merge tc39/test262#3862 and follow it up by pushing a commit to update test262 and then merge this.

@ptomato I think this one is ready to merge. After it's merged I can rebase #2574 on the updated main, and then that PR should be ready to go as well after tests are reviewed and merged.

At implementers' request to reduce the storage requirements of Temporal.TimeZone from 49+ bits to 12-13 bits, this commit requires that the [[OffsetNanoseconds]] internal slot of Temporal.TimeZone is limited to minute precision. Sub-minute precision is still allowed for custom time zone objects and built-in named time zones. In other words, this commit changes storage requirements but not internal calculation requirements. This commit is fairly narrow: * Changes |TimeZoneUTCOffsetName| production to restrict allowed offset syntax for parsing. * Changes FormatOffsetTimeZoneIdentifier AO to format minute strings only. * Moves sub-minute offset formatting from FormatOffsetTimeZoneIdentifier to instead be inlined in GetOffsetStringFor, which is now the only place where sub-minute offsets are formatted. Fixes tc39#2593.

This commit adds a "normalized format of a time zone identifier" definition, and uses it to ensure that SystemTimeZoneIdentifier only returns normalized identifiers like "-05:00" (not "-0500" nor "-05") or "America/Los_Angeles" (not "AMERICA/LOS_ANGELES" nor "AmErIcA/lOs_AnGeLeS"). No current implementations return offset strings in non-normalized format, so although this is a normative change, it won't break existing ECMAScript code nor will it require changes to current implementations. Fixes tc39#2584.

justingrant requested review from gibson042 and ptomato June 14, 2023 10:06

gibson042 reviewed Jun 14, 2023

View reviewed changes

justingrant force-pushed the offset-timezone-minutes branch from ae020e9 to 1dd7030 Compare June 15, 2023 04:08

justingrant force-pushed the offset-timezone-minutes branch 11 times, most recently from 9f1fc24 to 53f8027 Compare June 16, 2023 10:10

justingrant requested a review from gibson042 June 16, 2023 10:27

justingrant force-pushed the offset-timezone-minutes branch 7 times, most recently from 09cec14 to f6818ce Compare June 18, 2023 00:49

gibson042 requested changes Jun 18, 2023

View reviewed changes

justingrant changed the title ~~WIP: Limit offset time zones to minutes precision~~ DRAFT: Limit offset time zones to minutes precision Jun 18, 2023

justingrant force-pushed the offset-timezone-minutes branch from f6818ce to 123cb3e Compare June 18, 2023 20:21

ptomato mentioned this pull request Jul 13, 2023

Final normative spec text plan #2628

Closed

91 tasks

justingrant force-pushed the offset-timezone-minutes branch from dda8f6c to 199a94b Compare July 15, 2023 23:45

justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Jul 15, 2023

Editorial: refactor FormatSecondsStringPart

630ff7a

This editorial commit refactors FormatSecondsStringPart to include similar editorial changes made to GetOffsetStringFor in tc39#2607.

justingrant mentioned this pull request Jul 15, 2023

Editorial: DRY refactor of time formatting #2629

Merged

justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Jul 15, 2023

Editorial: refactor FormatSecondsStringPart

281f1bc

This editorial commit refactors FormatSecondsStringPart to include similar editorial changes made to GetOffsetStringFor in tc39#2607.

justingrant force-pushed the offset-timezone-minutes branch 2 times, most recently from 33ae966 to 5345608 Compare July 16, 2023 00:20

gibson042 reviewed Jul 16, 2023

View reviewed changes

Comment thread spec/timezone.html

justingrant marked this pull request as ready for review July 16, 2023 04:41

justingrant marked this pull request as draft July 16, 2023 04:42

justingrant marked this pull request as ready for review July 16, 2023 05:47

justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Jul 16, 2023

Editorial: refactor FormatSecondsStringPart

c409664

This editorial commit refactors FormatSecondsStringPart to include similar editorial changes made to GetOffsetStringFor in tc39#2607.

justingrant added a commit to justingrant/test262 that referenced this pull request Jul 17, 2023

Limit offset time zones to minute precision

9e1758d

Modify/add tests for tc39/proposal-temporal#2607

justingrant force-pushed the offset-timezone-minutes branch 2 times, most recently from 2c3a752 to 36487a6 Compare July 17, 2023 19:42

justingrant mentioned this pull request Jul 17, 2023

Normative: don't coerce non-string primitives to strings, esp. time zones, offsets, and calendars #2574

Merged

justingrant added 3 commits July 18, 2023 08:27

Update Test262

fd7fccd

This was referenced Jul 18, 2023

Editorial: Refactor offset string formatting #2632

Merged

Info from Mozilla on iCalendar spec #2631

Closed

Temporal: Test rejecting ISO strings with subminute offsets tc39/test262#3877

Merged

justingrant mentioned this pull request Sep 14, 2023

Fix getOffsetStringFor docs for sub-minute offsets #2674

Merged

Conversation

justingrant commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibson042 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

justingrant commented Jun 16, 2023

Uh oh!

justingrant commented Jun 16, 2023

Uh oh!

justingrant commented Jun 18, 2023

Uh oh!

gibson042 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justingrant commented Jul 16, 2023

Uh oh!

Uh oh!

justingrant commented Jul 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justingrant commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading