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

Replace BalanceISODate(Time) and rearrange time zone offset checks #3014

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

anba
Copy link
Contributor

@anba anba commented Oct 17, 2024

Disclaimer: I haven't yet implemented these changes, so it's possible that there are still some bugs in the proposed changes.

  1. The first three commits replace BalanceISODateTime and BalanceISODate with less general operations.
    • BalanceISODateTime is only used to add a time zone offset to a date-time. It seems like all but one occurrence can be replaced by plain subtraction. For that one caller where subtraction isn't possible, either rename BalanceISODateTime to AddOffsetNanosecondsToISODateTime or alternatively just inline it into GetISODateTimeFor. Commits for both alternatives are prepared.
    • And BalanceISODate is only used to add a days amount to an ISO Date record. Replace it with AddDaysToISODate, so it's easier to see what this operation does.
  2. The remaining commits reorder CheckISODaysRange calls to happen earlier, so it's easier to see (and implement) when ISO Date-Time records contain out-of-range values.
    • CheckISODaysRange currently allows inputs from nsMinInstant (inclusive) to nsMaxInstant + nsPerDay (exclusive). That looks like a bug, I think the lower limit should be nsMinInstant - nsPerDay (exclusive). This range can be checked through ISODateTimeWithinLimits.
    • In two places ISODateTimeWithinLimits is used to check GetUTCEpochNanoseconds inputs instead of CheckISODaysRange. Either use CheckISODaysRange consistently or inline CheckISODaysRange everywhere. Commits for both alternative are also prepared here.
    • All changes are editorial, because they only reorder when RangeError exceptions are thrown, but this isn't visible from user-code, because no other side-effects can happen.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.97%. Comparing base (c75c841) to head (9578597).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3014      +/-   ##
==========================================
+ Coverage   95.92%   95.97%   +0.04%     
==========================================
  Files          21       21              
  Lines        9677     9684       +7     
  Branches     1829     1745      -84     
==========================================
+ Hits         9283     9294      +11     
+ Misses        341      337       -4     
  Partials       53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thanks for the PR! I like most of the changes but some of the stuff around CheckISODaysRange I'm not sure is correct. I implemented it in this repo's reference code and I get different behaviour around the edges of the representable range.

1. Let _epochNanoseconds_ be GetUTCEpochNanoseconds(_balanced_).
1. Let _isoDate_ be CreateISODateRecord(_parsed_.[[Year]], _parsed_.[[Month]], _parsed_.[[Day]]).
1. Let _isoDateTime_ be CombineISODateAndTimeRecord(_isoDate_, _time_).
1. Perform ? CheckISODaysRange(_isoDate_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

In some cases, this will be incorrect, because the parsed YMD may be different from the actual YMD after balancing. e.g. Temporal.Instant.from("-271821-04-19T23:00-01:00"), the parsed YMD is out of range, but the instant itself is in range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't be a problem after 77bef95 when the allowed input range is changed from [nsMinInstant, nsMaxInstant + nsPerDay) to (nsMinInstant - nsPerDay, nsMaxInstant + nsPerDay).

@@ -915,9 +915,8 @@ <h1>
1. If _offsetBehaviour_ is ~wall~, or _offsetBehaviour_ is ~option~ and _offsetOption_ is ~ignore~, then
1. Return ? GetEpochNanosecondsFor(_timeZone_, _isoDateTime_, _disambiguation_).
1. If _offsetBehaviour_ is ~exact~, or _offsetBehaviour_ is ~option~ and _offsetOption_ is ~use~, then
1. Let _balanced_ be BalanceISODateTime(_isoDate_.[[Year]], _isoDate_.[[Month]], _isoDate_.[[Day]], _time_.[[Hour]], _time_.[[Minute]], _time_.[[Second]], _time_.[[Millisecond]], _time_.[[Microsecond]], _time_.[[Nanosecond]] - _offsetNanoseconds_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 122 to 123
1. If abs(ISODateToEpochDays(_isoDate_.[[Year]], _isoDate_.[[Month]] - 1, _isoDate_.[[Day]])) > 10<sup>8</sup>, then
1. Let _dateTime_ be CombineISODateAndTimeRecord(_isoDate_, MidnightTimeRecord()).
1. If ISODateTimeWithinLimits(_dateTime_) is *false*, then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't equivalent. (I know that's intentional from your description, but I'm not sure it's correct.) CheckISODaysRange has that weird range because that's the range that will avoid running into the undefined behaviour from tc39/ecma262#1087.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current CheckISODaysRange definition effectively allows inputs in the range [nsMinInstant, nsMaxInstant + nsPerDay), but I think the allowed range should instead be (nsMinInstant - nsPerDay, nsMaxInstant + nsPerDay).

Confusingly the ECMA-262 spec sometimes mentions that inputs resp. outputs are time values (i.e. times within the [nsMinInstant, nsMaxInstant] range), but the actually allowed times are with the local time zone offset applied. For example https://tc39.es/ecma262/#sec-date.prototype.getdate:

  1. Return DateFromTime(LocalTime(t)).

LocalTime(t) returns a local time zone adjusted value, which can be outside the valid time value bounds, but per the DateFromTime description, DateFromTime should only be called with time values.

spec/abstractops.html Outdated Show resolved Hide resolved
spec/zoneddatetime.html Outdated Show resolved Hide resolved
@anba anba force-pushed the balance-iso-date branch from 15f103a to 9578597 Compare November 5, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants