Fast-path conversions between Temporal types #1484
Conversation
This was duplicated in the spec text.
This was clearly meant to be ToTemporalDate, not ToTemporalTime.
Codecov Report
@@ Coverage Diff @@
## main #1484 +/- ##
==========================================
- Coverage 95.65% 92.71% -2.94%
==========================================
Files 19 17 -2
Lines 10897 11181 +284
Branches 1730 1643 -87
==========================================
- Hits 10424 10367 -57
- Misses 469 801 +332
- Partials 4 13 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
If passing a Temporal object with a [[Calendar]] internal slot where a calendar is expected, then read that slot instead of doing a Get on the "calendar" property. (The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the CalendarFrom operation becomes redundant, so remove it.) See: #1428
Left separate for ease of reviewing.
If passing a Temporal.ZonedDateTime where a Temporal.Instant is expected, then create a Temporal.Instant directly from the ZonedDateTime's [[Nanoseconds]] internal slot. Previously, the ZonedDateTime's toString() method would have been called, and the result would have been parsed into an Instant. See: #1428
Left separate for ease of reviewing.
If passing a Temporal.PlainDateTime where a Temporal.PlainDate is expected, then create a Temporal.PlainDate directly from the PlainDateTime's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots. Previously, the PlainDateTime's calendar, year, month, monthCode, and day getters would have been called. See: #1428
If passing a Temporal.PlainDate where a Temporal.PlainDateTime is expected, then create a Temporal.PlainDateTime directly from the PlainDate's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots, and assume midnight as the time, just as if a property bag with year, month/monthCode, and day properties had been passed. Previously, the PlainDate's calendar, year, month, monthCode, and day getters would have been called, and the time properties would have been accessed as well. See: #1428
Left separate for ease of reviewing.
If passing a Temporal.PlainDateTime where a Temporal.PlainTime is expected, then create a Temporal.PlainTime directly from the PlainDateTime's [[ISOHour]], [[ISOMinute]], [[ISOSecond]], [[ISOMillisecond]], [[ISOMicrosecond]], and [[ISONanosecond]] internal slots. Previously, the PlainDateTime's hour, minute, second, millisecond, microsecond, nanosecond, and calendar getters would have been called. See: #1428
ptomato
force-pushed
the
1428-fast-paths
branch
3 times, most recently
from
c90387b
to
71ac595
Compare
|
OK, thanks for the reviews! Only the last two commits are substantially different from the previous version. |
There was a problem hiding this comment.
Really clever (not in a bad way - it's very clean and clear, just took a few minutes to wrap my brain around what would get called in what abstract op), well done!
I think fixes are needed to era, eraYear in polyfill, and era, eraYear, and month in Intl spec text. The tests look great, awesome work with the helper. Also optionally can remove redundant [[InitializedTemporalDate]] slot checks in the spec when the alternative is calling ToTemporalDate since ToTemporalDate shortcuts that path anyway, which is why the polyfill doesn't do that check.
Approved once those changes are in!
polyfill/test/Instant/prototype/toZonedDateTime/plain-custom-timezone.js
Outdated
Show resolved
Hide resolved
ptomato
force-pushed
the
1428-fast-paths
branch
from
71ac595
to
f61f775
Compare
If passing a Temporal.ZonedDateTime (that is, a Temporal object with a [[TimeZone]] internal slot) where a Temporal.TimeZone is expected, then read that slot instead of doing a Get on the "timeZone" property. The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the TimeZoneFrom operation becomes redundant, so remove it. Additionally, the check for the "timeZone" property on plain objects was not implemented according to the consensus in #925 (comment) and this change fixes that as well. See: #1428
The value of a relativeTo property in a property bag is already allowed to be either a Temporal.PlainDateTime or Temporal.ZonedDateTime. If passing a Temporal.PlainDate, then create a Temporal.PlainDateTime from its slots as we do in ToTemporalDateTime. See: #1428
There are a few places where the `calendar` property of a property bag is read. If the property bag is actually a Temporal object with a [[Calendar]] internal slot, then read this slot instead. See: #1428
…Time If passing a Temporal.ZonedDateTime where a Temporal.PlainDate, Temporal.PlainTime, or Temporal.PlainDateTime is expected, then first call the built-in TimeZone.getPlainDateTimeFor() method to obtain a Temporal.PlainDateTime object and proceed accordingly. Previously, the ZonedDateTime's getters would have been called, potentially calling the getPlainDateTimeFor() method multiple times; very inefficient. See: #1428
ptomato
force-pushed
the
1428-fast-paths
branch
from
f61f775
to
c9f30e2
Compare
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
In ParseTemporalTimeZoneString, a time zone name may meet the grammar for time zone names while still not being a supported time zone name. Therefore, this should be a throw rather than an assertion. This was probably left over from when the grammar was defined more loosely as "any string that is a supported time zone name".
The constructors of PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth, and ZonedDateTime call ToTemporalCalendar on the calendar argument, and the constructor of ZonedDateTime additionally calls ToTemporalTimeZone on its timeZone argument. Now that these operations include an observable HasProperty operation, we do not want to call them more than once on the same calendar or time zone object. Therefore we add CreateTemporalDate etc. which create an instance without any observable validation of the arguments. The spec text already works like this, with abstract operations such as CreateTemporalZonedDateTime and friends that do not call ToTemporalCalendar or ToTemporalTimeZone. So this is not a normative change; it's a compliance bug in the polyfill. It is required for the test262 tests in 19cd26f to pass, which observe HasProperty(`timeZone`) operations. See: #1428
ptomato
force-pushed
the
1428-fast-paths
branch
from
c9f30e2
to
12dab2e
Compare
|
I believe that I've addressed the comments, and would like to get this landed before the plenary so I'm going to go ahead and merge it. Happy to address any further comments as they come in. |
|
@ptomato Could you help me to answer this question in https://chromium-review.googlesource.com/c/v8/v8/+/3531554/8/src/objects/js-temporal-objects.cc#6190 " why the spec rejects this but allows this kind of object for monthCode? Just strikes me as odd." This is referring to the fact that Temporal.Calendar.prototype.monthCode will accept a PlainMonthDay object but Temporal.Calendar.prototype.month will throw TypeError exception The throw is added by you in this PR I also have problem to understand the reason behind it, it seems both ISOMonth and ISOMonthCode get the information from [[ISOMonth]] internal slot. |
|
@FrankYFTang I created an account on Gerrit and followed up there. |
|
Copying @ptomato's note from the other site to close the loop back here:
|
Not all the changes have test262 tests yet, but writing them was taking so long that I decided to go ahead and put this up for review anyway. I'll add the undone ones to #1398.
Closes: #1428