Normative: Always use CopyDataProperties for object copy/merge #2245

gibson042 · 2022-05-31T04:09:37Z

The first three commits are editorial cleanup, while the rest include technically normative changes (albeit minor).

This stemmed from the surprising observation that AddDurationToOrSubtractDurationFromPlainYearMonth uses key+value enumeration to clone options, while MergeLargestUnitOption and ECMA-402 Temporal.Calendar.prototype.mergeFields and DefaultMergeFields use key enumeration followed by a for-each loop that Gets each source property in analogous copy/merge steps (and DefaultMergeFields in particular has further out-of-order steps related to overriding "month" and "monthCode" properties). The difference between the two is observable by ECMAScript code.

This seems unnecessarily convoluted and confusing, and very likely to result in unintentional implementation divergence. It would be simpler and more comprehensible to always read all properties and all values from source objects in any copy/merge operation, which is what this PR proposes. But because that order is observable (key enumeration followed by the for-each Get calls is a sequence of [[GetOwnProperty]]s followed by a sequence of [[Get]]s, while key+value enumeration is a sequence of [[GetOwnProperty]] + [[Get]] pairs) and so is inclusion of Symbol-valued keys, the change is normative, even though it will not affect expected normal code. That is also true even of the slightly more substantial changes in DefaultMergeFields, and actually improves the behavior AFAICT—Temporal.Calendar.from("iso8601").mergeFields({year: 2022, month: 5, day: 31}, {year: undefined, month: undefined, day: undefined}) will now produce {year: 2022, month: 5, day: 31} rather than {year: 2022, day: 31} (which is missing "month" because per the current algorithm its undefined value in additionalFields suppresses copy from there, while its mere presence suppresses copy from fields).

codecov · 2022-05-31T04:10:36Z

Codecov Report

Merging #2245 (8657e06) into main (056d45b) will decrease coverage by 0.06%.
The diff coverage is 85.86%.

❗ Current head 8657e06 differs from pull request most recent head 229adc6. Consider uploading reports for the commit 229adc6 to get more accurate results

@@            Coverage Diff             @@
##             main    #2245      +/-   ##
==========================================
- Coverage   94.75%   94.69%   -0.07%     
==========================================
  Files          20       20              
  Lines       10991    11056      +65     
  Branches     1962     1965       +3     
==========================================
+ Hits        10415    10469      +54     
- Misses        531      542      +11     
  Partials       45       45

Impacted Files	Coverage Δ
polyfill/lib/ecmascript.mjs	`98.43% <82.66%> (-0.23%)`	⬇️
polyfill/lib/calendar.mjs	`81.42% <100.00%> (-0.01%)`	⬇️
polyfill/lib/intrinsicclass.mjs	`90.54% <0.00%> (+1.35%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

spec/calendar.html

ptomato

Thanks for the thorough work, no bugs found in any case! However, I'm lukewarm on this normative change, personally... I'm certainly not opposed to it but I'm wondering if there is a particular motivation we can give to the plenary when we present it? I get the appeal of reusing CopyDataProperties, but I don't get supporting symbol keys on an object for which Temporal never cares about any symbol keys; maybe I'm missing something. (For example, I think this way is just as likely to lead to implementation divergence if implementations are using built-in JS, because the temptation to use the now-incorrect spread syntax seems likely.)

The editorial changes are all fine in my opinion. Would you mind splitting them out into a separate PR, so that the PR we present in July can be as small as possible? I can merge the editorial PR if you like, or you can hereby consider it already reviewed.

ptomato · 2022-06-12T13:27:10Z

polyfill/package.json

@@ -83,6 +83,7 @@
    "full-icu": "^1.4.0",
    "nyc": "^15.1.0",
    "rollup": "^2.68.0",
+    "rollup-plugin-node-polyfills": "^0.2.1",


What is the plugin needed for? Does CopyDataProperties depend on it?

I think call-bind was breaking without it.

oof, does rollup break things just like webpack 5, by not automatically polyfilling node builtins?

Yeah, that seems about right. You should be able to observe the problem if you checkout this PR, remove the rollup-plugin-node-polyfills dependency, and run npm install && npm run build (maybe followed by npm test && npm run test262; I don't remember if the issue manifested at build time).

ptomato · 2022-06-12T13:30:29Z

polyfill/lib/ecmascript.mjs

+      });
+
+      var enumerable = $isEnumerable(from, nextKey) || (
+      // this is to handle string keys being non-enumerable in older engines


I'm not overly concerned about handling this case in the online playground... we already require that the browser supports WeakMap, for example. I feel we can leave this support to other polyfills more intended for that use case in production.

Agreed, but it was copied from es-abstract/2020/CopyDataProperties.js and is unrelated to the CopyDataProperties changes being introduced. I've opened tc39/ecma262#2796 with the hopes of making this override unnecessary.

gibson042 · 2022-06-13T18:08:01Z

The editorial changes are all fine in my opinion. Would you mind splitting them out into a separate PR, so that the PR we present in July can be as small as possible? I can merge the editorial PR if you like, or you can hereby consider it already reviewed.

Done: #2295 (please review and/or merge because I can't do so unilaterally).

Thanks for the thorough work, no bugs found in any case! However, I'm lukewarm on this normative change, personally... I'm certainly not opposed to it but I'm wondering if there is a particular motivation we can give to the plenary when we present it? I get the appeal of reusing CopyDataProperties, but I don't get supporting symbol keys on an object for which Temporal never cares about any symbol keys; maybe I'm missing something. (For example, I think this way is just as likely to lead to implementation divergence if implementations are using built-in JS, because the temptation to use the now-incorrect spread syntax seems likely.)

The main goal is maximum consistency in how object-copy works, within ECMA-262 and especially within Temporal. I prefer using the existing CopyDataProperties operation to avoid further debate, but if there's strong opposition then I would be willing to compromise on excluding symbol-keyed properties—however, it's worth noting that object spread and rest include them¹ and that custom calendars and/or time zones might benefit from their preservation just as they benefit from preservation of string-keyed properties that are not used by any built-in operation.

Object spread and rest are in fact the current use of CopyDataProperties, cf. PropertyDefinitionEvaluation and RestDestructuringAssignmentEvaluation and RestBindingInitialization and observe that e.g. Reflect.ownKeys({ ...{ [Symbol("foo")]: true } }) is not empty. ↩

ptomato

OK, let's plan to present this in July, then. I'll mark it as draft awaiting that.

I think it will need another rebase though.

gibson042 · 2022-07-01T16:06:25Z

I think it will need another rebase though.

Done.

Also, I am concerned about cases in which options-object properties are read multiple times, which happens when CopyDataProperties is preceded by GetDifferenceSettings (as in Difference operations) and in AddDurationToOrSubtractDurationFromPlainYearMonth (where the original object is sent to CalendarDateAdd but a null-object copy is sent to CalendarYearMonthFromFields). But I'm not sure how to address that, since Get can look into the prototype chain while CopyDataProperties does not (making the difference observable when a relevant property exists but not on the object itself).

…t copy/merge

…erties This is normative because the latter includes Symbol-keyed properties while the former does not.

This tests some of the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to places where the MergeLargestUnitOperation was called. Due to the use of the pre-existing spec operation CopyDataProperties, the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs. This previously wasn't tested because TemporalHelpers.propertyBagObserver didn't track [[GetOwnProperty]] operations, but now it does.

This tests the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to the Temporal.Calendar.prototype.mergeFields method. Due to the use of the pre-existing spec operation CopyDataProperties, now symbol keys are merged into the return value, and the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs.

This tests some of the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to places where the MergeLargestUnitOperation was called. Due to the use of the pre-existing spec operation CopyDataProperties, the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs. This previously wasn't tested because TemporalHelpers.propertyBagObserver didn't track [[GetOwnProperty]] operations, but now it does.

This tests the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to the Temporal.Calendar.prototype.mergeFields method. Due to the use of the pre-existing spec operation CopyDataProperties, now symbol keys are merged into the return value, and the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs.

ptomato · 2022-10-19T00:43:55Z

I've rebased this and written tests for it, which are in tc39/test262#3691. Since it reached consensus at the July 2022 TC39 meeting and doesn't invalidate any current tests, I'm going to go ahead and merge it now.

This tests some of the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to places where the MergeLargestUnitOperation was called. Due to the use of the pre-existing spec operation CopyDataProperties, the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs. This previously wasn't tested because TemporalHelpers.propertyBagObserver didn't track [[GetOwnProperty]] operations, but now it does.

This tests the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to the Temporal.Calendar.prototype.mergeFields method. Due to the use of the pre-existing spec operation CopyDataProperties, now symbol keys are merged into the return value, and the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs.

This tests some of the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to places where the MergeLargestUnitOperation was called. Due to the use of the pre-existing spec operation CopyDataProperties, the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs. This previously wasn't tested because TemporalHelpers.propertyBagObserver didn't track [[GetOwnProperty]] operations, but now it does.

This tests the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to the Temporal.Calendar.prototype.mergeFields method. Due to the use of the pre-existing spec operation CopyDataProperties, now symbol keys are merged into the return value, and the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs.

This tests some of the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to places where the MergeLargestUnitOperation was called. Due to the use of the pre-existing spec operation CopyDataProperties, the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs. This previously wasn't tested because TemporalHelpers.propertyBagObserver didn't track [[GetOwnProperty]] operations, but now it does.

This tests the normative changes in tc39/proposal-temporal#2245, which achieved consensus in the July 2022 TC39 meeting, specifically as they apply to the Temporal.Calendar.prototype.mergeFields method. Due to the use of the pre-existing spec operation CopyDataProperties, now symbol keys are merged into the return value, and the order of observable property operations has changed from a batch of [[GetOwnProperty]] followed by a batch of [[Get]], to a series of interleaved [[GetOwnProperty]]/[[Get]] pairs.

justingrant · 2023-04-16T00:21:46Z

<rant>

Remember to clean up and/or squash commits before merging! There's several extra commits in this PR that ideally wouldn't have been in main. Other than the inherent value of keeping the repo tidy, this PR contains commits that make a change and revert the change later in the same PR. Porting these commits has been painful. :-(

git commit --fixup FTW!

</rant>

gibson042 added behavior Relating to behavior defined in the proposal spec-text Specification text involved normative Would be a normative change to the proposal labels May 31, 2022

gibson042 requested review from ptomato and ryzokuken May 31, 2022 04:09

ljharb reviewed Jun 6, 2022

View reviewed changes

spec/calendar.html Show resolved Hide resolved

spec/calendar.html Outdated Show resolved Hide resolved

gibson042 changed the title ~~Normative: Always use key+value source property enumeration for object copy/merge~~ Normative: Always use CopyDataProperties for object copy/merge Jun 6, 2022

gibson042 force-pushed the 2022-05-key+value-property-enumeration branch from 63180c3 to c7993f1 Compare June 8, 2022 17:47

ptomato reviewed Jun 12, 2022

View reviewed changes

This was referenced Jun 13, 2022

Editorial: Use consistent invocations of Difference{Zoned,ISO}DateTime and alias names for object-copy steps #2295

Merged

Editorial: Extend CopyDataProperties to support excluding properties by value tc39/ecma262#2796

Closed

gibson042 force-pushed the 2022-05-key+value-property-enumeration branch 2 times, most recently from da0596e to 7690d06 Compare June 14, 2022 14:49

gibson042 mentioned this pull request Jun 16, 2022

Change the order of Table 8: Duration Record Fields #2286

Closed

ptomato approved these changes Jun 22, 2022

View reviewed changes

ptomato marked this pull request as draft June 22, 2022 16:49

gibson042 force-pushed the 2022-05-key+value-property-enumeration branch from 7690d06 to 41296b6 Compare July 1, 2022 15:29

ptomato added this to the Next batch of normative changes milestone Jul 4, 2022

ptomato mentioned this pull request Sep 15, 2022

Ordering of observable property accesses should be enforced rather than implicit #2254

Closed

gibson042 added 4 commits October 18, 2022 16:51

Normative: Always use key+value source property enumeration for objec…

6d01faf

…t copy/merge

Editorial: Replace MergeLargestUnitOption with CopyDataProperties

a86efec

Normative: Replace EnumerableOwnPropertyNames loops with CopyDataProp…

2e5d63f

…erties This is normative because the latter includes Symbol-keyed properties while the former does not.

Polyfill: Remove MergeLargestUnitOption

c687e31

ptomato mentioned this pull request Oct 19, 2022

Temporal: Test use of CopyDataProperties tc39/test262#3691

Merged

ptomato force-pushed the 2022-05-key+value-property-enumeration branch from 41296b6 to 8657e06 Compare October 19, 2022 00:27

ptomato marked this pull request as ready for review October 19, 2022 00:27

gibson042 added 4 commits October 18, 2022 17:39

Polyfill: Replace manual property enumeration with CopyDataProperties

1cc9908

Appease eslint

d0537f4

Editorial: Remove trailing whitespace

61a4d69

Normative: Restore use of null-prototype objects

229adc6

ptomato force-pushed the 2022-05-key+value-property-enumeration branch from 8657e06 to 229adc6 Compare October 19, 2022 00:40

ptomato merged commit af8c764 into tc39:main Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Always use CopyDataProperties for object copy/merge #2245

Normative: Always use CopyDataProperties for object copy/merge #2245

gibson042 commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading

ptomato left a comment •

edited

Loading

ptomato Jun 12, 2022

gibson042 Jun 13, 2022

ljharb Jun 13, 2022

gibson042 Jun 13, 2022

ptomato Jun 12, 2022

gibson042 Jun 13, 2022

gibson042 commented Jun 13, 2022

ptomato left a comment

gibson042 commented Jul 1, 2022

ptomato commented Oct 19, 2022

justingrant commented Apr 16, 2023

Normative: Always use CopyDataProperties for object copy/merge #2245

Normative: Always use CopyDataProperties for object copy/merge #2245

Conversation

gibson042 commented May 31, 2022 • edited Loading

codecov bot commented May 31, 2022 • edited Loading

Codecov Report

ptomato left a comment • edited Loading

Choose a reason for hiding this comment

ptomato Jun 12, 2022

Choose a reason for hiding this comment

gibson042 Jun 13, 2022

Choose a reason for hiding this comment

ljharb Jun 13, 2022

Choose a reason for hiding this comment

gibson042 Jun 13, 2022

Choose a reason for hiding this comment

ptomato Jun 12, 2022

Choose a reason for hiding this comment

gibson042 Jun 13, 2022

Choose a reason for hiding this comment

gibson042 commented Jun 13, 2022

Footnotes

ptomato left a comment

Choose a reason for hiding this comment

gibson042 commented Jul 1, 2022

ptomato commented Oct 19, 2022

justingrant commented Apr 16, 2023

gibson042 commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading

ptomato left a comment •

edited

Loading