Normative: Address time zone and calendar case sensitivity #2395

ptomato · 2022-08-30T23:43:27Z

Makes all of the following work:

new Temporal.TimeZone('eTc/gMt+1')
Temporal.TimeZone.from('eTc/gMt+1')
Temporal.TimeZone.from('2022-08-30T14:29[eTc/gMt+1]')
new Temporal.Calendar('jApAnEsE')
Temporal.Calendar.from('jApAnEsE')
Temporal.Calendar.from('2022-08-30[u-ca=jApAnEsE]')

Closes: #2326

ptomato · 2022-08-30T23:44:41Z

cc @anba

codecov · 2022-08-30T23:47:03Z

Codecov Report

Merging #2395 (4b3288a) into main (056d45b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2395   +/-   ##
=======================================
  Coverage   94.75%   94.76%           
=======================================
  Files          20       20           
  Lines       10991    11003   +12     
  Branches     1962     1963    +1     
=======================================
+ Hits        10415    10427   +12     
  Misses        531      531           
  Partials       45       45

Impacted Files	Coverage Δ
polyfill/lib/calendar.mjs	`81.43% <100.00%> (ø)`
polyfill/lib/ecmascript.mjs	`98.65% <100.00%> (+<0.01%)`	⬆️

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

gibson042 · 2022-09-01T18:17:55Z

spec/abstractops.html

@@ -1041,6 +1041,8 @@ <h1>ISO 8601 grammar</h1>
          TimeZoneIANANameComponent `/` TimeZoneIANANameTail

      TimeZoneIANALegacyName :
+          &gt; An ASCII-case-insensitive match for one of the following:


I don't think this is valid notation, cf. https://tc39.es/ecma262/multipage/notational-conventions.html#sec-notational-conventions . There are probably many alternative approaches, but the one that comes to mind for me is providing a broad general definition of |TimeZoneIANALegacyName| (e.g., TimeZoneIANALegacyName ::: TimeZoneIANANameTail[+Legacy] where the Legacy parameter is passed down ultimately to |TZChar| for [+Legacy] DecimalDigit and [+Legacy] `+` expansions), using early errors or a [> but only …] prose assertion to restrict it to case-insensitive matches of this finite list, and adding an ambiguity-avoiding "but not TimeZoneIANANameTail" restriction in the reference to it from |TimeZoneIANAName|.

Ugh, I was hoping that the prose description given here would be a sufficiently unambiguous description of what this production can match. This suggestion makes the grammar less easy to understand in my opinion... but I've tacked it on in an extra commit, see what you think of it. Having seen both, I'd personally prefer to leave it as it was.

I used a prose assertion instead of an early error here to restrict it to the specific legacy names; I find the notation less readable, but on the other hand more readable that it's right there in |TimeZoneLegacyIANAName| rather than at the end where an early error would be. It doesn't seem to me that the "but not TimeZoneIANANameTail" is necessary, because if |TimeZoneLegacyIANAName| is restricted to a finite list, it can't be ambiguous with TimeZoneIANANameTail[~Legacy].

@gibson042 Let me know what you think of this solution. I'd rather keep it the way it was, but if you think this is an improvement then I'm fine with that too.

According to ECMA-262, "a few nonterminal symbols are described by a descriptive phrase in sans-serif type in cases where it would be impractical to list all the alternatives" — I think this sufficiently describes the original prose description I had; I find the current solution in this PR impractical. I'll propose that we go ahead and merge this with the original prose description unless I hear any objections soon, since Frank is waiting on it.

gibson042 · 2022-09-01T18:19:59Z

spec/abstractops.html

@@ -1041,6 +1041,8 @@ <h1>ISO 8601 grammar</h1>
          TimeZoneIANANameComponent `/` TimeZoneIANANameTail

      TimeZoneIANALegacyName :
+          &gt; An ASCII-case-insensitive match for one of the following:
+          `Etc/GMT` ASCIISign UnpaddedHour


If the same nonterminal is going to include legacy names and Etc/GMT±<h> names, I think it should be named "Special" rather than "Legacy".

Re-reading Timezone identifiers, Etc/GMT± is actually also a legacy name, so I think this change makes it actually more correct.

ptomato · 2022-09-13T18:32:29Z

This achieved consensus at the September 2022 TC39 meeting. It's covered by test262 tests.

While #2395 is still pending the resolution of the case-insensitive grammar question, its tests are expected to fail.

FrankYFTang · 2022-09-19T21:02:32Z

What is blocking this to be merged
My v8 fix reviews
https://chromium-review.googlesource.com/c/v8/v8/+/3903222
https://chromium-review.googlesource.com/c/v8/v8/+/3901200
are blocked by this merge now. It will be nice if you can merge this PR ASAP if there are no other issues.

spec/abstractops.html

Previously, there was a discrepancy in the case-sensitivity of IANA legacy time zone names that were accepted by new Temporal.TimeZone() vs. Temporal.TimeZone.from(). This clarifies definitively that the following are all valid: new Temporal.TimeZone('eTc/gMt+1') Temporal.TimeZone.from('eTc/gMt+1') Temporal.TimeZone.from('2022-08-30T14:18[eTc/gMt+1]')

For consistency with ECMA-402, where calendar names are case-insensitive, Temporal should also accept calendar names case-insensitively. This clarifies definitively that the following are all valid: new Temporal.Calendar('jApAnEsE') Temporal.Calendar.from('jApAnEsE') Temporal.Calendar.from('2022-08-30[u-ca=jApAnEsE]') This is in line with the already-existing new Date().toLocaleString('en', { calendar: 'jApAnEsE' }) new Date().toLocaleString('en-u-ca-jApAnEsE')

spec/abstractops.html

spec/calendar.html

