Bug 202139 – Object spread ({ ... } syntax): object key order is modified

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Damien Seguin]

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

[Ross Kirsling]

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

[Kevin Gibbons]

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.

[Kevin Gibbons]

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

[Ross Kirsling]

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

[Radar WebKit Bug Importer]

<rdar://problem/55721999>

[Alexey Shvayka]

(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 ***