Normative: Updates on fractionalSecondDigits in preparation for Temporal

[romulocintra]

Closes #590

Update 402 to :

• Accept 0 to mean the same as undefined
• Accept 4-9 in preparation for Temporal; in the mean time, digits in those positions are always will behave like 0

[romulocintra]

cc @FrankYFTang

[ptomato]

Hi, I'm not an ECMA-402 reviewer but I had some questions from reviewing the test262 tests for this change.

[ptomato]

There seems to be a pre-existing inconsistency in ECMA-402 about whether [[FractionalSecondDigits]] contains Number or mathematical values. Properties of Intl.DateTimeFormat instances says either undefined or a "positive non-zero integer Number value" but step 36.d of InitializeDateTimeFormat stores a mathematical value (returned from GetNumberOption in step 36.b.i) in the slot.

In any case I'm not sure if it's necessary to amend this table, since a value > 3 passed for this option is converted to undefined. The other place where it could be used is for the info in the [[LocaleData]].[[<locale>]].[[formats]].[[<calendar>]] internal slots, in which it might be better to say "a positive non-zero integer" as well.

I bring this up from a test262 perspective, because I'm not sure whether the values in this cell of the table are observable in that they need to be tested in test262.

I meant for it to be a comment

[sffc]

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-10-06.md#normative-update-spec-to-align-with-mdn--preparation-for-temporal

CC @FrankYFTang who voiced concerns.

[romulocintra]

As an action point from past TG2 Meeting #715 :

@sffc @FrankYFTang @ryzokuken any thoughts about the next steps?

[ryzokuken]

@ptomato's comment doesn't block this PR from landing, all green from my side!

[ryzokuken]

Perhaps we could file that as a separate issue on this repo and discuss it in isolation?

[ptomato]

Perhaps we could file that as a separate issue on this repo and discuss it in isolation?

→ #725

[sffc]

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-11-03.md#normative-update-spec-to-align-with-mdn--preparation-for-temporal-715

[ryzokuken]

@romulocintra any update?

[romulocintra]

Hi @ryzokuken,

I split this into two different parts:

• Normative: support fractionalSecondDigits to be 0 #733 addresses -> Accept 0 to mean the same as undefined

and this current PR :

• Accept 4-9 in preparation for Temporal; in the mean time, digits in those positions are always will behave like 0

[romulocintra]

^ @sffc

[FrankYFTang]

I am very concerned about this PR and oppose this getting merged into ECMA402 as a ECMA402 PR. Instead, I propose these changes be merged into the Temporal proposal.

One thing I would like to point out clearly, according to TG2 meeting notes, we never discussed the details during the meeting. In both meeting, I was always direct to “ to discuss offline “

TG2 Oct meeting
https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-10-06.md#normative-update-spec-to-align-with-mdn--preparation-for-temporal

TG2 Nov meeting
https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-11-03.md#normative-update-spec-to-align-with-mdn--preparation-for-temporal-715

So here I am, discuss it offline with you.

We need to discuss this PR with two different issues:
Dealing with 0
Dealing with 4-9

First of all, in the current shape of this PR, we change 3 different places of in ECMA402
In InitializeDateTimeFormat This is needed for #1 and #2 above
Section “Abstract Operations for DateTimeFormat Objects” This is only needed for #2 above. (no impact to #1).
In BasicFormatMatcher This is only needed for #2 above. (no impact to #1).

For dealing with 0, the current shape of ECMA402 disallows 0. And from what I read in #590 there are two only reasons mentioned:
MDN states it accept 0 (which is a MDN bug which should be fixed in MDN)
“Temporal allows zero for this option, so regardless of whether zero is allowed or not, we should probably align across Temporal and 402. “ . This is a valid one, but the only changes needed for Temporal about this is in InitializeDateTimeFormat. But Temporal already have a section to modify InitializeDateTimeFormat in https://tc39.es/proposal-temporal/#sec-temporal-initializedatetimeformat I see no reason such change cannot be just amended to Temporal spec only, if that is the ONLY reason we want to accept 0. And I see no value to add 0 now into ECMA402 prior to that.

Now, let’s talk about 4-9. The change by itself, from my point of view, is not only unnecessary, but very confusing in this stage and should not be landed into ECMA402:

let x = 3;
let y = (new Intl.DateTimeFormat("en", {fractionalSecondDigits: x })).resolvedOptions().fractionalSecondDigits;
x == y 

Prior to this PR, we got true here.

With this PR, it still get true. But now consider the following

x = 4
let y = (new Intl.DateTimeFormat("en", {fractionalSecondDigits: x })).resolvedOptions().fractionalSecondDigits;

without this PR, the new will simply throw RangeError because 4 is not an acceptable value

but with this PR, the new will success and y will be set to undefined.
and x == y will become false

But.... what will be the df format?
It will format without any fracitional second digits

and this is very confusing because the 4 didn't through, neither get honor in the format, it was just be treated as undefined.

[FrankYFTang]

I strongly object this PR

[FrankYFTang]

now, you will say, Temporal need to support fractionalSecondDigits 0, and 4-9 so we need to change, it. ok where are that stated in Temporal spec now?

Let's see, the word "fractionalSecondDigits" currently appeared 26 times inside
https://tc39.es/proposal-temporal/

It appear twice in ToSecondsStringPrecision and that function impact the following functions:

• Temporal.PlainTime.prototype.toString
• Temporal.PlainDateTime.prototype.toString
• Temporal.ZonedDateTime.prototype.toString
• Temporal.Duration.prototype.toString
• Temporal.Instant.prototype.toString
  then the other 24 times which mentioned the word "fractionalSecondDigits" are all in Chapter 15 which amending ECMA402

Now, according to the Temporal spec which passed Stage 3 (and the current shape of the spec text), the fractionalSecondDigits is only needed to support 1-3 for toLocaleString and Intl.DateTimeFormat (see step 36-b-i in https://tc39.es/proposal-temporal/#sec-temporal-initializedatetimeformat ) . That is what have already reach consense. There were no consense reached to support toLocaleString or Intl.DateTimeFormat beyond 3 to reach nanosecond precision.

If we follow the Temporal spec text which reach Stage 3 (and what is on the spec today) we would throw RangeError with the following code:

let t = Temporal.Now.plainDateTimeISO();
t.toLocaleString("en", {fractionalSecondDigits:4});
// throw RangeError
t.toLocaleString("en", {fractionalSecondDigits:9});
// throw RangeError

which is the spec we all agreed in TC39 to support so far when it get Stage 3 advacement. I never oppose the Temporal reach Stage 3 because it only require 3 fractionalSecondDigits but not beyond it. If the spec text require more than 3 I would block it when it asked to advance to Stage 3. But since the champion didn't I didn't block.

But... why I would block? The reason is simple. We will have implementation difficult to support sub millisecond precision for these toLocaleString or Intl.DateTimeFormat.prototype.format . Currently, we are depending on ICU to do the format, but ICU cannot handle nanosecond precision yet. (at least not in 72.1)

[FrankYFTang]

Let me illusrate the difficulty of supporting nanosecond precision in Temporal.*.prototype.toLocaleString and Intl.DateTimeFormat.prototype.format(|ToParts)

Currently, all known browsers implement it by (either C API or C++ API) using ICU.

Here is the ICU documentation:

https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/classicu_1_1SimpleDateFormat.html#a3118634b6b3042729ddfe2f2dbf7ea10#:~:text=Fractional%20Second

notice the text
"
Fractional Second - truncates (like other time fields) to the count of letters when formatting. Appends zeros if more than 3 letters specified.
'

Now, with the followng patch for v8, I can implement a BUGGY attempt to support Temporal to nanosecond precision, but this is only BUGGY attempt which prove the point it cannot be done with the current ICU API:

diff --git a/src/objects/js-date-time-format.cc b/src/objects/js-date-time-format.cc
index 6e6298e274..73269f6927 100644
--- a/src/objects/js-date-time-format.cc
+++ b/src/objects/js-date-time-format.cc
@@ -485,7 +485,7 @@ Handle<String> DateTimeStyleAsString(Isolate* isolate,
 
 int FractionalSecondDigitsFromPattern(const std::string& pattern) {
   int result = 0;
-  for (size_t i = 0; i < pattern.length() && result < 3; i++) {
+  for (size_t i = 0; i < pattern.length() && result < 9; i++) {
     if (pattern[i] == 'S') {
       result++;
     }
@@ -828,7 +828,13 @@ DateTimeValueRecord TemporalInstantToRecord(Isolate* isolate,
                      BigInt::FromInt64(isolate, 1000000))
           .ToHandleChecked()
           ->AsInt64();
-  return {milliseconds, kind};
+  double fraction =
+      BigInt::Remainder(isolate, Handle<BigInt>(instant->nanoseconds(), isolate),
+                     BigInt::FromInt64(isolate, 1000000))
+          .ToHandleChecked()
+          ->AsInt64();
+  printf("fraction %f\n", fraction);
+  return {milliseconds + fraction * 1e-6, kind};
 }
 
 Maybe<DateTimeValueRecord> TemporalPlainDateTimeToRecord(
@@ -1377,6 +1383,7 @@ icu::UnicodeString CallICUFormat(const icu::SimpleDateFormat& date_format,
   // For other Temporal objects, lazy generate a SimpleDateFormat for the kind.
   std::unique_ptr<icu::SimpleDateFormat> pattern(
       GetSimpleDateTimeForTemporal(date_format, kind));
+  printf("%.9f\n", time_in_milliseconds);
   pattern->format(time_in_milliseconds, result, fp_iter, status);
   return result;
 }
@@ -2430,7 +2437,7 @@ MaybeHandle<JSDateTimeFormat> JSDateTimeFormat::New(
       MAYBE_ASSIGN_RETURN_ON_EXCEPTION_VALUE(
           isolate, fsd,
           GetNumberOption(isolate, options,
-                          factory->fractionalSecondDigits_string(), 1, 3, 0),
+                          factory->fractionalSecondDigits_string(), 0, 9, 0),
           Handle<JSDateTimeFormat>());
       // Convert fractionalSecondDigits to skeleton.
       for (int i = 0; i < fsd; i++) {

The shortcoming is in two issues not just one

1. The ICU SimpleDateFormat::format only support to milisecond precision (which mean 1, 2 or 3 in fractionalSecondDigits) so there are 1,000,000 different value of nanoseconds of Instant will all formated to the same string, which they should not.
2. The ICU SimpleDateFormat::format take the Date (which is double) as the date type to represent millisecond, and the data type does not have the precision to encode up to nanosecond precision, even if we change ICU 73 to relax the restriction on 1 above.

Let me illustrate the issue with the following call (this is run on my local machine after I apply the patch above:

ftang@ftang4:~/v8/v8$ out/x64.release/d8 --harmony-temporal
V8 version 11.0.0 (candidate)
d8> let t = Temporal.Now.plainDateTimeISO();
undefined
d8> t
2022-11-28T14:18:35.661261056
d8> t.toLocaleString("en", {fractionalSecondDigits:9});
fraction 261056.000000
1669673915661.260986328
"11/28/2022, 2:18:35.661000000 PM"

Now, the fact it show
"11/28/2022, 2:18:35.661000000 PM"
instead of
"11/28/2022, 2:18:35.661261056 PM"

show the ICU API limitation mentioned in the API doc
but more interestingly is the debugging line I printted out

fraction 261056.000000
1669673915661.260986328

This show, my code did get the bigint as
1669673915661261056 nanosecond
but while I convert it to double, by taking
1669673915661 millisecond
and 261056 nanoseconds both as int and turn into a double, the result is
1669673915661.260986328
because double use only 53 bits to encode the digits, and
log(2^53)/log(10)= 15.9545897702
which mean we double can only encode the first 15 digits precisionly. and in this case, "1669673915661" is 13 digits, so two extra digits "26" is precious enough but it won't have the encodeing power to preiously encode the "1056", therefore what the ICU see from the double is the remaining 4 digits are "0986" instead.

Therefore, even if we fix the ICU SimpleDateFormat:format to format the rest of 6 digits from the double instead of "Appends zeros if more than 3 letters specified" , it will generate incorrect result, likely for the value fractionalSecondDigits> 5
Assuming the value is not represent not too far in the future or past. For value way in the future or in the past, the incorrect result will shown for a even smaller value in fractionalSecondDigits.

[FrankYFTang]

In summary, if we want to change ICU to support the formatting of nanosecond precision, not only it need to change the internal implementation of SimpleDateFomrat::format, it also need to add new API to take a new data type to encode the value which has nanosecond precision, which currently does not exist in ICU.

Accepting an behavior of 1 to 6 '0' as the format result in place with the real didigt stored in the Temporal object is a very very bad one because for anyone whe care about setting the fractionalSecondDigits > 3, we have to assume the information below millisecond is important enough for them to use such setting, and truncating it to millisecond then adding 0 will misguide the reader as a different value than what it actually is and could cause diaster for any application which care about sub millisecond precision. The users would either

1. they DO NOT care about sub millisecond precision so the API do not need to support it, OR
2. thet DO care about sub millisecond precision so the API have to format it it precisously

and supporting an API which could set it to higher precision but allow the formatted output in lower precision (incorrect value in that precision, actually) is very dangerous. and I would argue no body in the world will need such hehavior.

[FrankYFTang]

The requirement for ICU to support date time format for nanosecond precsion is filed under https://unicode-org.atlassian.net/browse/ICU-22218

[FrankYFTang]

The current Temporal proposal spec text only require 1 - 3
https://tc39.es/proposal-temporal/#sec-temporal-initializedatetimeformat

step 36-b-3

i. Let value be ? GetNumberOption(options, "fractionalSecondDigits", 1, 3, undefined).

[sffc]

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-12-08.md#normative-updates-on-fractionalseconddigits-in-preparation-for-temporal-715

Conclusion: Put this issue on the backlog, but pursue the ICU issue in parallel. Revisit after Temporal is merged.