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

Normative: Use DefaultTimeZone to get the local time zone for Date #2781

Merged
merged 1 commit into from
Oct 9, 2022

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented May 20, 2022

Currently, it is technically possible for the following two operations to
result in different local times and different time zones:

> 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'

This is because Date gets its notion of the current system time zone from
the operation LocalTZA, while Intl.DateTimeFormat uses the operation
DefaultTimeZone from ECMA-402 to determine the current system time zone.

This is a normative change because there was previously no requirement for
these to be linked. However, they should definitely be linked, and no
implementation that includes ECMA-402 that I'm aware of has a discrepancy
between the two. This patch adds this requirement.

This is accomplished by removing LocalTZA (which was actually two
operations rolled into one, anyway) and redefining the LocalTime and UTC
operations in terms of DefaultTimeZone, which moves into ECMA-262, as well
as the two new operations GetNamedTimeZoneOffsetNanoseconds and
GetNamedTimeZoneEpochNanoseconds.

It essentially pushes the "implementation-definedness" of LocalTZA into
GetNamedTimeZoneOffsetNanoseconds, GetNamedTimeZoneEpochNanoseconds, and
DefaultTimeZone.

Although it is a normative change because it adds the requirement for Date
and Intl.DateTimeFormat to use the same default time zone, it should have
no other effects. Despite specifying what LocalTZA used to do in terms of
different operations, the observable effects are the same.

@ptomato
Copy link
Contributor Author

ptomato commented May 20, 2022

@tc39/ecma262-editors This is the PR that we discussed in the editor call yesterday, arising from tc39/proposal-temporal#2171

I put all the new AOs together into the Date section, let me know if they need to go anywhere else. The number of new AOs is not too bad but if needed I can cut it down a bit more:

  • RoundTowardsZero could instead be a mathematical function like floor
  • We could remove the "these are the steps if your implementation only has UTC" parts of the implementation-defined AOs (maybe their utility is questionable?) and that would allow removing GetEpochFromISOParts as well.

@ptomato ptomato requested a review from gibson042 May 20, 2022 01:35
@michaelficarra michaelficarra added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels May 20, 2022
Copy link
Contributor

