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

Batch of rebases - early October 2023 #294

Merged
merged 12 commits into from
Feb 25, 2025
Merged

Batch of rebases - early October 2023 #294

merged 12 commits into from
Feb 25, 2025

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Feb 24, 2025

This brings the polyfill up to date as of early October 2023. The changes are mainly around removing unnecessary user calls.

ptomato and others added 12 commits February 20, 2025 15:11
This has been a bug in the polyfill since ZonedDateTime was originally
introduced! The spec says to convert the ZonedDateTime to PlainDateTime
once here, whereas the polyfill would Get each property in turn.

Tests will follow as part of #2519.

UPSTREAM_COMMIT=e7055b43b0af72a0439a7e6b30db5baf5457a368
Continue where #2667 left off by further clarifying this error message
and fixing a mistake where `+foo` was treated as a string coercion, not
the number coercion that it is.

UPSTREAM_COMMIT=820fffdaf32e9c6d3fbfda6d7674d57bc58eaa38
…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.

UPSTREAM_COMMIT=ec1df1f178578eecad0b027b8c96a20d30b2c590
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.

UPSTREAM_COMMIT=5194420e7d70fa1ae67c489f3b3b84955d1b2989
…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

UPSTREAM_COMMIT=1ff91a9730ace0af31177afab5c57bc343a5ab2f
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.

UPSTREAM_COMMIT=db1e0b33cd09284bf81fde141dec661e74844def
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().)

UPSTREAM_COMMIT=27c8876007c043982201f1939a2da3e8b696237a
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.

UPSTREAM_COMMIT=4bd36244f0de8420a08faa10d4614f7fe435d3a6
…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.

UPSTREAM_COMMIT=72d61c2296665aac023561ce00c6b11d6319a941
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

UPSTREAM_COMMIT=6eec81db8645823bd361310daa7b575fb702ac9f
Fix for bug in User Code Calls Part 2 (51ea969a), spotted by Anba. Thanks!
The existing code forgot to take negative durations into account.

Closes: #2679

UPSTREAM_COMMIT=26369ba0cf5be4b9dc8c6cecb5de16acba09e2e6
UPSTREAM_COMMIT=31a0156b7aac8e11571538d07cf4050cc55ae122
Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

Looks great!

One thing to think about is whether GetMethod should be replaced by simple property access. Not sure it adds much value and I assume it slows down perf. But to make rebasing easier, we can save changes like this for after we're all caught up to current.

@ptomato
Copy link
Contributor Author

ptomato commented Feb 24, 2025

Yep, in fact I'm running into type signature problems with GetMethod a bit down the line from here. But it's not worth going through and replacing; in the current state of the Temporal proposal, GetMethod isn't used at all.

@ptomato ptomato merged commit 81cf6d6 into main Feb 25, 2025
19 checks passed
@ptomato ptomato deleted the rebase branch February 25, 2025 00:19
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