Audit of user code calls, part 2 #2657
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
This was referenced Aug 18, 2023
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
==========================================
+ Coverage 96.10% 96.22% +0.12%
==========================================
Files 20 20
Lines 11771 11984 +213
Branches 2188 2227 +39
==========================================
+ Hits 11312 11532 +220
+ Misses 395 389 -6
+ Partials 64 63 -1
☔ View full report in Codecov by Sentry. |
ptomato
force-pushed
the
user-code-calls-part-2
branch
from
aca88c7
to
6a0553f
Compare
ptomato
force-pushed
the
user-code-calls-part-2
branch
2 times, most recently
from
e34ef5b
to
fa91d9f
Compare
ptomato
force-pushed
the
user-code-calls-part-2
branch
from
fa91d9f
to
05ac70e
Compare
|
I found one further opportunity to separate ZonedDateTime and PlainDateTime in "Normative: Separate zoned and plain operations in RoundDuration" so I've updated that commit to include it. |
ptomato
force-pushed
the
user-code-calls-part-2
branch
5 times, most recently
from
153c197
to
7765db5
Compare
|
FWIW, I took an extensive look at this as part of reviewing tc39/test262#3897 . Found one small bug that Philip's already fixed, and can report that each commit definitely does what it says on the box. |
justingrant
approved these changes
Sep 13, 2023
|
Also, shouldn't there be a commit at the end of this PR to update Test262? |
|
Also needs a rebase. |
Ms2ger
approved these changes
Sep 13, 2023
If the given rounding options specify a smallest unit of nanoseconds and a rounding increment of 1, then the rounding operation is a no-op. In that case, return a copy of the duration, without actually performing the rounding operation. This applies to all round(), until(), and since() methods. In the case of Duration.p.round(), other conditions must be fulfilled as well for the operation to be a no-op: no balancing must take place. In the case of Instant.p.round(), PlainDate.p.since(), PlainDate.p.until(), PlainDateTime.p.round(), PlainDateTime.p.since(), PlainDateTime.p.until(), PlainTime.p.round(), PlainTime.p.since(), and PlainTime.p.until(), the change is not observable, so implementors are free to make this optimization anyway. This optimization was already the case in PlainDate.p.since/until() and PlainYearMonth.p.since/until(), so this makes the other since/until() methods consistent with those.
…uration relativeTo as a PlainDate is only needed when smallestUnit is year, month, or week. relativeTo as a ZonedDateTime is used additionally when smallestUnit is day. However, ToTemporalDate only needs to be called in the former case. Since ToTemporalDate potentially calls user code, rearrange some steps to make sure to call it only when necessary. See: #2247
…mpare We call UnbalanceDurationRelative twice with the same relativeTo object. UnbalanceDurationRelative only uses plain or undefined relativeTo, not zoned, so it will convert it with ToTemporalDate. No need to do this twice; pre-convert it before the first call. See: #2247
This converts a ZonedDateTime relativeTo into a PlainDateTime relativeTo only when necessary, only after potentially throwing other errors, and only once. Previously, it could be converted up to a few separate times in each operation, such as UnbalanceDurationRelative, RoundDuration, and BalanceDurationRelative. Since the conversion is user-visible, we don't want to perform it when not necessary or perform it more times than necessary. Closes: #2247 Closes: #2529
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…Time There are several places where a ZonedDateTime is converted to a PlainDateTime, which is a user-visible time zone operation, and then the same operation is performed again right afterwards in AddZonedDateTime. Avoid this by making AddZonedDateTime take an optional precalculated PlainDateTime. If we have it, we can pass it in, and avoid the second conversion.
If all the fields are equal, then no balancing has to happen, and the relativeTo point doesn't matter; we know the durations are equal.
CalculateOffsetShift is only used in Duration.compare, when relativeTo is a ZonedDateTime. Calling it twice performs two ZonedDateTime additions, which are potentially user-observable method calls. If we are performing these two additions anyway, we may as well just add each duration to relativeTo and compare the timestamps. This saves a lot of other user-observable calls. It also allows removing the offset-shift parameter from TotalDurationNanoseconds, because now it is always zero. That parameter was confusing anyway. At the same time we remove the days parameter as well, because now there is no indication that days should only be 24-hour days; we push the responsibility of deciding whether 24-hour days are appropriate to the caller.
ptomato
force-pushed
the
user-code-calls-part-2
branch
from
5be195b
to
0455ddf
Compare
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.
Following on from #2645: #2519 is a large pull request that touches everything, and it's difficult to keep rebasing it while I work on the test coverage. I'd like to split it into parts, instead, and land each part as soon as the test coverage is ready. This is the second part, which consists of various removals of redundant observable operations. This now has test coverage in tc39/test262#3897.
These commits have already been reviewed in #2519. The changes consist only of rebasing.
Closes: #2247
Closes: #2529