-
Notifications
You must be signed in to change notification settings - Fork 153
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
Duration normalization, part 2 of 3 #2727
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2727 +/- ##
==========================================
- Coverage 96.73% 96.58% -0.16%
==========================================
Files 21 23 +2
Lines 12444 12226 -218
Branches 2255 2280 +25
==========================================
- Hits 12038 11808 -230
- Misses 349 359 +10
- Partials 57 59 +2 ☔ View full report in Codecov by Sentry. |
0620aed
to
8b6cbab
Compare
b9f2c58
to
0e2f662
Compare
0e2f662
to
08722d4
Compare
Thanks for the comments, @anba. I've addressed some of them and pushed up an interim version of the PR. I'll continue addressing the remaining ones in the next few days. |
be83c49
to
89235f9
Compare
89235f9
to
854f5e1
Compare
854f5e1
to
21653be
Compare
40dde05
to
66a1b45
Compare
In order to avoid unbounded integer arithmetic, we place an upper bound on the total of the time portion of a duration (days through nanoseconds). For the purpose of determining the limits, days are always 24 hours, even if a calendar day may be a different number of hours relative to a particular ZonedDateTime. It's now no longer possible to make Duration.prototype.total() come up with an infinite result; removing the loops in UnbalanceDurationRelative made it so that the duration's calendar units must be able to be added to a relativeTo date without overflowing, and this change makes it so that the duration's time units are too small to overflow to infinity either.
This introduces Normalized Time Duration Records, which we use to encapsulate 96-bit integer operations on duration times. (In the reference polyfill, the TimeDuration class fulfills the same purpose.) These operations are specified naively in the mathematical value domain, but can be changed in a later editorial commit to correspond to how implementations would write 64+32 bit operations, if we so desire. (The results must be exactly the same, so that can be decided later, outside of a TC39 plenary.) This commit also replaces TotalDurationNanoseconds with NormalizeTimeDuration, and NanosecondsToDays with NormalizedTimeDurationToDays. Several operations are changed to return a Normalized Duration Record, which is a Normalized Time Duration record combined with a Date Duration Record. Having already limited time units of durations in the previous commit, this does not affect any results, nor any existing tests in test262. But I can't prove conclusively that there isn't some edge case somewhere that makes this change observable. (also obsoletes several pre-existing editorial mistakes) Closes: #2536 Closes: #2638 Closes: #2616
Carefully crafted custom time zones can cause NormalizedTimeDurationToDays to calculate a day length that is arbitrarily long. In order for implementations not to have to use a normalized time duration or bigint to represent the day length in nanoseconds, limit it to less than 2⁵³ ns. This way, it can be stored in a 64-bit float. This allows time zones to specify day lengths of up to ~104.25 real 24-hour days, which should be more than enough. This also ensures that the number of days returned from NormalizedTimeDurationToDays is within the limit of 2⁵³/86400, which allows implementations to use 64-bit integer addition in RoundDuration.
In order to prevent having to use bigint arithmetic, limit years, months, and weeks to 32 bits each in durations. There are more changes to the reference code than to the spec in this commit because the upper limit now allows us to rewrite the reference code's RoundDuration algorithm in a way that's more similar to how it was already specified in the spec text.
I believe all comments have been addressed and tests are complete now (tc39/test262#3961). (They are still showing up as failing, because the CI runs them against the pinned commit of test262, but they pass against that branch.) The change has had scrutiny from several people (either as part of #2612 or on this PR thread) and is just missing a review from someone with merge privileges. |
66a1b45
to
006a2f9
Compare
(Note: stacked on top of #2722, will rebase as appropriate)This includes everything from #2612 except the last commit, which I'm still checking for a potential bug. These are the changes that place the limits on each component of a Temporal.Duration, and implement the internal calculations in a "normalized form" - 32-bit years, months, weeks, and time units expressed as one <96-bit integer.