Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions Source/JavaScriptCore/runtime/IntlObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1951,10 +1951,23 @@ static bool isValidTimeZoneNameFromICUTimeZone(StringView timeZoneName)
return false;
if (timeZoneName.startsWith("Etc/"_s))
return true;
// IANA time zone names include '/'. Some of them are not including, but it is in backward links.
// And ICU already resolved these backward links.
if (!timeZoneName.contains('/'))
return timeZoneName == "UTC"_s || timeZoneName == "GMT"_s;
// IANA time zone names usually include '/'. The following legacy identifiers
// are first-class `Zone`s in the IANA `etcetera` file (not backward links),
// so they are primary IANA zones and must be treated as valid:
// CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET
// plus UTC/GMT which are always valid.
Comment on lines +1954 to +1958

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The comment states these identifiers "are first-class Zones in the IANA etcetera file (not backward links)", but as of tzdata 2024b (Sep 2024) all eight were moved out of etcetera and are now Link entries in the backward file. The code change is still correct — test262/ECMA-402 require these to be accepted regardless — but consider rewording the rationale to e.g. "legacy POSIX-style IANA identifiers required by ECMA-402/test262" so the comment doesn't go stale against current IANA data.

Extended reasoning...

What the issue is

The newly added comment at IntlObject.cpp:1954-1958 (and the PR description) asserts that CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET "are first-class Zones in the IANA etcetera file (not backward links)". That was historically accurate, but it is no longer true at the time this PR was written.

Why it's stale

In tzdata 2024b (released 2024-09-04), the IANA tz maintainers moved all of these names — along with EST, HST, MST — out of etcetera and into the backward file as Link entries pointing at geographical zones. The 2024b NEWS entry reads:

Names present only for compatibility with UNIX System V (last released in the 1990s) have been moved to 'backward'. … The affected names are CET, CST6CDT, EET, EST, EST5EDT, HST, MET, MST, MST7MDT, PST8PDT, and WET.

Checking upstream confirms it: etcetera no longer contains any of the eight names, and backward contains all of them — e.g. Link Europe/Brussels CET, Link America/Chicago CST6CDT, Link America/Los_Angeles PST8PDT. So the comment is wrong on both counts: they are not in etcetera, and they are backward links.

Step-by-step proof

  1. The PR description links to https://github.com/eggert/tz/blob/main/etcetera as the source for the "first-class Zone" claim.
  2. Fetch that file today: it defines only Etc/GMT, Etc/UTC, Etc/GMT±N, GMT, and a handful of Etc/ links. None of CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET appear.
  3. Fetch backward from the same repo: each of the eight appears as a Link line targeting a geographical zone.
  4. Therefore the literal statement in the new comment — "first-class Zones in the IANA etcetera file (not backward links)" — is factually incorrect against current upstream tzdata.

Why this doesn't affect correctness of the code

The functional change — whitelisting these eight names in isValidTimeZoneNameFromICUTimeZone — is still right. ECMA-402 and test262 (intl402/DateTimeFormat/timezone-case-insensitive.js) require these identifiers to be accepted and to round-trip through resolvedOptions().timeZone regardless of whether IANA currently classifies them as Zones or Links, and ICU's canonical enumeration still surfaces them. Only the rationale in the comment is misleading; a future maintainer cross-referencing IANA could be confused about why the whitelist exists.

Suggested fix

Reword the comment to avoid asserting current IANA Zone-vs-Link status, e.g.:

// IANA time zone names usually include '/'. The following are legacy
// POSIX/System V-style identifiers (CET, CST6CDT, EET, EST5EDT, MET,
// MST7MDT, PST8PDT, WET) that ECMA-402 and test262 require us to accept
// as valid named time zones, plus UTC/GMT which are always valid.

This keeps the rationale durable even as IANA reshuffles Zone/Link classifications.

if (!timeZoneName.contains('/')) {
return timeZoneName == "UTC"_s
|| timeZoneName == "GMT"_s
|| timeZoneName == "CET"_s
|| timeZoneName == "CST6CDT"_s
|| timeZoneName == "EET"_s
|| timeZoneName == "EST5EDT"_s
|| timeZoneName == "MET"_s
|| timeZoneName == "MST7MDT"_s
|| timeZoneName == "PST8PDT"_s
|| timeZoneName == "WET"_s;
}
return true;
}

Expand Down
Loading