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

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

Merged
merged 8 commits into from
Oct 19, 2022
35 changes: 17 additions & 18 deletions polyfill/lib/calendar.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ const ArraySort = Array.prototype.sort;
const MathAbs = Math.abs;
const MathFloor = Math.floor;
const ObjectAssign = Object.assign;
const ObjectCreate = Object.create;
const ObjectEntries = Object.entries;
const ObjectKeys = Object.keys;

const impl = {};

Expand Down Expand Up @@ -265,23 +265,17 @@ impl['iso8601'] = {
return fields;
},
mergeFields(fields, additionalFields) {
fields = ES.ToObject(fields);
additionalFields = ES.ToObject(additionalFields);
const merged = {};
const keys = ObjectKeys(fields);
for (let index = 0; index < keys.length; index++) {
const nextKey = keys[index];
if (nextKey === 'month' || nextKey === 'monthCode') continue;
merged[nextKey] = fields[nextKey];
}
const newKeys = ObjectKeys(additionalFields);
for (let index = 0; index < newKeys.length; index++) {
const nextKey = newKeys[index];
merged[nextKey] = additionalFields[nextKey];
}
if (!ES.Call(ArrayIncludes, newKeys, ['month']) && !ES.Call(ArrayIncludes, newKeys, ['monthCode'])) {
const { month, monthCode } = fields;
if (month !== undefined) merged.month = month;
if (monthCode !== undefined) merged.monthCode = monthCode;
ES.CopyDataProperties(merged, fields, [], [undefined]);
const additionalFieldsCopy = ObjectCreate(null);
ES.CopyDataProperties(additionalFieldsCopy, additionalFields, [], [undefined]);
if ('month' in additionalFieldsCopy || 'monthCode' in additionalFieldsCopy) {
delete merged.month;
delete merged.monthCode;
}
ES.CopyDataProperties(merged, additionalFieldsCopy, []);
return merged;
},
dateAdd(date, years, months, weeks, days, overflow, calendar) {
Expand Down Expand Up @@ -1917,8 +1911,13 @@ const nonIsoGeneralImpl = {
return fields;
},
mergeFields(fields, additionalFields) {
const fieldsCopy = { ...fields };
const additionalFieldsCopy = { ...additionalFields };
fields = ES.ToObject(fields);
additionalFields = ES.ToObject(additionalFields);
const fieldsCopy = {};
ES.CopyDataProperties(fieldsCopy, fields, [], [undefined]);
const additionalFieldsCopy = {};
ES.CopyDataProperties(additionalFieldsCopy, additionalFields, [], [undefined]);

// era and eraYear are intentionally unused
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { month, monthCode, year, era, eraYear, ...original } = fieldsCopy;
Expand Down
81 changes: 72 additions & 9 deletions polyfill/lib/ecmascript.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,29 @@ const ObjectEntries = Object.entries;
const StringPrototypeSlice = String.prototype.slice;

import bigInt from 'big-integer';
import callBound from 'call-bind/callBound';
import Call from 'es-abstract/2022/Call.js';
import CreateDataPropertyOrThrow from 'es-abstract/2022/CreateDataPropertyOrThrow.js';
import Get from 'es-abstract/2022/Get.js';
import GetMethod from 'es-abstract/2022/GetMethod.js';
import IsArray from 'es-abstract/2022/IsArray.js';
import IsIntegralNumber from 'es-abstract/2022/IsIntegralNumber.js';
import ToIntegerOrInfinity from 'es-abstract/2022/ToIntegerOrInfinity.js';
import IsPropertyKey from 'es-abstract/2022/IsPropertyKey.js';
import SameValue from 'es-abstract/2022/SameValue.js';
import ToLength from 'es-abstract/2022/ToLength.js';
import ToNumber from 'es-abstract/2022/ToNumber.js';
import ToObject from 'es-abstract/2022/ToObject.js';
import ToPrimitive from 'es-abstract/2022/ToPrimitive.js';
import ToString from 'es-abstract/2022/ToString.js';
import Type from 'es-abstract/2022/Type.js';
import HasOwnProperty from 'es-abstract/2022/HasOwnProperty.js';

import every from 'es-abstract/helpers/every.js';
import forEach from 'es-abstract/helpers/forEach.js';
import OwnPropertyKeys from 'es-abstract/helpers/OwnPropertyKeys.js';
import some from 'es-abstract/helpers/some.js';

import { GetIntrinsic } from './intrinsicclass.mjs';
import {
CreateSlots,
Expand Down Expand Up @@ -67,6 +79,9 @@ import {
NANOSECONDS
} from './slots.mjs';

const $TypeError = GetIntrinsic('%TypeError%');
const $isEnumerable = callBound('Object.prototype.propertyIsEnumerable');

const DAY_SECONDS = 86400;
const DAY_NANOS = bigInt(DAY_SECONDS).multiply(1e9);
const NS_MIN = bigInt(-DAY_SECONDS).multiply(1e17);
Expand Down Expand Up @@ -205,6 +220,7 @@ const ES2022 = {
ToIntegerOrInfinity,
ToLength,
ToNumber,
ToObject,
ToPrimitive,
ToString,
Type
Expand Down Expand Up @@ -232,6 +248,48 @@ function getIntlDateTimeFormatEnUsForTimeZone(timeZoneIdentifier) {
}

export const ES = ObjectAssign({}, ES2022, {
// copied from es-abstract/2022/CopyDataProperties.js
// with modifications per Temporal spec/mainadditions.html
CopyDataProperties: (target, source, excludedKeys, excludedValues) => {
if (Type(target) !== 'Object') {
throw new $TypeError('Assertion failed: "target" must be an Object');
}

if (!IsArray(excludedKeys) || !every(excludedKeys, IsPropertyKey)) {
throw new $TypeError('Assertion failed: "excludedKeys" must be a List of Property Keys');
}

if (typeof source === 'undefined' || source === null) {
return target;
}

var from = ToObject(source);

var keys = OwnPropertyKeys(from);
forEach(keys, function (nextKey) {
var excluded = some(excludedKeys, function (e) {
return SameValue(e, nextKey) === true;
});

var enumerable =
$isEnumerable(from, nextKey) ||
// this is to handle string keys being non-enumerable in older engines
(typeof source === 'string' && nextKey >= 0 && IsIntegralNumber(ToNumber(nextKey)));
if (excluded === false && enumerable) {
var propValue = Get(from, nextKey);
if (excludedValues !== undefined) {
forEach(excludedValues, function (e) {
if (SameValue(e, propValue) === true) {
excluded = true;
}
});
}
if (excluded === false) CreateDataPropertyOrThrow(target, nextKey, propValue);
}
});

return target;
},
ToPositiveInteger: ToPositiveInteger,
ToIntegerThrowOnInfinity,
ToIntegerWithoutRounding,
Expand Down Expand Up @@ -931,10 +989,6 @@ export const ES = ObjectAssign({}, ES2022, {
if (UNITS_DESCENDING.indexOf(unit1) > UNITS_DESCENDING.indexOf(unit2)) return unit2;
return unit1;
},
MergeLargestUnitOption: (options, largestUnit) => {
if (options === undefined) options = ObjectCreate(null);
return ObjectAssign(ObjectCreate(null), options, { largestUnit });
},
PrepareTemporalFields: (
bag,
fields,
Expand Down Expand Up @@ -3364,7 +3418,9 @@ export const ES = ObjectAssign({}, ES2022, {
const date1 = ES.CreateTemporalDate(y1, mon1, d1, calendar);
const date2 = ES.CreateTemporalDate(y2, mon2, d2, calendar);
const dateLargestUnit = ES.LargerOfTwoTemporalUnits('day', largestUnit);
const untilOptions = ES.MergeLargestUnitOption(options, dateLargestUnit);
const untilOptions = ObjectCreate(null);
ES.CopyDataProperties(untilOptions, options, []);
untilOptions.largestUnit = dateLargestUnit;
let { years, months, weeks, days } = ES.CalendarDateUntil(calendar, date1, date2, untilOptions);
// Signs of date part and time part may not agree; balance them together
({ days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds } = ES.BalanceDuration(
Expand Down Expand Up @@ -3506,7 +3562,9 @@ export const ES = ObjectAssign({}, ES2022, {
if (operation === 'since') roundingMode = ES.NegateTemporalRoundingMode(roundingMode);
const roundingIncrement = ES.ToTemporalRoundingIncrement(options, undefined, false);

const untilOptions = ES.MergeLargestUnitOption(options, largestUnit);
const untilOptions = ObjectCreate(null);
ES.CopyDataProperties(untilOptions, options, []);
untilOptions.largestUnit = largestUnit;
let { years, months, weeks, days } = ES.CalendarDateUntil(calendar, plainDate, other, untilOptions);

if (smallestUnit !== 'day' || roundingIncrement !== 1) {
Expand Down Expand Up @@ -3739,7 +3797,9 @@ export const ES = ObjectAssign({}, ES2022, {
thisFields.day = 1;
const thisDate = ES.CalendarDateFromFields(calendar, thisFields);

const untilOptions = ES.MergeLargestUnitOption(options, largestUnit);
const untilOptions = ObjectCreate(null);
ES.CopyDataProperties(untilOptions, options, []);
untilOptions.largestUnit = largestUnit;
let { years, months } = ES.CalendarDateUntil(calendar, thisDate, otherDate, untilOptions);

if (smallestUnit !== 'month' || roundingIncrement !== 1) {
Expand Down Expand Up @@ -3811,7 +3871,9 @@ export const ES = ObjectAssign({}, ES2022, {
'or smaller because day lengths can vary between time zones due to DST or time zone offset changes.'
);
}
const untilOptions = ES.MergeLargestUnitOption(options, largestUnit);
const untilOptions = ObjectCreate(null);
ES.CopyDataProperties(untilOptions, options, []);
untilOptions.largestUnit = largestUnit;
({ years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds } =
ES.DifferenceZonedDateTime(ns1, ns2, timeZone, calendar, largestUnit, untilOptions));
({ years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds } =
Expand Down Expand Up @@ -4305,7 +4367,8 @@ export const ES = ObjectAssign({}, ES2022, {
const startDate = ES.CalendarDateFromFields(calendar, fields);
const Duration = GetIntrinsic('%Temporal.Duration%');
const durationToAdd = new Duration(years, months, weeks, days, 0, 0, 0, 0, 0, 0);
const optionsCopy = ObjectAssign(ObjectCreate(null), options);
const optionsCopy = ObjectCreate(null);
ES.CopyDataProperties(optionsCopy, options, []);
const addedDate = ES.CalendarDateAdd(calendar, startDate, durationToAdd, options);
const addedDateFields = ES.PrepareTemporalFields(addedDate, fieldNames, []);

Expand Down
2 changes: 2 additions & 0 deletions polyfill/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
],
"dependencies": {
"big-integer": "^1.6.51",
"call-bind": "^1.0.2",
"es-abstract": "^1.20.4"
},
"devDependencies": {
Expand All @@ -81,6 +82,7 @@
"js-yaml": "^4.1.0",
"progress": "^2.0.3",
"rollup": "^2.68.0",
"rollup-plugin-node-polyfills": "^0.2.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think call-bind was breaking without it.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

"rollup-plugin-terser": "^7.0.2",
"tiny-glob": "^0.2.9"
},
Expand Down
3 changes: 3 additions & 0 deletions polyfill/rollup.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import commonjs from '@rollup/plugin-commonjs';
import nodePolyfills from 'rollup-plugin-node-polyfills';
import resolve from '@rollup/plugin-node-resolve';
import babel from '@rollup/plugin-babel';
import replace from '@rollup/plugin-replace';
Expand Down Expand Up @@ -27,6 +28,7 @@ export default [
plugins: [
replace({ ...replaceConfig, __debug__: false, __isTest262__: isTest262 }),
commonjs(),
nodePolyfills(),
resolve(resolveConfig)
],
output: {
Expand All @@ -47,6 +49,7 @@ export default [
plugins: [
replace({ ...replaceConfig, __debug__: true, __isTest262__: false }),
commonjs(),
nodePolyfills(),
resolve(resolveConfig),
babel(babelConfig)
]
Expand Down
22 changes: 0 additions & 22 deletions spec/abstractops.html
Original file line number Diff line number Diff line change
Expand Up @@ -505,28 +505,6 @@ <h1>LargerOfTwoTemporalUnits ( _u1_, _u2_ )</h1>
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-mergelargestunitoption" type="abstract operation">
<h1>
MergeLargestUnitOption (
_options_: an Object,
_largestUnit_: a String,
): either a normal completion containing an Object, or an abrupt completion
</h1>
<dl class="header">
<dt>description</dt>
<dd>It returns a new null-prototype Object with enumerable own String properties copied from the _options_ Object, and the `largestUnit` property set to _largestUnit_.</dd>
</dl>
<emu-alg>
1. Let _merged_ be OrdinaryObjectCreate(*null*).
1. Let _keys_ be ? EnumerableOwnProperties(_options_, ~key~).
1. For each element _nextKey_ of _keys_, do
1. Let _propValue_ be ? Get(_options_, _nextKey_).
1. Perform ! CreateDataPropertyOrThrow(_merged_, _nextKey_, _propValue_).
1. Perform ! CreateDataPropertyOrThrow(_merged_, *"largestUnit"*, _largestUnit_).
1. Return _merged_.
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-maximumtemporaldurationroundingincrement" aoid="MaximumTemporalDurationRoundingIncrement">
<h1>MaximumTemporalDurationRoundingIncrement ( _unit_ )</h1>
<emu-alg>
Expand Down
26 changes: 8 additions & 18 deletions spec/calendar.html
Original file line number Diff line number Diff line change
Expand Up @@ -723,24 +723,14 @@ <h1>
</dl>
<emu-alg>
1. Let _merged_ be OrdinaryObjectCreate(%Object.prototype%).
1. Let _fieldsKeys_ be ? EnumerableOwnProperties(_fields_, ~key~).
1. For each element _key_ of _fieldsKeys_, do
1. If _key_ is not *"month"* or *"monthCode"*, then
1. Let _propValue_ be ? Get(_fields_, _key_).
1. If _propValue_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_merged_, _key_, _propValue_).
1. Let _additionalFieldsKeys_ be ? EnumerableOwnProperties(_additionalFields_, ~key~).
1. For each element _key_ of _additionalFieldsKeys_, do
1. Let _propValue_ be ? Get(_additionalFields_, _key_).
1. If _propValue_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_merged_, _key_, _propValue_).
1. If _additionalFieldsKeys_ does not contain either *"month"* or *"monthCode"*, then
1. Let _month_ be ? Get(_fields_, *"month"*).
1. If _month_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_merged_, *"month"*, _month_).
1. Let _monthCode_ be ? Get(_fields_, *"monthCode"*).
1. If _monthCode_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_merged_, *"monthCode"*, _monthCode_).
1. Perform ? CopyDataProperties(_merged_, _fields_, « », « *undefined* »).
1. Let _additionalFieldsCopy_ be OrdinaryObjectCreate(*null*).
1. Perform ? CopyDataProperties(_additionalFieldsCopy_, _additionalFields_, « », « *undefined* »).
1. NOTE: Every property of _additionalFieldsCopy_ is a data property with non-*undefined* value, but some property keys may be Symbols.
ljharb marked this conversation as resolved.
Show resolved Hide resolved
1. If ! _additionalFieldsCopy_.[[OwnPropertyKeys]]() contains *"month"* or *"monthCode"*, then
1. Perform ! DeletePropertyOrThrow(_merged_, *"month"*).
1. Perform ! DeletePropertyOrThrow(_merged_, *"monthCode"*).
1. Perform ? CopyDataProperties(_merged_, _additionalFieldsCopy_, « »).
1. Return _merged_.
</emu-alg>
</emu-clause>
Expand Down
12 changes: 2 additions & 10 deletions spec/intl.html
Original file line number Diff line number Diff line change
Expand Up @@ -2076,17 +2076,9 @@ <h1>Temporal.Calendar.prototype.mergeFields ( _fields_, _additionalFields_ )</h1
1. If _calendar_.[[Identifier]] is *"iso8601"*, then
1. Return ? DefaultMergeCalendarFields(_fields_, _additionalFields_).
1. Let _fieldsCopy_ be OrdinaryObjectCreate(%Object.prototype%).
1. Let _fieldsKeys_ be ? EnumerableOwnProperties(_fields_, ~key~).
1. For each element _key_ of _fieldsKeys_, do
1. Let _propValue_ be ? Get(_fields_, _key_).
1. If _propValue_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_fieldsCopy_, _key_, _propValue_).
1. Perform ? CopyDataProperties(_fieldsCopy_, _fields_, « », « *undefined* »).
1. Let _additionalFieldsCopy_ be OrdinaryObjectCreate(%Object.prototype%).
1. Let _additionalFieldsKeys_ be ? EnumerableOwnProperties(_additionalFields_, ~key~).
1. For each element _key_ of _additionalFieldsKeys_, do
1. Let _propValue_ be ? Get(_additionalFields_, _key_).
1. If _propValue_ is not *undefined*, then
1. Perform ! CreateDataPropertyOrThrow(_additionalFieldsCopy_, _key_, _propValue_).
1. Perform ? CopyDataProperties(_additionalFieldsCopy_, _additionalFields_, « », « *undefined* »).
1. Return CalendarDateMergeFields(_calendar_.[[Identifier]], _fieldsCopy_, _additionalFieldsCopy_).
</emu-alg>
</emu-clause>
Expand Down
37 changes: 37 additions & 0 deletions spec/mainadditions.html
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,43 @@ <h1>Mathematical Operations</h1>
<p>[...]</p>
</emu-clause>

<emu-clause id="sec-copydataproperties" type="abstract operation">
<h1>
CopyDataProperties (
_target_: an Object,
_source_: an ECMAScript language value,
<del>_excludedItems_: a List of property keys,</del>
<ins>_excludedKeys_: a List of property keys,</ins>
<ins>optional _excludedValues_: a List of ECMAScript language values,</ins>
): either a normal completion containing ~unused~ or a throw completion
</h1>
<dl class="header">
</dl>
<emu-alg>
1. If _source_ is *undefined* or *null*, return ~unused~.
1. Let _from_ be ! ToObject(_source_).
1. Let _keys_ be ? <emu-meta effects="user-code">_from_.[[OwnPropertyKeys]]</emu-meta>().
1. For each element _nextKey_ of _keys_, do
1. Let _excluded_ be *false*.
1. For each element _e_ of <del>_excludedItems_</del><ins>_excludedKeys_</ins>, do
1. If SameValue(_e_, _nextKey_) is *true*, then
1. Set _excluded_ to *true*.
1. If _excluded_ is *false*, then
1. Let _desc_ be ? <emu-meta effects="user-code">_from_.[[GetOwnProperty]]</emu-meta>(_nextKey_).
1. If _desc_ is not *undefined* and _desc_.[[Enumerable]] is *true*, then
1. Let _propValue_ be ? Get(_from_, _nextKey_).
1. <ins>If _excludedValues_ is present, then</ins>
1. <ins>For each element _e_ of _excludedValues_, do</ins>
1. <ins>If SameValue(_e_, _propValue_) is *true*, then</ins>
1. <ins>Set _excluded_ to *true*.</ins>
1. <del>Perform</del><ins>If _excluded_ is *false*, perform</ins> ! CreateDataPropertyOrThrow(_target_, _nextKey_, _propValue_).
1. Return ~unused~.
</emu-alg>
<emu-note>
<p>The target passed in here is always a newly created object which is not directly accessible in case of an error being thrown.</p>
</emu-note>
</emu-clause>

<emu-clause id="sec-temporal-properties-of-the-legacy-date-prototype-object">
<h1><a href="https://tc39.es/ecma262/#sec-properties-of-the-date-prototype-object">Properties of the Date Prototype Object</a></h1>

Expand Down
4 changes: 3 additions & 1 deletion spec/plaindate.html
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,9 @@ <h1>
1. Set _other_ to ? ToTemporalDate(_other_).
1. If ? CalendarEquals(_temporalDate_.[[Calendar]], _other_.[[Calendar]]) is *false*, throw a *RangeError* exception.
1. Let _settings_ be ? GetDifferenceSettings(_operation_, _options_, ~date~, &laquo; &raquo;, *"day"*, *"day"*).
1. Let _untilOptions_ be ? MergeLargestUnitOption(_settings_.[[Options]], _settings_.[[LargestUnit]]).
1. Let _untilOptions_ be OrdinaryObjectCreate(*null*).
1. Perform ? CopyDataProperties(_untilOptions_, _settings_.[[Options]], « »).
1. Perform ! CreateDataPropertyOrThrow(_untilOptions_, *"largestUnit"*, _settings_.[[LargestUnit]]).
1. Let _result_ be ? CalendarDateUntil(_temporalDate_.[[Calendar]], _temporalDate_, _other_, _untilOptions_).
1. If _settings_.[[SmallestUnit]] is not *"day"* or _settings_.[[RoundingIncrement]] &ne; 1, then
1. Set _result_ to (? RoundDuration(_result_.[[Years]], _result_.[[Months]], _result_.[[Weeks]], _result_.[[Days]], 0, 0, 0, 0, 0, 0, _settings_.[[RoundingIncrement]], _settings_.[[SmallestUnit]], _settings_.[[RoundingMode]], _temporalDate_)).[[DurationRecord]].
Expand Down
Loading