Normative: Avoid duplicate reads of since/until options data #2447
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
Codecov Report
@@ Coverage Diff @@
## main #2447 +/- ##
==========================================
- Coverage 94.97% 94.73% -0.24%
==========================================
Files 20 20
Lines 10905 11156 +251
Branches 1995 1981 -14
==========================================
+ Hits 10357 10569 +212
- Misses 487 540 +53
+ Partials 61 47 -14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
ljharb
reviewed
Dec 8, 2022
| @@ -1796,7 +1796,7 @@ <h1> | |||
| <h1> | |||
| GetDifferenceSettings ( | |||
| _operation_: ~since~ or ~until~, | |||
| _options_: an ECMAScript language value, | |||
| _options_: an Object, | |||
There was a problem hiding this comment.
should there be some kind of assertion here that this is an ordinary object with only data properties whose [[Prototype]] is null?
There was a problem hiding this comment.
I think it's fine without that. The operation remains meaningful regardless (and is not relying upon such assumptions), we're just choosing not to employ it with potentially tricky input.
ptomato
approved these changes
Dec 29, 2022
There was a problem hiding this comment.
I had thought about proposing something like this for #2254 but ended up not suggesting it due to it not being an existing precedent in Intl. Now I wish I had, though — it makes a lot of sense, especially for options objects that are passed to calendar methods, and seems pretty uncontroversial either way. The timing is a bit unfortunate for implementors, coming on the heels of that earlier change, but we can try to get all of the observable operation reorderings done and present them at the same time.
littledan
approved these changes
Feb 2, 2023
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Feb 7, 2023
In since() and until() methods, we copy the options object with CopyDataProperties. Previously, its properties could be read in more than one place (the method itself, and the calendar method), triggering user code each time. Now, we pass around null-prototype objects with only data properties. See tc39/proposal-temporal#2447
I've made these changes in tc39/test262#3781, though I could not find anything that needed to be changed in the last one in that list. |
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Feb 14, 2023
In since() and until() methods, we copy the options object with CopyDataProperties. Previously, its properties could be read in more than one place (the method itself, and the calendar method), triggering user code each time. Now, we pass around null-prototype objects with only data properties. See tc39/proposal-temporal#2447
ptomato
added a commit
to tc39/test262
that referenced
this pull request
Feb 14, 2023
In since() and until() methods, we copy the options object with CopyDataProperties. Previously, its properties could be read in more than one place (the method itself, and the calendar method), triggering user code each time. Now, we pass around null-prototype objects with only data properties. See tc39/proposal-temporal#2447
Make a clone in iteration order and read from that
Properties not read by GetDifferenceSettings are irrelevant, but this preserves consistency with analogous since/until operations on calendar-bearing types.
ptomato
force-pushed
the
2022-12-difference-options-observable-reads
branch
from
4020d99
to
b1584b6
Compare
|
This was adopted in the January-February 2023 TC39 meeting, and the tests have landed in test262 now. |
ptomato
added a commit
that referenced
this pull request
Mar 11, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Mar 11, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Apr 22, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Apr 22, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Apr 26, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Apr 26, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Apr 27, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Apr 27, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
May 3, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
May 3, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
May 11, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
May 11, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
May 13, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
May 13, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
May 19, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
May 19, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
May 31, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
May 31, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Jun 12, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Jun 12, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Jun 20, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Jun 20, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately. (We don't necessarily have to include this commit if others don't want it. I'm including it for completeness because that's what we did in Richard's change in #2447.)
ptomato
added a commit
that referenced
this pull request
Aug 11, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). In PlainDateTime.from, delay validation of the options until after validation of the ISO string, for consistency with ZonedDateTime.from and in accordance with our general principle of validating arguments in order. This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Aug 11, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately.
ptomato
added a commit
that referenced
this pull request
Aug 16, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). In PlainDateTime.from, delay validation of the options until after validation of the ISO string, for consistency with ZonedDateTime.from and in accordance with our general principle of validating arguments in order. This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Aug 16, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately.
ptomato
added a commit
that referenced
this pull request
Aug 16, 2023
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). In PlainDateTime.from, delay validation of the options until after validation of the ISO string, for consistency with ZonedDateTime.from and in accordance with our general principle of validating arguments in order. This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland.
ptomato
added a commit
that referenced
this pull request
Aug 16, 2023
…m,p.with} Also following the precedent set in #2447, this preserves consistency with analogous from/with operations in {Plain,Zoned}DateTime that need to read the overflow option twice in order to deal with time and date units separately.
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.
Instead, clone options in iteration order and read from the clone (slightly breaking the "read known properties in alphabetical order" goal in a subset of cases to fulfill the higher goals of consistency across types1 and robustness against malicious or broken user code).
Ref #2289 (comment)
If accepted, this will require changing some test262 files:
calendar-insensitive sequence
adds ownKeys and a read of each property
calendar-sensitive sequence
removes duplicate reads
Footnotes
Specifically, including {Instant,PlainTime}.{since,until} for consistency with the other types, even though they are not calendar-bearing and therefore options properties not read by GetDifferenceSettings are irrelevant. ↩