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.

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

Normative: Check for invalid epoch nanoseconds in Temporal.TimeZone.prototype.get{Next,Previous}Transition #2311

Closed
wants to merge 1 commit into from

Conversation

anba
Copy link
Contributor

@anba anba commented Jun 17, 2022

GetIANATimeZoneNextTransition and GetIANATimeZonePreviousTransition
aren't required to return an epoch nanoseconds value which is within the
supported range, therefore we have to call IsValidEpochNanoseconds
before invoking CreateTemporalInstant.

…rototype.get{Next,Previous}Transition

`GetIANATimeZoneNextTransition` and `GetIANATimeZonePreviousTransition`
aren't required to return an epoch nanoseconds value which is within the
supported range, therefore we have to call `IsValidEpochNanoseconds`
before invoking `CreateTemporalInstant`.
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #2311 (ebb098a) into main (d96e662) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2311   +/-   ##
=======================================
  Coverage   91.08%   91.08%           
=======================================
  Files          19       19           
  Lines       10566    10566           
  Branches     1695     1695           
=======================================
  Hits         9624     9624           
  Misses        932      932           
  Partials       10       10           
Flag Coverage Δ
test262 83.94% <ø> (ø)
tests 81.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d96e662...ebb098a. Read the comment docs.

@anba anba mentioned this pull request Jun 17, 2022
@@ -308,6 +308,7 @@ <h1>Temporal.TimeZone.prototype.getNextTransition ( _startingPoint_ )</h1>
1. If _timeZone_.[[OffsetNanoseconds]] is not *undefined*, return *null*.
1. Let _transition_ be GetIANATimeZoneNextTransition(_startingPoint_.[[Nanoseconds]], _timeZone_.[[Identifier]]).
1. If _transition_ is *null*, return *null*.
1. If ! IsValidEpochNanoseconds(_transition_) is *false*, throw a *RangeError* exception.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might have gone for returning null, but either way it doesn't really matter.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

We could also define the GetIANATimeZone...Transition ops so that they guarantee not to return an out-of-range time and return null instead. That seems more in line with what e.g. SystemUTCEpochNanoseconds does; it clamps the value to the range. Clamping wouldn't be appropriate here, I think, but returning null would. We did receive guidance specifically during the Stage 3 reviews of this proposal not to throw, so it seems in any case that we shouldn't throw here either.

@anba
Copy link
Contributor Author

anba commented Jun 21, 2022

I think both options are probably fine. There's a slight semantic difference:

  1. Throwing an error indicates that there's another transition, but we can't represent it.
  2. Whereas returning null means there are no further transitions.

Returning null allows to write more concise code, though:

let instant = startInstant;
while (instant !== null) {
  // Use `instant`.

  instant = tz.getNextTransition(instant);
}

When an error is thrown, we'd need to wrap the call into a try-catch block:

let instant = startInstant;
while (instant !== null) {
  // Use `instant`.

  try {
    instant = tz.getNextTransition(instant);
  } catch {
    instant = null;
  }
}

But then again going through all transitions seems a rather unlikely case to happen in practice.

@justingrant
Copy link
Collaborator

IMO we should not throw an errors for built-in time zones like this. The system is supposed to be reliable! So I'd definitely recommend null in this case.

Or should we go further and add an assertion that the implementation must not return out-of-range values from built-in time zone implementations?

@ptomato
Copy link
Collaborator

ptomato commented Jul 7, 2022

Meeting 2022-07-07: we should prevent the internal GetIANATimeZone{Previous,Next}Transition operations from returning epoch nanoseconds that are out of range. Therefore I will close this PR and open a new one that fixes the issue that way.

@ptomato ptomato closed this Jul 7, 2022
@ptomato
Copy link
Collaborator

ptomato commented Jul 7, 2022

#2351

@anba anba deleted the transition-validate-instant branch August 4, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants