tc39 / ecma262

Status, process, and documents for ECMA-262
https://tc39.es/ecma262/
Other
14.85k stars 1.27k forks source link

Normative: Revert U+2212 (Unicode minus sign) as a valid character in timezone offsets #3334

Open justingrant opened 1 month ago

justingrant commented 1 month ago

Following ISO-8601, #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 https://github.com/tc39/proposal-temporal/issues/2843#issuecomment-2119724671

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 I'll follow up with a normative Temporal PR to remove this character from Temporal as well.

FYI @ptomato @gibson042.

ptomato commented 1 month ago

FWIW as far as I understand it, this change would not be observable if made in ECMA-262 today, without Temporal. (But it should probably be discussed in plenary anyway.)

justingrant commented 1 month ago

Clarifying @ptomato's comment above: this U+2212 character is currently only exposed via Temporal, because the only way an offset can get into Date is via SystemTimeZoneIdentifier which AFAIK is required to be ASCII in all known implementations.

So the current plan is to present a normative Temporal PR in Helsinki to remove U+2212 from the Temporal grammar, and mention as part of this discussion that consensus on that PR also means percolating this grammar change back to 262.

Sound OK?

anba commented 1 month ago

It's normative for ECMA-402, where Intl.DateTimeFormat has been changed to accept offset time zone strings.

gibson042 commented 3 weeks ago

It's normative for ECMA-402, where Intl.DateTimeFormat has been changed to accept offset time zone strings.

concrete example:

const dtf = new Intl.DateTimeFormat("pt", {
  dateStyle: "short",
  timeStyle: "full",
  timeZone: "\u{2212}01:00",
});
dtf.format(new Date("2024-01-01T00:00Z"))
// => "31/12/2023, 23:00:00 GMT-01:00"
ptomato commented 2 weeks ago

This normative change reached consensus at the TC39 meeting of 2024-06-12.

ptomato commented 5 days ago

Tests are in https://github.com/tc39/test262/pull/4120. (The first commit specifically contains tests for this PR. The second commit for the companion Temporal PR.)

Eusinho1985 commented 3 days ago

Clarifying @ptomato's comment above: this U+2212 character is currently only exposed via Temporal, because the only way an offset can get into Date is via SystemTimeZoneIdentifier which AFAIK is required to be ASCII in all known implementations.

So the current plan is to present a normative Temporal PR in Helsinki to remove U+2212 from the Temporal grammar, and mention as part of this discussion that consensus on that PR also means percolating this grammar change back to 262.

Sound OK?

justingrant commented 20 hours ago

I rebased to latest main. Hopefully ready to merge?

justingrant commented 20 hours ago

Hmm, I'm seeing a failure in "check IPR form". What's an IPR form? I looked at @ljharb's PR #2832 but couldn't figure out from that PR what (if anything) I need to do.

ljharb commented 20 hours ago

@justingrant you're a delegate, so you don't need to worry about it; i think there's a bug in the action and I'll look into it asap.

michaelficarra commented 20 hours ago

I rebased to latest main. Hopefully ready to merge?

We typically require 2 editor approvals for anything that's not completely trivial.