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: specify time zone ID requirements to reduce divergence between engines #877

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Mar 31, 2024

This proposed change resolves #825 by adding normative spec text to clarify how ECMA-402 implementations should decide which IANA time zone IDs should be primary vs. non-primary. This will enable more consistency between ECMAScript implementations and prevent future divergence.

This PR also accommodates to web reality by aligning ECMA-402 with CLDR and ICU. This should make it easier for all ECMAScript engines to comply with the spec while still being able to use ICU.

This PR is stacked on #876, so please ignore the first commit when reviewing this PR.

Note that the problem of out-of-date primary IDs for renamed Zones (like Asia/Calcutta) is out of scope to this PR, because the spec already requires current IDs, and there's already a plan to fix it that requires no spec changes.

Summary of proposed changes

This PR implements "Option C" in #825 by deterministically defining ECMAScript's exceptions from the IANA Time Zone Database's defaults, and then pointing implementers at ICU as a convenient implementation of those exceptions.

We'll start with a baseline of IANA's Zone and Link names and specify a few exceptions:

  1. Existing special-cases for Etc/UTC, Etc/GMT, and GMT will be retained.
  2. Primary and non-primary IDs must share the same ISO-3166-2 country code, so IANA's time zone merges between countries will be reverted. CLDR and ICU already disallow merges across country boundaries. Note that IANA's merges of time zones *inside* of a country (e.g. Asia/Chongquing=>Asia/Shanghai) will be accepted, also matching current ICU/CLDR behavior.

