202139 2019-09-24 02:45:32 -0700 Object spread ({ ... } syntax): object key order is modified 2020-08-30 04:27:42 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other Mac macOS 10.14 RESOLVED DUPLICATE 142933 InRadar P2 Normal --- 1 dmnsgn webkit-unassigned ashvayka bakkot fpizlo ross.kirsling ticaiolima webkit-bug-importer ysuzuki oldest_to_newest 1573615 0 dmnsgn 2019-09-24 02:45:32 -0700 Version: Safari 13 Using Object assign as follow: Object.assign({ a: 1, b: 100, c: 3 }, { b: 2 }) the output will be: {a: 1, b: 2, c: 3} where the object key order is a,b,c. However, using Object spread syntax as follow: { ...{ a: 1, b: 100, c: 3 }, ...{ b: 2 } } the output will be: {a: 1, c: 3, b: 2} where the object key order is a,c,b. This behaviour is different from Chrome where key order is preserved and I guess the underlying question here is: is this following ECMA specification? I thought the spec was clear since ES2015. See the following jsfiddle output to compare: https://jsfiddle.net/dmnsgn/r1n8q6o2/ Thanks 1573925 1 ross.kirsling 2019-09-24 18:41:36 -0700 Spec-wise, it apparently all depends on *how* you get the keys out of the object. Object.{keys, values, entries} and JSON.stringify apparently use EnumerableOwnPropertyNames which ultimately states "[t]he mechanics and order of enumerating the properties is not specified". (https://tc39.es/ecma262/#sec-enumerableownpropertynames) That said, Reflect.ownKeys uses OrdinaryOwnPropertyKeys which states that string keys should be in "ascending chronological order of property creation". (https://tc39.es/ecma262/#sec-ordinaryownpropertykeys) So we definitely have a bug with the latter: Reflect.ownKeys(Object.assign({}, { a: 1, b: 100, c: 3 }, { b: 3 })) > a,b,c Reflect.ownKeys({ ...{ a: 1, b: 100, c: 3 }, ...{ b: 3 } }) > a,c,b And fixing that may result in a "fix" for the behavior you mentioned too. This would certainly be desirable, because there is in fact a proposal aiming to (partially) specify the order of EnumerableOwnPropertyNames which is about to reach stage 3 (https://github.com/tc39/proposal-for-in-order/). 1573943 2 bakkot 2019-09-24 21:36:41 -0700 Poking around some, it looks like the bug is actually in defineProperty, as reproduced by Reflect.ownKeys(Object.defineProperty({ a: 0, b: 0 }, 'a', { value: 1, enumerable: true, configurable: true, writable: true })) which outputs ["b", "a"] which is backwards from what the spec requires. Contrast the essentially equivalent x = { a: 0, b: 0 }; x.a = 1; Reflect.ownKeys(x) which outputs ["a", b"] as it should. --- Tracking this down, ObjectSpreadExpression is implemented in terms of copyDataPropertiesNoExclusionsPrivateName https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L4745-L4751 which is implemented in terms of defineEnumerableWritableConfigurableDataProperty https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/builtins/GlobalOperations.js#L114-L137 which is implemented in terms of emitCallDefineProperty https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L1399-L1411 which is implemented in terms of OpDefineDataProperty https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp#L3443 which I _think_ (it is hard to follow the macros) is implemented in terms of JSObject::defineOwnProperty https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/dfg/DFGOperations.cpp#L1627-L1647 which is implemented in terms of defineOwnNonIndexProperty, which is implemented in terms of validateAndApplyPropertyDescriptor https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/runtime/JSObject.cpp#L3718-L3749 which (as per the note in the previous code block) performs a delete when updating an existing property https://github.com/WebKit/webkit/blob/2d7b35108aa7c8cd1c4ee0f4f6070206a55d5ad6/Source/JavaScriptCore/runtime/JSObject.cpp#L3643-L3658 which is the bug: it changes the order in which properties appear to have been added to the object, which is observable with Reflect.ownKeys. I don't know why it does that delete. 1573954 3 bakkot 2019-09-24 23:03:39 -0700 Unfortunately, just deleting the relevant object->methodTable(vm)->deleteProperty(object, exec, propertyName) line in validateAndApplyPropertyDescriptor causes Object.getOwnPropertyDescriptor(Object.defineProperty({ a: 0 }, 'a', { value: 1, enumerable: false }), 'a').enumerable to start returning `true` instead of `false`, so some more complex solution will be necessary to allow updating the attributes of an existing property without first deleting it. --- Also, note that any fix to this will require making all of Reflect.ownKeys(Object.defineProperty({ a: 0, b: 0 }, 'a', { enumerable: false })) Reflect.ownKeys(Object.defineProperty({ a: 0, b: 0 }, 'a', { get: () => 0 })) Reflect.ownKeys(Object.defineProperty(Object.create(null, { a: { get: () => {}, configurable: true }, b: { get: () => {}, configurable: true } }), 'a', { value: 1 })) Reflect.ownKeys(Object.defineProperty(Object.create(null, { a: { get: () => {}, configurable: true }, b: { get: () => {}, configurable: true } }), 'a', { get: () => {} })) return `["a","b"]` as well. (There are four distinct deleteProperty calls in validateAndApplyPropertyDescriptor, each of which needs to be avoided to preserve property enumeration order.) 1574179 4 ross.kirsling 2019-09-25 13:08:41 -0700 Looks like the deletes were called out as a place for future improvement when Object.defineProperty was first implemented in r48542 (at which time this behavior wouldn't've been a bug). 1574256 5 webkit-bug-importer 2019-09-25 15:42:18 -0700 <rdar://problem/55721999> 1684011 6 ashvayka 2020-08-30 04:27:42 -0700 (In reply to Kevin Gibbons from comment #2) > which is the bug: it changes the order in which properties appear to have > been added to the object, which is observable with Reflect.ownKeys. I don't > know why it does that delete. Thank you Kevin for super-detailed investigation! r264574 replaces deleteProperty() calls in validateAndApplyPropertyDescriptor() with attribute-change transition, fixing the object spread order and the following test cases: (In reply to Kevin Gibbons from comment #3) > Also, note that any fix to this will require making all of > > Reflect.ownKeys(Object.defineProperty({ a: 0, b: 0 }, 'a', { enumerable: > false })) > Reflect.ownKeys(Object.defineProperty({ a: 0, b: 0 }, 'a', { get: () => 0 })) > Reflect.ownKeys(Object.defineProperty(Object.create(null, { a: { get: () => > {}, configurable: true }, b: { get: () => {}, configurable: true } }), 'a', > { value: 1 })) > Reflect.ownKeys(Object.defineProperty(Object.create(null, { a: { get: () => > {}, configurable: true }, b: { get: () => {}, configurable: true } }), 'a', > { get: () => {} })) > > return `["a","b"]` as well. (There are four distinct deleteProperty calls in validateAndApplyPropertyDescriptor, each of which needs to be avoided to preserve property enumeration order.) Test262 coverage: https://github.com/tc39/test262/pull/2516. *** This bug has been marked as a duplicate of bug 142933 ***