@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 have some comments around naming and incorrect references to Unix (cf. tc39/proposal-temporal#2175 ), but the bulk of this looks good.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
TimeSeparator[?Extended] MinuteSecond
TimeSeparator[?Extended] MinuteSecond TimeSeparator[?Extended] MinuteSecond Fraction?

UTCOffsetString :::

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone with this for now, but are you sure about having the "Temporal" name in there? It might be confusing since Temporal isn't part of the specification yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahead of the Temporal proposal landing, "temporal" still makes sense as a generic adjective indicating relationship to time—necessary here because date and time representations can use , as a decimal separator and as a negative sign, neither of which are true of non-temporal (i.e., non-time-related) numeric strings in ECMAScript.

Copy link
Contributor

@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 just see a few more grammar points and stray references to IANA, and a possible improvement in operation naming.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@gibson042
Copy link
Contributor

This reached consensus. The promised followup details regarding DST transitions:

The requirements are described in GetNamedTimeZoneEpochNanoseconds (which returns a List of nanoseconds since epoch for a specific time zone name and local time) similar to the current text in LocalTZA: https://arai-a.github.io/ecma262-compare/?pr=2781#sec-getnamedtimezoneepochnanoseconds (emphasis mine)

When the input represents a local time occurring more than once because of a negative time zone transition (e.g. when daylight saving time ends or the time zone offset is decreased due to a time zone rule change), the returned List will have more than one element and will be sorted by ascending numerical value. When the input represents a local time skipped because of a positive time zone transition (e.g. when daylight saving time begins or the time zone offset is increased due to a time zone rule change), the returned List will be empty. Otherwise, the returned List will have one element.

And specified more precisely in UTC (which interprets its input as a number representing local time and returns a single milliseconds since epoch): https://arai-a.github.io/ecma262-compare/?pr=2781#sec-utc-t (emphasis mine)

If possibleInstants is not empty, then… Let disambiguatedInstant be possibleInstants[0]. [i.e., the chronologically first matching local time]
Else [a local time skipped at a positive time zone transition], … Let possibleInstants be GetNamedTimeZoneEpochNanoseconds [corresponding with] tBefore is the largest integral Number < t for which possibleInstants is not empty (i.e., tBefore represents the last local time before the transition) … Let disambiguatedInstant be the last element of possibleInstants

t is interpreted using the time zone offset before the transition

@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 6, 2022
@ptomato
Copy link
Contributor Author

ptomato commented Jun 14, 2022

I'm not sure there's really an effective test262 test possible for this; if I were to write one, it'd depend on implementation-defined date and time zone format strings. What else would be needed for merging this?

@michaelficarra michaelficarra added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Jun 14, 2022
@michaelficarra
Copy link
Member

@ptomato This does not require test262 tests to be merged. Waiting on 262 editor review.

spec.html Outdated Show resolved Hide resolved
@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Oct 5, 2022
spec.html Outdated
Comment on lines 32153 to 32156
<dd>It returns a String value representing the host environment's current time zone, which is either a String representing a UTC offset for which IsTimeZoneOffsetString returns *true*, or a String identifier accepted by GetNamedTimeZoneEpochNanoseconds and GetNamedTimeZoneOffsetNanoseconds.</dd>
</dl>

<p>An ECMAScript implementation that includes the ECMA-402 Internationalization API must implement the DefaultTimeZone abstract operation as specified in the ECMA-402 specification.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

If a valid ECMA-262 implementation can return a UTC offset string from DefaultTimeZone, then ECMA-402 must not prohibit that (cf. API Overview: "This specification introduces new language values observable to ECMAScript code… and also refines the definition of some functions specified in ECMA-262 (as described below). Neither category prohibits behaviour that is otherwise permitted for values and interfaces defined in ECMA-262, in order to support adoption of this specification by any implementation of ECMA-262."). I believe this therefore affirms tc39/ecma402#683 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concretely, should there be any change? Should we remove the paragraph starting "An ECMAScript implementation that includes the ECMA-402 ..." in anticipation of ECMA-402 no longer providing a definition for DefaultTimeZone?

(FWIW, I think ECMA-402's "A conforming implementation must recognize [...] Zone and Link names" makes it OK to also say that the return value of DefaultTimeZone must be one of them, but I see your point as well, so I don't see any reason to be strict about that.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think anything needs to change here, I'm just calling attention to a downstream consequence. We can remove that text when there is no longer an override, but I think it should remain until then.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ptomato
Copy link
Contributor Author

ptomato commented Oct 6, 2022

Rebased; added the changes that Richard requested in a fixup commit.

…c39#2781)

Currently, it is technically possible for the following two operations to
result in different local times and different time zones:

> 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'

This is because Date gets its notion of the current system time zone from
the operation LocalTZA, while Intl.DateTimeFormat uses the operation
DefaultTimeZone from ECMA-402 to determine the current system time zone.

This is a normative change because there was previously no requirement for
these to be linked. However, they *should* definitely be linked, and no
implementation that includes ECMA-402 that I'm aware of has a discrepancy
between the two. This patch adds this requirement.

This is accomplished by removing LocalTZA (which was actually two
operations rolled into one, anyway) and redefining the LocalTime and UTC
operations in terms of DefaultTimeZone, which moves into ECMA-262, as well
as the two new operations GetNamedTimeZoneOffsetNanoseconds and
GetNamedTimeZoneEpochNanoseconds.

It essentially pushes the "implementation-definedness" of LocalTZA into
GetNamedTimeZoneOffsetNanoseconds, GetNamedTimeZoneEpochNanoseconds, and
DefaultTimeZone.

Although it is a normative change because it adds the requirement for Date
and Intl.DateTimeFormat to use the same default time zone, it should have
no other effects. Despite specifying what LocalTZA used to do in terms of
different operations, the observable effects are the same.
@ljharb ljharb merged commit 43fd5f2 into tc39:main Oct 9, 2022
@ptomato ptomato deleted the default-time-zone branch October 10, 2022 14:27
ptomato added a commit to tc39/proposal-temporal that referenced this pull request Oct 11, 2022
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.
ptomato added a commit to tc39/proposal-temporal that referenced this pull request Oct 11, 2022
…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.
@jmdyck jmdyck mentioned this pull request Oct 11, 2022
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this pull request Oct 11, 2022
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.
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this pull request Oct 11, 2022
…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.
justingrant pushed a commit to justingrant/temporal-polyfill that referenced this pull request Apr 15, 2023
…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.

UPSTREAM_COMMIT=654e300db9882fc985840e11f35525864848f8fd
justingrant pushed a commit to justingrant/temporal-polyfill that referenced this pull request Apr 15, 2023
…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.

UPSTREAM_COMMIT=654e300db9882fc985840e11f35525864848f8fd
justingrant pushed a commit to js-temporal/temporal-polyfill that referenced this pull request Apr 15, 2023
…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.

UPSTREAM_COMMIT=654e300db9882fc985840e11f35525864848f8fd
justingrant added a commit to justingrant/ecma262 that referenced this pull request May 23, 2024
Following ISO-8601, tc39#2781 introduced U+2212 (Unicode minus) as an alias
for the regular ASCII minus sign for use in time zone offsets.

There's two new data that lead me to believe that this was a mistake,
and that we should revert this change.

The first is that the newly-released RFC 9557 (the string format
standard that Temporal uses) disallows non-ASCII characters. Its
predecessor RFC 3339 also disallows non-ASCII characters. So
strings that follow the current (since 2022) ECMAScript spec
could be rejected by RFC 9557 clients.

The second new data is feedback from implementers of a Rust version of
Temporal that this single obscure character in the grammar will incur a
performance cost because they must now use Rust strings instead
of plain U8 ASCII data. See
tc39/proposal-temporal#2843 (comment)

This performance issue doesn't seem to be limited to Rust. Any
native implementation would likely benefit from being able to know that
valid date/time input (both Date and Temporal) is always ASCII-only.

I don't know whether all engines have actually implemented this 2022
grammar change. But it's also a safe bet that real-world usage of this
Unicode character is likely minimal. So the web-compat risk seems small.

If this PR is accepted, then we'll follow up with a normative Temporal
PR to remove this character from Temporal as well.
justingrant added a commit to justingrant/ecma262 that referenced this pull request Jul 2, 2024
Following ISO-8601, tc39#2781 introduced U+2212 (Unicode minus) as an alias
for the regular ASCII minus sign for use in time zone offsets.

There's two new data that lead me to believe that this was a mistake,
and that we should revert this change.

The first is that the newly-released RFC 9557 (the string format
standard that Temporal uses) disallows non-ASCII characters. Its
predecessor RFC 3339 also disallows non-ASCII characters. So
strings that follow the current (since 2022) ECMAScript spec
could be rejected by RFC 9557 clients.

The second new data is feedback from implementers of a Rust version of
Temporal that this single obscure character in the grammar will incur a
performance cost because they must now use Rust strings instead
of plain U8 ASCII data. See
tc39/proposal-temporal#2843 (comment)

This performance issue doesn't seem to be limited to Rust. Any
native implementation would likely benefit from being able to know that
valid date/time input (both Date and Temporal) is always ASCII-only.

I don't know whether all engines have actually implemented this 2022
grammar change. But it's also a safe bet that real-world usage of this
Unicode character is likely minimal. So the web-compat risk seems small.

If this PR is accepted, then we'll follow up with a normative Temporal
PR to remove this character from Temporal as well.
ljharb pushed a commit to justingrant/ecma262 that referenced this pull request Jul 17, 2024
Following ISO-8601, tc39#2781 introduced U+2212 (Unicode minus) as an alias
for the regular ASCII minus sign for use in time zone offsets.

There's two new data that lead me to believe that this was a mistake,
and that we should revert this change.

The first is that the newly-released RFC 9557 (the string format
standard that Temporal uses) disallows non-ASCII characters. Its
predecessor RFC 3339 also disallows non-ASCII characters. So
strings that follow the current (since 2022) ECMAScript spec
could be rejected by RFC 9557 clients.

The second new data is feedback from implementers of a Rust version of
Temporal that this single obscure character in the grammar will incur a
performance cost because they must now use Rust strings instead
of plain U8 ASCII data. See
tc39/proposal-temporal#2843 (comment)

This performance issue doesn't seem to be limited to Rust. Any
native implementation would likely benefit from being able to know that
valid date/time input (both Date and Temporal) is always ASCII-only.

I don't know whether all engines have actually implemented this 2022
grammar change. But it's also a safe bet that real-world usage of this
Unicode character is likely minimal. So the web-compat risk seems small.

If this PR is accepted, then we'll follow up with a normative Temporal
PR to remove this character from Temporal as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants