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.

Sign up for GitHub

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

Jump to bottom

Normative: Prevent loss of TimeZone information #2479

Closed
wants to merge 2 commits into from
Closed

Normative: Prevent loss of TimeZone information #2479

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
697a7bc
Prevent loss of TimeZone information
Aditi-1400 Jan 17, 2023
756bce1
Add suggestions from review comments
Aditi-1400 Jan 20, 2023
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
53 changes: 34 additions & 19 deletions spec/intl.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,9 @@ <h1>DateTime Format Functions</h1>
</emu-clause>

<emu-clause id="sec-formatdatetimepattern" aoid="FormatDateTimePattern">
<h1>FormatDateTimePattern ( _dateTimeFormat_, <ins>_pattern_</ins>, _patternParts_, <del>_x_</del><ins>_epochNanoseconds_</ins>, _rangeFormatOptions_ )</h1>

<h1>FormatDateTimePattern ( _dateTimeFormat_, <ins>_pattern_</ins>, _patternParts_, <del>_x_</del><ins>_epochNanoseconds_</ins>, _rangeFormatOptions_<ins>, _timeZone_</ins> )</h1>
<p>
The FormatDateTimePattern abstract operation is called with arguments _dateTimeFormat_ (which must be an object initialized as a DateTimeFormat), <ins>_pattern_ (which is a Record of the type contained by the %DateTimeFormat%.[[LocaleData]].[[<_locale_>]].[[formats]].[[<_calendar_>]] List as described in <emu-xref href="#sec-intl.datetimeformat-internal-slots"></emu-xref>),</ins> _patternParts_ (which is a list of Records as returned by PartitionPattern), <del>_x_</del><ins>_epochNanoseconds_</ins> (which must be a <del>Number</del><ins>BigInt</ins> value), and _rangeFormatOptions_ (which is a range pattern Record as used in [[rangePattern]] or *undefined*), interprets _x_ as a time value as specified in ES2023, <emu-xref href="#sec-time-values-and-time-range"></emu-xref>, and creates the corresponding parts according _pattern_ and to the effective locale and the formatting options of _dateTimeFormat_ and _rangeFormatOptions_. The following steps are taken:
The FormatDateTimePattern abstract operation is called with arguments _dateTimeFormat_ (which must be an object initialized as a DateTimeFormat), <ins>_pattern_ (which is a Record of the type contained by the %DateTimeFormat%.[[LocaleData]].[[<_locale_>]].[[formats]].[[<_calendar_>]] List as described in <emu-xref href="#sec-intl.datetimeformat-internal-slots"></emu-xref>),</ins> _patternParts_ (which is a list of Records as returned by PartitionPattern), <del>_x_</del><ins>_epochNanoseconds_</ins> (which must be a <del>Number</del><ins>BigInt</ins> value), _rangeFormatOptions_ (which is a range pattern Record as used in [[rangePattern]] or *undefined*) <ins>and _timeZone_</ins>, interprets _x_ as a time value as specified in ES2023, <emu-xref href="#sec-time-values-and-time-range"></emu-xref>, and creates the corresponding parts according _pattern_ and to the effective locale and the formatting options of _dateTimeFormat_ and _rangeFormatOptions_. The following steps are taken:
</p>

<emu-alg>
Expand All @@ -419,7 +418,9 @@ <h1>FormatDateTimePattern ( _dateTimeFormat_, <ins>_pattern_</ins>, _patternPart
1. Perform ! CreateDataPropertyOrThrow(_nf3Options_, *"minimumIntegerDigits"*, _fractionalSecondDigits_).
1. Perform ! CreateDataPropertyOrThrow(_nf3Options_, *"useGrouping"*, *false*).
1. Let _nf3_ be ? Construct(%NumberFormat%, &laquo; _locale_, _nf3Options_ &raquo;).
1. Let _tm_ be ToLocalTime(<del>_x_</del><ins>_epochNanoseconds_</ins>, _dateTimeFormat_.[[Calendar]], _dateTimeFormat_.[[TimeZone]]).
1. <ins>If _dateTimeFormat_.[[TimeZone]] is not *undefined*, then</ins>
1. <ins>Throw a *RangeError* exception.</ins>
Comment on lines +421 to +422
Copy link
Collaborator

Choose a reason for hiding this comment

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

To implement option 8, I think the logic we want here is:

  • If both dateTimeFormat.[[TimeZone]] and timeZone are not undefined, throw a RangeError exception.
  • If timeZone is undefined, set timeZone to dateTimeFormat.[[TimeZone]].
  • If timeZone is undefined, set timeZone to DefaultTimeZone().

That means we also have to leave dateTimeFormat.[[TimeZone]] undefined if it's not explicitly specified in InitializeDateTimeFormat and not call DefaultTimeZone there. Probably in practice, implementations would still call DefaultTimeZone in InitializeDateTimeFormat, and keep track of whether the time zone was explicitly specified using a separate flag. We could choose to do that in the spec as well. It might look something like this:

  • If timeZone is not undefined and dateTimeFormat.[[TimeZoneSpecified]] is true, throw a RangeError exception.
  • If timeZone is undefined, set timeZone to dateTimeFormat.[[TimeZone]].

1. <ins>Let _tm_ be ToLocalTime(_epochNanoseconds_, _dateTimeFormat_.[[Calendar]], _timeZone_).</ins>
1. Let _result_ be a new empty List.
1. For each Record { [[Type]], [[Value]] } _patternPart_ in _patternParts_, do
1. Let _p_ be _patternPart_.[[Type]].
Expand All @@ -436,7 +437,7 @@ <h1>FormatDateTimePattern ( _dateTimeFormat_, <ins>_pattern_</ins>, _patternPart
1. Append a new Record { [[Type]]: _p_, [[Value]]: _fv_ } as the last element of the list _result_.
1. Else if _p_ is equal to *"timeZoneName"*, then
1. Let _f_ be _dateTimeFormat_.[[TimeZoneName]].
1. Let _v_ be _dateTimeFormat_.[[TimeZone]].
1. Let _v_ be <del>_dateTimeFormat_.[[TimeZone]]</del><ins>_timeZone_</ins>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this incorrect for cases where timeZone is undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes! I'll fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we go with the suggestion from my previous comment, then timeZone can't be undefined here anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Notice the timeZone here could only possible be undefined or the same as dateTimeFormat.[[TimeZone]] . there are no other possible values for timeZone , guarded by the RangeError throw in step 6 of HandleDateTimeTemporalZonedDateTime see https://tc39.es/proposal-temporal/#sec-temporal-handledatetimevaluetemporalzoneddatetime

1. Let _fv_ be a String value representing _v_ in the form given by _f_; the String value depends upon the implementation and the effective locale of _dateTimeFormat_. The String value may also depend on the value of the [[InDST]] field of _tm_ if _f_ is *"short"*, *"long"*, *"shortOffset"*, or *"longOffset"*. If the implementation does not have a localized representation of _f_, then use the String value of _v_ itself.
1. Append a new Record { [[Type]]: _p_, [[Value]]: _fv_ } as the last element of the list _result_.
1. Else if _p_ matches a Property column of the row in <emu-xref href="#table-datetimeformat-components"></emu-xref>, then
Expand Down Expand Up @@ -497,7 +498,7 @@ <h1>PartitionDateTimePattern ( _dateTimeFormat_, _x_ )</h1>
<emu-alg>
1. <ins>Let _x_ be ? HandleDateTimeValue(_dateTimeFormat_, _x_).</ins>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to add [[timeZone]]: *undefined* to the Record returned by HandleDateTimeOthers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops!

1. Let _patternParts_ be PartitionPattern(<del>_dateTimeFormat_.[[Pattern]]</del><ins>_x_.[[pattern]]</ins>).
1. Let _result_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_x_.[[pattern]]</ins>, _patternParts_, <del>_x_</del><ins>_x_.[[epochNanoseconds]]</ins>, *undefined*).
1. Let _result_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_x_.[[pattern]]</ins>, _patternParts_, <del>_x_</del><ins>_x_.[[epochNanoseconds]]</ins>, *undefined*, <ins>x.[[timeZone]]</ins>).
Aditi-1400 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

notice the only possible value for x.[[timeZone]] here is either undefined or the same value of DefaultTimeZone() but nothing else. The value DefaultTimeZone() came from ZDT and the value undefined came from other data type. There is impossible to have any other values at this point Jan 31, 2023 655e3ef .

1. Return _result_.
</emu-alg>

Expand Down Expand Up @@ -562,9 +563,15 @@ <h1>PartitionDateTimeRangePattern ( _dateTimeFormat_, _x_, _y_ )</h1>
1. <ins>If ! SameTemporalType(_x_, _y_) is *false*, throw a *TypeError* exception.</ins>
1. <ins>Let _x_ be ? HandleDateTimeValue(_dateTimeFormat_, _x_).</ins>
1. <ins>Let _y_ be ? HandleDateTimeValue(_dateTimeFormat_, _y_).</ins>
1. <ins>If x.[[timeZone]] is not equal to y.[[timeZone]], then</ins>
1. <ins>Throw a *RangeError* exception.</ins>
1. <ins>Else if x.[[timeZone]] is not *undefined*, then</ins>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need a similar change to the logic, either to include a call to DefaultTimeZone or checking whether the dateTimeFormat.[[TimeZone]] was specified explicitly, whichever way we decide to go with.

1. <ins>Let _timeZone_ be x.[[timeZone]].</ins>
Aditi-1400 marked this conversation as resolved.
Show resolved Hide resolved
1. <ins>Else,</ins>
1. <ins>Let _timeZone_ be _dateTimeFormat_.[[TimeZone]].</ins>
1. If _x_<ins>.[[epochNanoseconds]]</ins> is greater than _y_<ins>.[[epochNanoseconds]]</ins>, throw a *RangeError* exception.
1. Let _tm1_ be ToLocalTime(_x_<ins>.[[epochNanoseconds]]</ins>, _dateTimeFormat_.[[Calendar]], _dateTimeFormat_.[[TimeZone]]).
1. Let _tm2_ be ToLocalTime(_y_<ins>.[[epochNanoseconds]]</ins>, _dateTimeFormat_.[[Calendar]], _dateTimeFormat_.[[TimeZone]]).
1. Let _tm1_ be ToLocalTime(_x_<ins>.[[epochNanoseconds]]</ins>, _dateTimeFormat_.[[Calendar]], <del>_dateTimeFormat_.[[TimeZone]]</del> <ins>_timeZone_</ins>).
1. Let _tm2_ be ToLocalTime(_y_<ins>.[[epochNanoseconds]]</ins>, _dateTimeFormat_.[[Calendar]], <del>_dateTimeFormat_.[[TimeZone]]</del><ins>_timeZone_</ins>).
1. Let _rangePatterns_ be <del>_dateTimeFormat_.[[RangePatterns]]</del><ins>_x_.[[rangePatterns]]</ins>.
1. <ins>Assert: _rangePatterns_ is equal to _y_.[[rangePatterns]].</ins>
1. Let _rangePattern_ be *undefined*.
Expand Down Expand Up @@ -610,7 +617,7 @@ <h1>PartitionDateTimeRangePattern ( _dateTimeFormat_, _x_, _y_ )</h1>
1. If _dateFieldsPracticallyEqual_ is *true*, then
1. Let _pattern_ be <del>_dateTimeFormat_.[[Pattern]]</del><ins>_x_.[[pattern]]</ins>.
1. Let _patternParts_ be PartitionPattern(_pattern_).
1. Let _result_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_pattern_,</ins> _patternParts_, _x_<ins>.[[epochNanoseconds]]</ins>, *undefined*).
1. Let _result_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_pattern_,</ins> _patternParts_, _x_<ins>.[[epochNanoseconds]]</ins>, *undefined*, <ins>_timeZone_</ins>).
1. For each element _r_ in _result_, do
1. Set _r_.[[Source]] to *"shared"*.
1. Return _result_.
Expand All @@ -625,7 +632,7 @@ <h1>PartitionDateTimeRangePattern ( _dateTimeFormat_, _x_, _y_ )</h1>
1. Else,
1. Let _z_ be _y_<ins>.[[epochNanoseconds]]</ins>.
1. Let _patternParts_ be PartitionPattern(_pattern_).
1. Let _partResult_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_pattern_,</ins> _patternParts_, _z_, _rangePattern_).
1. Let _partResult_ be ? FormatDateTimePattern(_dateTimeFormat_, <ins>_pattern_,</ins> _patternParts_, _z_, _rangePattern_, <ins>_timeZone_</ins>).
1. For each element _r_ in _partResult_, do
1. Set _r_.[[Source]] to _source_.
1. Add all elements in _partResult_ to _result_ in order.
Expand Down Expand Up @@ -770,7 +777,8 @@ <h1>HandleDateTimeTemporalDate ( _dateTimeFormat_, _temporalDate_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -796,7 +804,8 @@ <h1>HandleDateTimeTemporalYearMonth ( _dateTimeFormat_, _temporalYearMonth_ )</h
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -823,7 +832,8 @@ <h1>HandleDateTimeTemporalMonthDay ( _dateTimeFormat_, _temporalMonthDay_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -848,7 +858,8 @@ <h1>HandleDateTimeTemporalTime ( _dateTimeFormat_, _temporalTime_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -874,7 +885,8 @@ <h1>HandleDateTimeTemporalDateTime ( _dateTimeFormat_, _dateTime_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -895,7 +907,8 @@ <h1>HandleDateTimeTemporalInstant ( _dateTimeFormat_, _instant_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand All @@ -905,7 +918,7 @@ <h1>HandleDateTimeTemporalInstant ( _dateTimeFormat_, _instant_ )</h1>
<h1>HandleDateTimeTemporalZonedDateTime ( _dateTimeFormat_, _zonedDateTime_ )</h1>

<p>
The abstract operation HandleDateTimeTemporalZonedDateTime accepts the arguments _dateTimeFormat_ (which must be an object initialized as a DateTimeFormat) and _zonedDateTime_ (which must be an ECMAScript value has an [[InitializedTemporalDateTime]] internal slot). It returns a record which contains the appropriate pattern and epochNanoseconds values for the input. This abstract operation functions as follows:
The abstract operation HandleDateTimeTemporalZonedDateTime accepts the arguments _dateTimeFormat_ (which must be an object initialized as a DateTimeFormat) and _zonedDateTime_ (which must be an ECMAScript value has an [[InitializedTemporalDateTime]] internal slot). It returns a record which contains the appropriate pattern, epochNanoseconds and timezone values for the input. This abstract operation functions as follows:
Aditi-1400 marked this conversation as resolved.
Show resolved Hide resolved
</p>

<emu-alg>
Expand All @@ -922,7 +935,8 @@ <h1>HandleDateTimeTemporalZonedDateTime ( _dateTimeFormat_, _zonedDateTime_ )</h
1. Return the Record {
[[pattern]]: _pattern_.[[pattern]],
[[rangePatterns]]: _pattern_.[[rangePatterns]],
[[epochNanoseconds]]: _instant_.[[Nanoseconds]]
[[epochNanoseconds]]: _instant_.[[Nanoseconds]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

(leaving a comment here because I can't leave it on line 918 above)

I think this spec text above may be a problem:

  1. If dateTimeFormat.[[TimeZone]] is not equal to DefaultTimeZone(), and timeZone is not equal to dateTimeFormat.[[TimeZone]], then
  2. Throw a RangeError exception.

If I'm reading this correctly, the code below would not throw when the DTF's timeZone option matches the system's time zone. Based on my understanding, this is not desired behavior. @ptomato @sffc is this a correct reading?

// run this code on a computer with system time zone America/Los_Angeles
zdt = Temporal.ZonedDateTime.from('2020-01-01[Asia/Tokyo');

new Intl.DateTimeFormat('en').format(zdt);
// => as expected, formats using ZDT time zone Asia/Tokyo

new Intl.DateTimeFormat('en', { timeZone: 'America/Denver' }).format(zdt);
// => as expected, RangeError because timeZone option is disallowed when formatting ZDT

new Intl.DateTimeFormat('en', { timeZone: 'America/Los_Angeles' }).format(zdt);
// expected: RangeError because timeZone option is disallowed when formatting ZDT
// actual: no exception, uses ZDT's time zone because time zone option matches DefaultTimeZone()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we'd need to change those lines above. If we go with my suggestion from a previous comment, I think we can remove them altogether because the check would be done in FormatDateTimePattern?

[[timeZone]]: _timeZone_,
Copy link
Contributor

Choose a reason for hiding this comment

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

the "[[timeZone]]: timeZone" here is always equal to "dateTimeFormat.[[TimeZone]]" and equal to "DefaultTimeZone()" because otherwise, a RangeError would already throw 3 steps above.

}.
</emu-alg>
</emu-clause>
Expand Down Expand Up @@ -950,7 +964,8 @@ <h1>HandleDateTimeOthers ( _dateTimeFormat_, _x_ )</h1>
1. Return the Record {
[[pattern]]: _pattern_,
[[rangePatterns]]: _rangePatterns_,
[[epochNanoseconds]]: _epochNanoseconds_
[[epochNanoseconds]]: _epochNanoseconds_,
[[timeZone]]: *undefined*
}.
</emu-alg>
</emu-clause>
Expand Down