In case the prose description is not sufficiently correct notation, here's an alternative notation suggested by Richard, where time zone characters get a "Legacy" parameter which includes the otherwise illegal + and digit characters, and |TimeZoneIANALegacyName| gets an explicit prose assertion that ensures it can only match case-insensitive matches for the specific legacy names.

With this change, the corresponding test262 tests will no longer fail.

This is implied by the canonical form of a calendar type being lowercase, but not explicit until now.

This moves the definitions of ASCII-uppercase, ASCII-lowercase, and ASCII- case-insensitive match into ECMA-262 and deletes them from ECMA-402. It also generalizes the definitions of ASCII-uppercase and ASCII-lowercase to cover sequences of code points as well as strings. This allows a simplification in the TimeZoneIANALegacyName production. (Note, the unnecessary parentheses are because grammarkdown seems to choke on the version without them)

ptomato · 2022-10-18T23:49:52Z

I would still rather we'd go with the much simpler prose description of IANATimeZoneLegacyName, but I'm fine to let that be an editorial matter for Stage 4, especially since Frank is waiting for this PR to be merged.

ptomato added this to the Next batch of normative changes milestone Aug 30, 2022

ptomato requested review from Ms2ger, gibson042 and Aditi-1400 August 30, 2022 23:43

ptomato changed the title ~~Address time zone and calendar case sensitivity~~ Normative: Address time zone and calendar case sensitivity Aug 30, 2022

Ms2ger approved these changes Aug 31, 2022

View reviewed changes

ptomato marked this pull request as draft August 31, 2022 14:59

This was referenced Aug 31, 2022

Case-normalisation and/or canonicalisation for Calendar identifiers? #2314

Closed

Normative: Allow annotations after YYYY-MM and MM-DD #2398

Closed

gibson042 requested changes Sep 1, 2022

View reviewed changes

ptomato requested a review from gibson042 September 6, 2022 01:26

Aditi-1400 mentioned this pull request Sep 7, 2022

Temporal: Add Temporal calendar and timezone case insensitive tests tc39/test262#3655

Merged

ptomato marked this pull request as ready for review September 13, 2022 18:22

ptomato added a commit that referenced this pull request Sep 14, 2022

Update expected failures

0017ed9

While #2395 is still pending the resolution of the case-insensitive grammar question, its tests are expected to fail.

ptomato added a commit that referenced this pull request Sep 14, 2022

Update expected failures

ba24b84

While #2395 is still pending the resolution of the case-insensitive grammar question, its tests are expected to fail.

ptomato added a commit that referenced this pull request Sep 14, 2022

Update expected failures

008a7dc

While #2395 is still pending the resolution of the case-insensitive grammar question, its tests are expected to fail.

ptomato force-pushed the 2326-time-zone-case-sensitivity branch from a91e528 to 1f53d41 Compare September 14, 2022 16:58

gibson042 reviewed Sep 20, 2022

View reviewed changes

spec/abstractops.html Outdated Show resolved Hide resolved

spec/abstractops.html Outdated Show resolved Hide resolved

ptomato force-pushed the 2326-time-zone-case-sensitivity branch from 1f53d41 to 8156502 Compare September 26, 2022 18:35

ptomato requested a review from gibson042 September 26, 2022 18:35

ptomato added 2 commits October 18, 2022 12:43

gibson042 requested changes Oct 18, 2022

View reviewed changes

spec/abstractops.html Outdated Show resolved Hide resolved

spec/calendar.html Show resolved Hide resolved

ptomato added 2 commits October 18, 2022 12:45

Update expected failures

b003411

With this change, the corresponding test262 tests will no longer fail.

ptomato force-pushed the 2326-time-zone-case-sensitivity branch from 8156502 to b003411 Compare October 18, 2022 19:46

ptomato added 2 commits October 18, 2022 16:42

Editorial: Add lowercase restriction to AvailableCalendars

f347a03

This is implied by the canonical form of a calendar type being lowercase, but not explicit until now.

ptomato requested a review from gibson042 October 18, 2022 23:46

gibson042 approved these changes Oct 20, 2022

View reviewed changes

ptomato merged commit 0af69d3 into main Oct 20, 2022

ptomato deleted the 2326-time-zone-case-sensitivity branch October 20, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Address time zone and calendar case sensitivity #2395

Normative: Address time zone and calendar case sensitivity #2395

ptomato commented Aug 30, 2022

ptomato commented Aug 30, 2022

codecov bot commented Aug 30, 2022 •

edited

Loading

gibson042 Sep 1, 2022

ptomato Sep 6, 2022

ptomato Sep 13, 2022

ptomato Sep 19, 2022 •

edited

Loading

gibson042 Sep 1, 2022

anba Sep 1, 2022

ptomato commented Sep 13, 2022

FrankYFTang commented Sep 19, 2022

ptomato commented Oct 18, 2022

Normative: Address time zone and calendar case sensitivity #2395

Normative: Address time zone and calendar case sensitivity #2395

Conversation

ptomato commented Aug 30, 2022

ptomato commented Aug 30, 2022

codecov bot commented Aug 30, 2022 • edited Loading

Codecov Report

gibson042 Sep 1, 2022

Choose a reason for hiding this comment

ptomato Sep 6, 2022

Choose a reason for hiding this comment

ptomato Sep 13, 2022

Choose a reason for hiding this comment

ptomato Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

gibson042 Sep 1, 2022

Choose a reason for hiding this comment

anba Sep 1, 2022

Choose a reason for hiding this comment

ptomato commented Sep 13, 2022

FrankYFTang commented Sep 19, 2022

ptomato commented Oct 18, 2022

codecov bot commented Aug 30, 2022 •

edited

Loading

ptomato Sep 19, 2022 •

edited

Loading