Audit of user code calls, part 3 #2671
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tests are in tc39/test262#3923. |
This was referenced Sep 14, 2023
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2671 +/- ##
==========================================
+ Coverage 96.22% 96.31% +0.08%
==========================================
Files 20 20
Lines 11987 12200 +213
Branches 2227 2285 +58
==========================================
+ Hits 11535 11750 +215
+ Misses 389 386 -3
- Partials 63 64 +1
☔ View full report in Codecov by Sentry. |
gibson042
reviewed
Sep 14, 2023
ptomato
force-pushed
the
user-code-calls-part-3
branch
2 times, most recently
from
8520009
to
78edc4a
Compare
|
I pushed two additional changes to "Normative: Fast-path AddDaysToZonedDateTime in AddZonedDateTime" and "Normative: Fast-path dateUntil() when difference largest unit is days" to inline the fast paths from AddZonedDateTime and DifferenceISODateTime into places where days-and-time-units-only arithmetic was done. These are not strictly necessary here, but it makes things more convenient in Part 4. |
ptomato
force-pushed
the
user-code-calls-part-3
branch
3 times, most recently
from
a47eaf5
to
26c1d65
Compare
This was referenced Sep 25, 2023
ptomato
force-pushed
the
user-code-calls-part-3
branch
from
26c1d65
to
59d696e
Compare
Ms2ger
approved these changes
Sep 26, 2023
ptomato
force-pushed
the
user-code-calls-part-3
branch
2 times, most recently
from
36cf559
to
817a3d7
Compare
cjtenny
reviewed
Oct 4, 2023
There was a problem hiding this comment.
Just had one question from reviewing tc39/test262#3923 , see below. Everything else looks great and clear!
…o ZDT We call AddZonedDateTime in several places with all of the duration components being zero except for days. In this case, we can skip the calendar operations altogether, because adding/subtracting days to/from the ISO fields is not calendar-dependent.
In the case where AddZonedDateTime is called with years, months, and weeks all zero, we can fall back to AddDaysToZonedDateTime to avoid the calendar call, and then AddInstant on the result. In BalancePossiblyInfiniteTimeDurationRelative, inline the fast path from AddZonedDateTime since we are not adding years, months, or weeks, and can now no longer call any calendar methods. Give all intermediate objects the ISO 8601 calendar for simplicity.
…o PlainDate In a few places, we called CalendarDateAdd with a days-only duration. For days-only, it's not necessary to consult the calendar: we can just add the days in the ISO calendar space to the ISO calendar values in the internal slots. (also fixes a rebase error with truncate(_fractionalDays_)) Closes: #2685
Introduce an operation AddDate, which has a fast-path through the ISO calendar in the case where we would otherwise call calendar.dateAdd() with years, months, and weeks all zero.
If two Temporal objects have identical internal slots, then we can skip calculating the difference with a calendar method; the difference is always 0. This optimization isn't necessary for PlainTime or Instant differences, since no calendar methods are called for those. Implementations can return early for PlainTime or Instant if they like, but it won't be reflected in the spec text in order to keep things as simple as possible. Note that the early return still comes after the processing of the options object. Passing an illegal options object to until() or since() should still throw, even if the difference is zero. (We'll also fast-path CalendarDateUntil, but this fast path also cuts out calls to calendar.dateFromFields().)
Introduce an operation DifferenceDate, which has a fast-path through the ISO calendar (via DaysUntil) in the case where we would otherwise call calendar.dateUntil() with largestUnit === "day". Also return a blank duration immediately if the two dates are the same day. Note that this makes it impossible for the following BalanceDuration call in DifferenceISODateTime to throw. The dateUntil() method will no longer be called for largestUnits of "day" or less. So dateDifference.[[Days]] can be at most Number.MAX_VALUE, but timeDifference cannot balance into more than one day and make it overflow in the BalanceDuration call. In NanosecondsToDays, inline the fast path from DifferenceISODateTime since we only have largestUnit === "day" there and can now no longer call any calendar methods. Give all intermediate objects the ISO 8601 calendar for simplicity. Note, as evident from the corresponding test262 tests, this removes several loopholes where it was possible to return particular values from user calls that would cause infinite loops, or calculate zero-length days.
…mbiguous datetime In DisambiguatePossibleInstants, when getPossibleInstantsFor has returned an empty array, we calculate the UTC offset in the time zone one day before and one day after. Don't look up the method twice when doing this. Similarly, in InterpretISODateTimeOffset, when getPossibleInstantsFor has returned more than one possibility, we compare the offset nanoseconds of each possibility. Also don't look up the method twice when doing this.
There are a few more places where we can avoid doing an additional lookup and call of getOffsetNanosecondsFor on the same ZonedDateTime, to convert it into a PlainDateTime. This affects - Temporal.Duration.prototype.add (with relativeTo ZonedDateTime) - Temporal.Duration.prototype.subtract (ditto) - Temporal.Duration.prototype.round (ditto) - Temporal.Duration.prototype.total (ditto) - Temporal.ZonedDateTime.prototype.since - Temporal.ZonedDateTime.prototype.until (also fixes "and" vs "or" prose mistakes) Closes: #2680 Closes: #2681
ptomato
force-pushed
the
user-code-calls-part-3
branch
from
a2239a8
to
9a3c08b
Compare
|
On to part 4! |
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Oct 11, 2023
…ants In the AO DisambiguatePossibleInstants, a PlainDateTime instance is passed to user code. This instance should have the built-in ISO 8601 calendar. Here are some tests that ensure it does. See tc39/proposal-temporal#2671.
ptomato
added a commit
that referenced
this pull request
Oct 11, 2023
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Oct 12, 2023
…ants In the AO DisambiguatePossibleInstants, a PlainDateTime instance is passed to user code. This instance should have the built-in ISO 8601 calendar. Here are some tests that ensure it does. See tc39/proposal-temporal#2671.
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Oct 18, 2023
…ants In the AO DisambiguatePossibleInstants, a PlainDateTime instance is passed to user code. This instance should have the built-in ISO 8601 calendar. Here are some tests that ensure it does. See tc39/proposal-temporal#2671.
ptomato
added a commit
to tc39/test262
that referenced
this pull request
Oct 18, 2023
…ants In the AO DisambiguatePossibleInstants, a PlainDateTime instance is passed to user code. This instance should have the built-in ISO 8601 calendar. Here are some tests that ensure it does. See tc39/proposal-temporal#2671.
ptomato
added a commit
that referenced
this pull request
Oct 18, 2023
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696
ptomato
added a commit
that referenced
this pull request
Oct 26, 2023
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696
ptomato
added a commit
that referenced
this pull request
Nov 6, 2023
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696
ptomato
added a commit
that referenced
this pull request
Nov 6, 2023
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Next part of #2519. These commits have mostly been reviewed already as part of that PR. Changes are mostly due to rebasing, and addressing of review comments that were left on #2519. However, there are a few fixes to minor mistakes that I noticed while rebasing, and a few changes to the commits: