From c114be95f512ae2a082d02775c7d2176d058d409 Mon Sep 17 00:00:00 2001
From: davidot <davidot@serenityos.org>
Date: Sun, 13 Feb 2022 02:04:37 +0100
Subject: [PATCH] LibJS: Use CopyDataProperties when spreading in object
 expressions

Before this was a mix of different strategies but copy_data_properties
does all of that in a spec way.
This fixes numeric properties in object spreading. And ensures that any
new properties added during spreading are not taken into account.
---
 Userland/Libraries/LibJS/AST.cpp              | 24 +-------
 .../Libraries/LibJS/Tests/object-spread.js    | 57 +++++++++++++++++++
 2 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp
index eca2383e6f..519e2d195f 100644
--- a/Userland/Libraries/LibJS/AST.cpp
+++ b/Userland/Libraries/LibJS/AST.cpp
@@ -2987,28 +2987,10 @@ Completion ObjectExpression::execute(Interpreter& interpreter, GlobalObject& glo
     for (auto& property : m_properties) {
         auto key = TRY(property.key().execute(interpreter, global_object)).release_value();
 
+        // PropertyDefinition : ... AssignmentExpression
         if (property.type() == ObjectProperty::Type::Spread) {
-            if (key.is_object() && is<Array>(key.as_object())) {
-                auto& array_to_spread = static_cast<Array&>(key.as_object());
-                for (auto& entry : array_to_spread.indexed_properties()) {
-                    auto value = TRY(array_to_spread.get(entry.index()));
-                    object->indexed_properties().put(entry.index(), value);
-                }
-            } else if (key.is_object()) {
-                auto& obj_to_spread = key.as_object();
-
-                for (auto& it : obj_to_spread.shape().property_table_ordered()) {
-                    if (it.value.attributes.is_enumerable()) {
-                        auto value = TRY(obj_to_spread.get(it.key));
-                        object->define_direct_property(it.key, value, JS::default_attributes);
-                    }
-                }
-            } else if (key.is_string()) {
-                auto& str_to_spread = key.as_string().string();
-
-                for (size_t i = 0; i < str_to_spread.length(); i++)
-                    object->define_direct_property(i, js_string(interpreter.heap(), str_to_spread.substring(i, 1)), JS::default_attributes);
-            }
+            // 4. Return ? CopyDataProperties(object, fromValue, excludedNames).
+            TRY(object->copy_data_properties(key, {}, global_object));
             continue;
         }
 
diff --git a/Userland/Libraries/LibJS/Tests/object-spread.js b/Userland/Libraries/LibJS/Tests/object-spread.js
index 93008a8352..2b5a501858 100644
--- a/Userland/Libraries/LibJS/Tests/object-spread.js
+++ b/Userland/Libraries/LibJS/Tests/object-spread.js
@@ -115,3 +115,60 @@ test("respects custom Symbol.iterator method", () => {
     let a = [...o];
     expect(a).toEqual([1, 2, 3]);
 });
+
+test("object with numeric indices", () => {
+    const obj = { 0: 0, 1: 1, foo: "bar" };
+    const result = { ...obj };
+    expect(result).toHaveProperty("0", 0);
+    expect(result).toHaveProperty("1", 1);
+    expect(result).toHaveProperty("foo", "bar");
+});
+
+describe("modification of spreadable objects during spread", () => {
+    test("spreading object", () => {
+        const object = {
+            0: 0,
+            2: 2,
+            9999: 9999,
+            bar: 44,
+            get 3() {
+                object[4] = 4;
+                object[5000] = 5000;
+                return 3;
+            },
+        };
+
+        const result = { ...object };
+        expect(Object.getOwnPropertyNames(result)).toHaveLength(5);
+        expect(Object.getOwnPropertyNames(result)).not.toContain("4");
+        expect(Object.getOwnPropertyNames(result)).toContain("bar");
+    });
+
+    test("spreading array", () => {
+        const array = [0];
+        array[2] = 2;
+        array[999] = 999;
+        Object.defineProperty(array, 3, {
+            get() {
+                array[4] = 4;
+                array[1000] = 1000;
+                return 3;
+            },
+            enumerable: true,
+        });
+
+        const objectResult = { ...array };
+        expect(Object.getOwnPropertyNames(objectResult)).toHaveLength(4);
+        expect(Object.getOwnPropertyNames(objectResult)).not.toContain("4");
+
+        const arrayResult = [...array];
+        expect(arrayResult).toHaveLength(1001);
+        expect(arrayResult).toHaveProperty("0", 0);
+        expect(arrayResult).toHaveProperty("2", 2);
+        expect(arrayResult).toHaveProperty("3", 3);
+        // Yes the in flight added items need to be here in this case! (since it uses an iterator)
+        expect(arrayResult).toHaveProperty("4", 4);
+        expect(arrayResult).toHaveProperty("999", 999);
+        expect(arrayResult).toHaveProperty("1000", 1000);
+    });
+});