This PR also makes two other smaller text changes that we expect to have zero impact on current engines:

  • Changes Editorial: align time zone identifier text and AOs with ECMA-262 #876's recommendations into requirements to limit observable dynamic updating of TZDB info. AFAIK, no existing ECMAScript engine updates TZDB (observably or otherwise!) during the lifetime of the surrounding agent, so this is really a future-compatibility change.
  • Adds text recommending a two-year waiting period before a newly-renamed ID becomes primary. These are rare (the last was 2022's Europe/Kiev=>Europe/Kyiv) and when the next rename happens, we'll try to convince CLDR to implement this requirement for us. There are no pending renames, so this is also a future-compatibility change.

Per-engine changes required

Implementing the changes in this PR will impact JS engines differently, given the current divergence between engines:

  • For V8 (cc @FrankYFTang) and JSC (cc @Constellation), there should be no change because they rely on ICU and CLDR which already behave like 1-3 above. Note that this PR doesn't affect the plan to fix out-of-date canonicalizations like Asia/Calcutta and Europe/Kiev. This plan is unchanged: as part of landing Temporal Stage 4, switch to use ICU's new icu::TimeZone::getIanaID(), which returns the latest IANA IDs instead of out-of-date canonical IDs like Asia/Calcutta.

  • For SpiderMonkey (cc @anba), more changes are needed because currently SpiderMonkey conforms to the spec which requires using backward in TZDB to determine canonicalization. SM could use icu::TimeZone::getIanaID() to implement (2) and (3) above, or could implement the same behavior by reading CLDR data or IANA data directly. Also, this PR will reduce SM's differences in Intl.supportedValuesOf('timeZone') vs. V8/JSC.

Testing

Test262 changes will be needed to validate these normative changes, but I'm not sure how we can run those tests except using the Temporal polyfill. @ptomato I'll be looking for your advice (and perhaps help writing tests!) on this point.

Feedback requested

Feedback is welcome on any part of this proposal, but I'm most interested in making sure that the spec text actually accomplishes what the summary above claims that it does.

@sffc
Copy link
Contributor

sffc commented Apr 2, 2024

Thanks for putting this together @justingrant! We can discuss this at the next TG2 meeting. In the mean time, I encourage the listed reviewers to take a look.

@ptomato
Copy link
Contributor

ptomato commented Apr 3, 2024

Test262 changes will be needed to validate these normative changes, but I'm not sure how we can run those tests except using the Temporal polyfill.

It's fine to submit tests to test262 that no engine can pass yet, as long as they are correct according to the current snapshot of ECMA-262 or 402 or a Stage 3 proposal.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I don't feel qualified to review the details — I mostly haven't followed those discussions. At a general level, this all looks very reasonable.

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@justingrant justingrant force-pushed the time-zone-id-requirements branch 4 times, most recently from c78de1d to 3bb6dee Compare April 3, 2024 01:48
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
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.

Mostly editorial comments, but I am not comfortable with making the rename waiting period mandatory.

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@FrankYFTang
Copy link
Contributor

4. e.g. PST7PDT=>America/Los_Angeles

I assuem you mean to say "e.g. PST8PDT=>America/Los_Angeles" ?
not "PST7PDT=>America/Los_Angeles" right?

@sffc sffc moved this from Priority Issues to Previously Discussed in ECMA-402 Meeting Topics Apr 25, 2024
@justingrant
Copy link
Contributor Author

I just pushed a new commit that includes what I think resolves all review feedback. @gibson042 @sffc (and anyone else who's interested) do you want to re-review?

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.

10 editorial suggestions and one request for better specification.

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@justingrant justingrant force-pushed the time-zone-id-requirements branch 7 times, most recently from 0810433 to b458ba3 Compare May 23, 2024 06:16
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.

The algorithm looks good, but I'd like to use more precise phrasing than «the first, space-delimited column in backzoneLinkLine after…».

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
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.

Looking good! Some final tweaks to StringSplitToList and its use, then I think this is ready.

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@justingrant justingrant force-pushed the time-zone-id-requirements branch 3 times, most recently from dc691aa to b177cec Compare May 27, 2024 01:48
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.

If you want to update StringSplitToList to support empty-string S (with result in alignment with String.prototype.split, e.g. « "" »), I would not be opposed. But dealing with it at call sites for now seems fine to me.

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@justingrant justingrant force-pushed the time-zone-id-requirements branch 3 times, most recently from 8e1198a to adc0cf3 Compare May 29, 2024 04:27
@justingrant
Copy link
Contributor Author

If you want to update StringSplitToList to support empty-string S (with result in alignment with String.prototype.split, e.g. « "" »), I would not be opposed. But dealing with it at call sites for now seems fine to me.

Nah, I think it's fine as-is, and I actually think it's kinda good bad to force callsites to decide how they want to handle the empty-string case because it's not always intuitive how to handle that case.

Anyway, I adopted both of your last suggestions, rebased, and squashed. I think this PR should be ready to merge.

@justingrant
Copy link
Contributor Author

justingrant commented May 29, 2024

I think this PR should be ready to merge.

Actually, I assume we want Test262 tests for this new text. I'll start working on those.

And of course it can't be merged until after it's approved at the next TC39 meeting in a few weeks.

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.

On second thought, let's save the IsWellFormedUnitIdentifier changes for a followup. This PR ought to be narrowly focused on time zones, and IsWellFormedUnitIdentifier is not the only preexisting operation that will benefit from StringSplitToList.

@justingrant justingrant force-pushed the time-zone-id-requirements branch 3 times, most recently from 6273e7f to 9be09da Compare May 31, 2024 19:51
This PR resolves tc39#825 by adding spec text that more clearly defines
how ECMA-402 implementations should decide which IANA time zone IDs
should be primary vs. non-primary.

This PR implements "Option C" in tc39#825 by deterministically defining
ECMAScript's exceptions from the IANA Time Zone Database's defaults,
and then pointing implementers at ICU as a convenient implementation
of those exceptions.

This PR also accommodates to web reality by aligning the 402 spec text
with the existing behavior of ICU and CLDR while providing deterministic
rules that can guide future changes in CLDR data.

Finally, this PR introduces a new StringSplitToList abstract operation
and uses it to simplify AvailableNamedTimeZoneIdentifiers and
IsWellFormedUnitIdentifier.
@justingrant
Copy link
Contributor Author

On second thought, let's save the IsWellFormedUnitIdentifier changes for a followup.

OK, I reverted that part. I think this PR should be ready to present at Helsinki, unless there's other review feedback. Thanks @gibson042 for your help whipping this into shape!

justingrant added a commit to justingrant/agendas that referenced this pull request Jun 2, 2024
Add agenda item for:

Normative: specify time zone ID requirements to reduce divergence between engines
tc39/ecma402#877
michaelficarra pushed a commit to tc39/agendas that referenced this pull request Jun 3, 2024
Add agenda item for:

Normative: specify time zone ID requirements to reduce divergence between engines
tc39/ecma402#877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
ECMA-402 Meeting Topics
Previously Discussed
Development

Successfully merging this pull request may close these issues.

Should ECMA-402 spec text for time zone canonicalization refer to CLDR or to IANA as authoritative?
8 participants