Port commits from proposal-temporal 2023-04-16 #244
Conversation
This avoids an observable HasProperty operation when the argument is a Temporal.Calendar instance; it gets returned directly. UPSTREAM_COMMIT=2a43b393647dcf330b152c4771d68033bb03c074
Similar to the previous commit, this avoids an observable HasProperty operation when the argument is a Temporal.TimeZone instance; it gets returned directly. UPSTREAM_COMMIT=54cea53bfc0a8887185416e31794fd3eec4670e2
My guess is that this check was part of #889 where we agreed to accept a property bag in this method, and then it became obsolete when we adopted the convention that a property bag with `timeZone` was valid for _any_ entry point that went through ToTemporalTimeZone, and it just never got removed. UPSTREAM_COMMIT=7dfbd806c5c6b24205fae8eed5cc8efb2da5b878
…eTime
Exactly the same as the change in ToTemporalTimeZone, but this separate
check is necessary because the argument to PlainDate.p.toZonedDateTime is
also allowed to have the shape { timeZone, plainTime? }.
UPSTREAM_COMMIT=fcab1af41eae56600da3314ce226d24dd985e11e
…spec This early return is present in the spec text but was missing from the reference code. UPSTREAM_COMMIT=30d4d58aabd7afd36dfa4493b5fc91dd52a5465d
UPSTREAM_COMMIT=a192c062b523650aa3d74b107290b60646525feb
UPSTREAM_COMMIT=92f1d4ff64e20b69f1f7adc86c45bc2590a5740d
When converting a number of nanoseconds to a number of days and a nanoseconds remainder, neither the resulting number of days nor the nanoseconds remainder should have the opposite sign as the original number of nanoseconds. This could happen due to shenanigans in a custom time zone's getOffsetNanosecondsFor or getPossibleInstantsFor methods, or a custom calendar's dateAdd method. See: #2357 UPSTREAM_COMMIT=e13c52d0c0e89849ccf326ac7ea6c7190fc1235d
When converting a number of nanoseconds to a number of days and a nanoseconds remainder, the remainder shouldn't be longer than the length of the last day. This could happen due to shenanigans in a custom time zone's getOffsetNanosecondsFor or getPossibleInstantsFor methods, or a custom calendar's dateAdd method. See: #2357 UPSTREAM_COMMIT=ac69b63a5904620d0271238f028b4cee068bfcad
For consistency with ECMA-402, where calendar names are case-insensitive,
Temporal should also accept calendar names case-insensitively.
This clarifies definitively that the following are all valid:
new Temporal.Calendar('jApAnEsE')
Temporal.Calendar.from('jApAnEsE')
Temporal.Calendar.from('2022-08-30[u-ca=jApAnEsE]')
This is in line with the already-existing
new Date().toLocaleString('en', { calendar: 'jApAnEsE' })
new Date().toLocaleString('en-u-ca-jApAnEsE')
UPSTREAM_COMMIT=03101c6f03f671bac707d8134ff5504ec708b09f
See: #2112 UPSTREAM_COMMIT=f2746783e808c0144f2ce669e004a8c6286f9fc7
See: #2112 UPSTREAM_COMMIT=768b9163d47a375426b32bc734724f89d63a5081
See: #2112 UPSTREAM_COMMIT=1dceb57ed653253a65f1bd320ed55b7b8f38545e
Throwing earlier in the abstract operation (though it is indistinguishable from user code's perspective) prevents the editorial error of trying to calculate remainder(±∞, 1) which is not the domain that the remainder function is defined on. Closes: #2389 UPSTREAM_COMMIT=96e0e3ecab3c0f7d5940045d76b63da6a8e86c4e
In ToTemporalZonedDateTime, the spec text has the property accesses of the 'offset' and 'timeZone' properties before the call to InterpretTemporalDateTimeFields. This is necessary, because the user code in InterpretTemporalDateTimeFields could poison those properties. If they are to be read infallibly, they need to be read before user code can touch them. This is already the case in the spec text. The reference code needs to be changed to match. UPSTREAM_COMMIT=669ea82f93ab085e7a175e9afabbd1f9aa71095a
The "show" could be confusing because the option's property name isn't called that. See: #2223 UPSTREAM_COMMIT=ab5b1ba91054a921e2ae129e3fe024bd1c6e7f2f
As in ToTimeZoneNameOption, the "show" could be confusing because the option's property name isn't called that. Closes: #2223 UPSTREAM_COMMIT=2c9e2615f74a5ff15a812dac5cc2c3fa0c4a5922
The order of observable operations in the reference code was not correct according to the spec text. This commit reorders them. UPSTREAM_COMMIT=4b85b88160ecc6843822b2d1e07cb8604dfd0afd
The order of observable operations in the reference code was not correct according to the spec text. This commit reorders them. UPSTREAM_COMMIT=c32d7f14f10a92f946210ec67f85e75a5d851cb6
…spec The order of observable operations in the reference code was not correct according to the spec text. This commit reorders them. UPSTREAM_COMMIT=14f4ef37e2a3ec98a705cd8b0d96f09aee8d5128
Due to PrepareTemporalFields a few steps above this, the `offset` property of _fields_ is already guaranteed to be an own data property with a primitive value, which is either a string or undefined. So, there is no need to convert it to a string here. We can remove it because this conversion was not observable. UPSTREAM_COMMIT=24ebcbd80b5f1aea26b5b030d41127476fbe3dce
UPSTREAM_COMMIT=ed1364a094736494751a2352db61607e9758ddff
In order to prevent bugs like `new Temporal.ZonedDateTime(0n, cal, tz)` (where the calendar and time zone arguments are switched, silently poisoning the object) ToTemporalCalendar should throw if it encounters a Temporal.TimeZone instance, and ToTemporalTimeZone should throw if it encounters a Temporal.Calendar instance. Includes implementation in the reference code. Co-authored-by: Aditi <aditisingh1400@gmail.com> Closes: #2354 UPSTREAM_COMMIT=2084e772cd79c2113a5ef6343984bf7fd49f8200
Ms2ger
left a comment
Ms2ger
left a comment
There was a problem hiding this comment.
It seems like there's a fair number of these commits that don't change expectations, though they seems to be observable. Are those indicative of missing coverage?
|
Yes, I think that every observable commit that doesn't also change the expected failures file is probably a sign of a possible test gap. Maybe it's possible in some cases that the tests were deterred? @ptomato may know. I'll open an issue in the proposal repo to investigate this further. |
|
Here are the ones where that seems to be the case: All except the last one in the list have to do with observable order of operations. My guess is that each of those individual commits were necessary but not sufficient to make one of the test262 If you can identify the normative PR in the proposal-temporal repo that corresponds to a commit in this repo, I have been fairly consistently linking the test262 PRs that correspond to them (and if I forgot, there's likely a commit in the PR labeled "Update test262" so you can see which test262 commit it points to) |
|
@justingrant I wonder if something that might be useful would be a "test262 results" comparison system built into the rebase tool where we could gather up the expected-failures lists from both versions and compare them to see whether there are tests that should have been re-enabled (or disabled) but didn't for some reason? It would probably mean having to maintain another expected-failures-like list of test cases we know should be different from upstream "for reasons", but might make rebasing easier overall? |
justingrant
left a comment
justingrant
left a comment
There was a problem hiding this comment.
Thanks for the review! A few responses and notes below.
Also, do you know if there's a trick to keeping the original committer's name on a commit when rebasing using -i? The last few times I tried this, it lost Philip on the updated commit.
The upstream repo has essentially no expected failures, except weird cases like V8 bugs and for those this repo should fail too. Also, the tests are somewhat out of sync because upstream the Test262 commit is usually the last one in the PR, but here I've been removing tests from the expected failures file for each commit that needs them. So I'm not sure how what you describe will be helpful. But maybe I'm missing something? FWIW, dealing with Test262 using the test runner option you built has been a huge help. I just run every Test262 with that option set. So Test262 is essentially a non-issue for the porting workflow... adds zero extra work for me, because my workflow is this:
After doing this 50+ times I'm getting pretty fast at it. If we can fix the ecmascript thing to be merge conflicts then I could probably 2x+ my pace. |
Implements the APIs added in the previous commit in the reference code. (No support in non-ISO calendars currently; they just default to returning the ISO yearOfWeek. Probably in an ICU-based implementation this would be different.) UPSTREAM_COMMIT=aa1c7b1193d176578a9b1b0a5cc4021966192476
Previously, the abstract operation ToTemporalRoundingIncrement was technically unimplementable because of trying to calculate ℝ(infinity). Address this by separating out the case where there is no maximum for roundingIncrement into a separate code path from the case where there is a maximum (which goes into a separate abstract operation, ValidateTemporalRoundingIncrement.) The separation is a bit awkward; it would be better if the truncated integer was returned from ToTemporalRoundingIncrement, instead of the original Number value, but that's not possible without a normative change because currently, the range check is done with the original Number value. (This is inconsistent with fractionalSecondDigits, where the range check is done with the truncated integer.) I'm planning to make this normative change as part of #2254 since we are changing the order of observable property accesses on options bags anyway. Closes: #2226 UPSTREAM_COMMIT=712c449f53e385433aaa663f326f26998ec81329
Some time ago we deduplicated some code in the various difference operations into GetDifferenceSettings. This makes the corresponding change in the reference code. UPSTREAM_COMMIT=fe806faf164d7fd72b4ea41150a0df04adb8c45c
This operation was already out of sync between the reference code and the spec text because it was called from entirely different places. Remove it altogether, inlining it into all of its call sites both in the spec text and the polyfill. UPSTREAM_COMMIT=aca38be8052aeebd013c040e4c88d91a8731c73d
Implements the normative change from the previous commit in the reference code: annotations are permitted after date-only notations, and UTC offsets are forbidden. UPSTREAM_COMMIT=2b663f38167408b9f73b78295f821959a35ff1b9
When the list of properties to access is known, they should be accessed in alphabetical order. This changes PrepareTemporalFields to sort its list of properties so that the alphabetical order is guaranteed. See: #2254 UPSTREAM_COMMIT=6f35271d76c48b2078c1fa6590afee5973e4578a
We now sort the field names passed to PrepareTemporalFields, so we can remove a now-redundant sort when it is performed just before calling PrepareTemporalFields. UPSTREAM_COMMIT=c48a6eabda4604f4388245f0e0fe7d4585bf2ca4
The 'offset' and 'timeZone' fields were accessed separately in ToRelativeTemporalObject; instead, append them to the list of field names passed to PrepareTemporalFields, so that they are accessed in alphabetical order just like in ToTemporalZonedDateTime. UPSTREAM_COMMIT=a4f78da004e5fb16be631f13796bce1cb5cf64eb
In various types' toString() methods, this splits the observable property access of the fractionalSecondDigits and smallestUnit properties of options bags, into separate operations from the validation (which needs the values of both properties combined.) The accesses are not reordered in this commit relative to other options properties, although they will be in a following commit. The normative change here is that the fractionalSecondDigits property is always observably accessed, even if smallestUnit takes precedence. The correct way is now: - Call ToFractionalSecondDigits to access the fractionalSecondDigits prop - Call GetTemporalUnit to access the smallestUnit prop - Throw if smallestUnit is not an allowed value - Call ToSecondsStringPrecision to determine which of the two options "wins" and obtain the information about how precisely to print the result ToSecondsStringPrecision becomes infallible now. See: #2254 UPSTREAM_COMMIT=62a85c123f073c928d4803cb0fd13be4f6862f5d
This brings the validation of roundingIncrement options in line with that of fractionalSecondDigits options, and allows performing the truncation in ToTemporalRoundingIncrement instead of in ValidateTemporalRoundingIncrement. The latter no longer needs to return a value. UPSTREAM_COMMIT=8fc8130ddb5257f84d12dd5c2d541af89804a0bf
For every Temporal API that takes an options bag, ensure that the options bag's properties are accessed in alphabetical order. Validation of a property's value should normally follow the property access. An exception to this is validation that requires the values of more than one property. This should be done after both property values are obtained. See: #2254 UPSTREAM_COMMIT=2cc46eb9bfa798bfe2973e4a73437d9c2792077b
…cord UPSTREAM_COMMIT=60f105239e4667dd25d24067d522fa59b9eed553
This moves the check for the special case (where dividend is 1 and inclusive is false) out of ValidateTemporalRoundingIncrement, and into the two call sites where it matters (PDT.p.round and ZDT.p.round). This will hopefully make the logic in ValidateTemporalRoundingIncrement easier to follow. UPSTREAM_COMMIT=9bd27a20ef157c69d4d4954e64724eb332b32723
justingrant
force-pushed
the
port-commits-2023-04-16
branch
from
5799732 to
dd67b85
Compare
5 participants
Big batch of migrated commits! I think this is about 40% of the remaining commits, although the huge ones like the string slot changes are still ahead.
Thanks again @12wrigja for the great tools to make this easier.