Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Redefine time zone handling in legacy Date in terms of DefaultTimeZone #2171

Merged
merged 16 commits into from
Oct 11, 2022

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented May 4, 2022

The intention of this change is to ensure that it is impossible for
Temporal to support a different set of time zones than legacy Date.

Technically, legacy Date doesn't "support" any time zones — its data model
is the same as Temporal.Instant — but calls like new Date(y, m, d, ...)
interpret the given date and time as a local time in the system's current
time zone. We need to ensure that this vaguely defined "current time zone"
is conceptually the same time zone that Temporal.Now.timeZone() returns.

After tc39/ecma262#2781, this is pretty easy to achieve, by using DefaultTimeZone. The hard work was done in that PR, this is now mostly just a rebase, but it also adds a new AvailableTimeZones abstract operation.

This should affect absolutely nothing for implementations. It's just a
more formal guarantee of what was already stipulated.

Closes: #519

@ptomato ptomato requested review from Ms2ger, sffc and gibson042 May 4, 2022 00:59
@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #2171 (11352bc) into main (341aae8) will increase coverage by 0.30%.
The diff coverage is 100.00%.

❗ Current head 11352bc differs from pull request most recent head b698b54. Consider uploading reports for the commit b698b54 to get more accurate results

@@            Coverage Diff             @@
##             main    #2171      +/-   ##
==========================================
+ Coverage   94.73%   95.04%   +0.30%     
==========================================
  Files          20       20              
  Lines       10939    10851      -88     
  Branches     1947     1929      -18     
==========================================
- Hits        10363    10313      -50     
+ Misses        532      504      -28     
+ Partials       44       34      -10     
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.31% <100.00%> (-0.33%) ⬇️
polyfill/lib/timezone.mjs 100.00% <100.00%> (+0.63%) ⬆️
polyfill/lib/plainmonthday.mjs 95.34% <0.00%> (-0.37%) ⬇️
polyfill/lib/plainyearmonth.mjs 96.73% <0.00%> (-0.19%) ⬇️
polyfill/lib/zoneddatetime.mjs 99.81% <0.00%> (-0.01%) ⬇️
polyfill/lib/regex.mjs 100.00% <0.00%> (ø)
polyfill/lib/plaindate.mjs 99.01% <0.00%> (+0.33%) ⬆️
polyfill/lib/plaindatetime.mjs 99.36% <0.00%> (+1.49%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ptomato
Copy link
Collaborator Author

ptomato commented May 4, 2022

Although this is an editorial change that should have absolutely no effect on implementations, it modifies a weird and scary part of 262. I'd like to get the opinion of @tc39/ecma262-editors if this is a direction that you agree with. I can join the editors call if you want to discuss it.

Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it might be useful to make these changes to 262 before landing all of temporal, but I guess it might depend on too much to usefully extricate.

spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
@ptomato
Copy link
Collaborator Author

ptomato commented May 4, 2022

Re. adding this to 262 separately: I could rewrite this not to use CreateTemporalTimeZone if that was the direction we wanted to go in. Then it wouldn't depend on any Temporal objects and would only pull in:

  • DefaultTimeZone
  • IsValidTimeZoneName
  • AvailableTimeZones
  • CanonicalizeTimeZoneName
  • GetIANATimeZoneOffsetNanoseconds
  • RoundTowardsZero
  • GetIANATimeZoneEpochValue
  • GetEpochFromISOParts
  • BalanceISODateTime
  • BalanceTime
  • BalanceISODate
  • BalanceISOYearMonth
  • ISODaysInYear
  • ISODaysInMonth
  • ParseTimeZoneOffsetString
  • The grammar for TimeZoneNumericUTCOffset

I could look into rewriting the BalanceISODateTime step to use MakeDate, MakeTime, etc. which would cut out a big chunk of the above list.

It would probably require resolving tc39/proposal-intl-enumeration#37 first.

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good overall, but I do have some questions.

spec/intl.html Outdated Show resolved Hide resolved
spec/intl.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/timezone.html Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the available-time-zones branch 3 times, most recently from 885227e to 3a3eee4 Compare May 5, 2022 23:31
@ptomato
Copy link
Collaborator Author

ptomato commented May 5, 2022

Re. adding this to 262 separately: I could rewrite this not to use CreateTemporalTimeZone if that was the direction we wanted to go in.

I've done this rewrite, in separate commits so you can easily see what changed.

Adding this to 262 would now only pull in:

  • DefaultTimeZone
  • ParseTimeZoneOffsetString
  • the grammar of TimeZoneNumericUTCOffset
  • GetIANATimeZoneEpochValue
  • GetEpochFromISOParts
  • GetIANATimeZoneOffsetNanoseconds
  • RoundTowardsZero

If we wanted, additionally:

  • IsValidTimeZoneName
  • CanonicalizeTimeZoneName
  • AvailableTimeZones
  • IsValidISODate (but only in an assertion, so could be omitted)
  • ISODaysInMonth (ditto)

spec/timezone.html Outdated Show resolved Hide resolved
spec/intl.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/intl.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/timezone.html Show resolved Hide resolved
Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend a few tweaks, but this is good!

@ptomato ptomato force-pushed the available-time-zones branch 2 times, most recently from 86b13bb to 2262e04 Compare May 19, 2022 20:39
<dd>true</dd>
</dl>
<emu-alg>
1. Let _timeZones_ be a List containing the String value of each Zone or Link name in the IANA Time Zone Database that is supported by the implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let _timeZones_ be a List containing the String value of each Zone or Link name in the IANA Time Zone Database that is supported by the implementation.
1. Let _timeZones_ be the List of String values representing time zones supported by the implementation, including supported aliases.

If an ECMAScript implementation does not include the ECMA-402 API the following specification of the AvailableTimeZones abstract operation is used.
</p>
<emu-alg>
1. Let _timeZones_ be the List of String values representing time zones supported by the implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let _timeZones_ be the List of String values representing time zones supported by the implementation.
1. Let _timeZones_ be the List of String values representing time zones supported by the implementation, including supported aliases.

</p>
<p>
Once IsValidTimeZoneName(_timeZone_) has returned *true*, for the lifetime of the surrounding agent, IsValidTimeZoneName(_variant_) must return *true* if _variant_ is an ASCII-case-insensitive match for either _timeZone_ or CanonicalizeTimeZoneName(_timeZone_).
Once IsAvailableTimeZoneName(_timeZone_) has returned *true*, for the lifetime of the surrounding agent, IsAvailableTimeZoneName(_variant_) must return *true* if _variant_ is an ASCII-case-insensitive match for either _timeZone_ or CanonicalizeTimeZoneName(_timeZone_).
</p>
<p>
For the purposes of this section, a String value _A_ is an ASCII-case-insensitive match for String value _B_ if the String value derived from _A_ by replacing each occurrence of a lowercase ASCII letter code unit (0x0061 through 0x007A, inclusive) with the corresponding uppercase ASCII letter code unit (0x0041 through 0x005A, inclusive) while preserving all other code units is exactly the same sequence of code units as the String value that is derived from _B_ in the same way.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ASCII-case-insensitive" is now also used in CanonicalizeTimeZoneName, so this definition should move up at least a level here (and ultimately apply to all of ECMA-262, cf. tc39/ecma402#630 (comment) ).

<p>
  The <dfn>ASCII-uppercase</dfn> of a String value _S_ is the String value derived from _S_ by replacing each occurrence of an ASCII lowercase letter code unit (0x0061 through 0x007A, inclusive) with the corresponding ASCII uppercase letter code unit (0x0041 through 0x005A, inclusive) while preserving all other code units.
</p>
<p>
  A String value _A_ is an <dfn>ASCII-case-insensitive match</dfn> for String value _B_ if the ASCII-uppercase of _A_ is exactly the same sequence of code units as the ASCII-uppercase of _B_. A sequence of Unicode code points _A_ is an ASCII-case-insensitive match for _B_ if _B_ is an ASCII-case-insensitive match for ! CodePointsToString(_A_).
</p>

@ptomato
Copy link
Collaborator Author

ptomato commented May 19, 2022

I brought this to the 262 editors call yesterday. The conclusion is, this seems like a good thing to stage into ECMA-262 in advance of Temporal, because (1) it reduces the size of the Temporal proposal, (2) it cleans up the gnarly LocalTZA operation.

However, it is technically a normative change because it links Date and Intl.DateTimeFormat through DefaultTimeZone. Currently, it is possible for the following two operations to take totally different default time zones and therefore result in different local times and time zone names:

> date = new Date()
> date.toTimeString()
'13:43:47 GMT-0700 (Pacific Daylight Time)'
> new Intl.DateTimeFormat(undefined, {timeStyle: 'full'}).format(date)
'1:43:47 p.m. Pacific Daylight Time'

None of the Intl-supporting implementations that I have in my eshost-cli actually differ here, and although it seems unlikely that any would, still, linking these two operations is desirable, and that's a third reason to put this into 262 in advance of Temporal.

The next steps are for me to prepare a PR for https://github.com/tc39/ecma262 that introduces the relevant parts of this PR, and pulls in whatever abstract operations are needed from Temporal. This should make it easier for the editors to review, without the <ins> and <del> blocks which are necessary in this PR. Depending on how big the PR is, it may still be reasonable to conclude that it pulls in too much, and we don't want to deal with it right now.

<ins class="block">
<emu-clause id="sup-availabletimezones" type="implementation-defined abstract operation">
<h1>
AvailableTimeZones (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to land this commit ahead of intl-enumeration changing to match?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not ☹️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intl-enumeration is not going to change to match, so unfortunately I think the best thing to do here is to keep this as it is, but later try to get it into ECMA-402 via an editorial PR once intl-enumeration reaches stage 4.

This will be necessary for the rebase on
tc39/ecma262#2781

Delete the reference to LocalTZA which no longer exists in ECMA-262; this
reference was already inside text to be deleted from ECMA-402 anyway.

EnumerableOwnPropertyNames was renamed to EnumerableOwnProperties in
tc39/ecma262#2899 so that change also needs to be
made when pulling in the new biblio.
RoundTowardsZero was already merged into ECMA-262 as a new mathematical
function named 'truncate'. RoundTowardsZero no longer needs to be part of
this proposal.
GetEpochFromISOParts was already merged into ECMA-262 and renamed to
GetUTCEpochNanoseconds. Its IsValidISODate assertion was removed, so add
that in the one place where it isn't redundant.
…anoseconds

GetIANATimeZoneEpochValue was already merged into ECMA-262 and renamed to
GetNamedTimeZoneEpochNanoseconds.
…eOffsetNanoseconds

GetIANATimeZoneOffsetNanoseconds was already merged into ECMA-262 and
renamed to GetNamedTimeZoneOffsetNanoseconds. The order of its two
arguments was swapped for consistency with
GetNamedTimeZoneEpochNanoseconds.
DefaultTimeZone was already moved from ECMA-402 into ECMA-262, so this
proposal no longer has to do it.
…tString

During the editorial process for tc39/ecma262#2781
ParseTimeZoneOffsetString was changed to be infallible, and a second
operation IsTimeZoneOffsetString was added as a predicate for whether a
given string is parseable as a UTC offset.
ECMA-262 now has a UTCOffset grammar nonterminal, so we can remove the
TimeZoneNumericUTCOffset nonterminal and use the one from ECMA-262
instead.
…xtTransition

This rename, as well as the reordering of the two arguments, is for
consistency with now-existing ECMA-262 operations such as
GetNamedTimeZoneEpochNanoseconds.
…nePreviousTransition

This rename, as well as the reordering of the two arguments, is for
consistency with now-existing ECMA-262 operations such as
GetNamedTimeZoneEpochNanoseconds.
For consistency with other operations in the spec, rename this function to
GetNamedTimeZone... and swap its arguments.
(This has no effect on the proposal, as it's only used in parts of the
polyfill that implement the TZDB, which is not specified. This is only for
consistency in the code.)
AvailableTimeZones is an abstract operation defined in the
Intl.Enumeration proposal (though there is a small difference, see below.)
Sharing AvailableTimeZones between ECMA-262 and ECMA-402 should allow us
to stipulate that if an implementation supports any time zone for
formatting in Intl.DateTimeFormat, it must support it for Temporal as
well.

This change means IsValidTimeZoneName is no longer implementation-defined,
and does not need to exist in ECMA-402, only in 262.

Note, this requires a slightly different algorithm for AvailableTimeZones
than the one in the Intl.Enumeration proposal. Intl.Enumeration considered
it too risky for their Stage 4 bid to change this, despite being an
unobservable change, so we will have to make changes in ECMA-402 later,
after Intl.Enumeration reaches Stage 4.

See: #541
See: #519
For implementation-defined operations having to do with time zones, it
seems useful to have a minimum implementation spelled out, for
implementations that do not have local political rules for time zones.

We already do this for AvailableTimeZones and CanonicalizeTimeZoneName.
CanonicalizeTimeZoneName must only be called with names for which
IsValidTimeZoneName returns true. In the minimal implementation, this is
only UTC.
In order to align with AvailableTimeZones and ECMA-402 terminology, use
"Available" in the name of the operation instead of "Valid".
With the addition of DefaultTimeZone to ECMA-262, there should be nothing
left to define from this note.

Closes: #519
@Ms2ger Ms2ger merged commit 21ee5b1 into main Oct 11, 2022
@Ms2ger Ms2ger deleted the available-time-zones branch October 11, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure Temporal.now.timeZone() returns same time zone as new Date()
3 participants