From 630f046e95a91254d44f6414e6ba9356ff69f746 Mon Sep 17 00:00:00 2001
From: Shannon Booth <shannon@serenityos.org>
Date: Mon, 28 Aug 2023 20:13:01 +1200
Subject: [PATCH] LibJS: Apply normative change to Array.fromAsync to avoid
 double construct

Apply a normative fix to Array.fromAsync fixed in:

https://github.com/tc39/proposal-array-from-async/pull/41

This avoids a double construction when Array.fromAsync is given an array
like object.
---
 .../LibJS/Runtime/ArrayConstructor.cpp        | 40 ++++++++++---------
 .../Tests/builtins/Array/Array.fromAsync.js   | 24 ++++++++++-
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp b/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
index 688b3f4f674589c..c86c65c43eccdee 100644
--- a/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
+++ b/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
@@ -335,39 +335,39 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayConstructor::from_async)
             using_sync_iterator = TRY(async_items.get_method(vm, vm.well_known_symbol_iterator()));
         }
 
-        GCPtr<Object> array;
-
-        // e. If IsConstructor(C) is true, then
-        if (constructor.is_constructor()) {
-            // i. Let A be ? Construct(C).
-            array = TRY(JS::construct(vm, constructor.as_function()));
-        }
-        // f. Else,
-        else {
-            // i. Let A be ! ArrayCreate(0).
-            array = MUST(Array::create(realm, 0));
-        }
-
-        // g. Let iteratorRecord be undefined.
+        // e. Let iteratorRecord be undefined.
         Optional<IteratorRecord> iterator_record;
 
-        // h. If usingAsyncIterator is not undefined, then
+        // f. If usingAsyncIterator is not undefined, then
         if (using_async_iterator) {
             // i. Set iteratorRecord to ? GetIterator(asyncItems, async, usingAsyncIterator).
             // FIXME: The Array.from proposal is out of date - it should be using GetIteratorFromMethod.
             iterator_record = TRY(get_iterator_from_method(vm, async_items, *using_async_iterator));
         }
-        // i. Else if usingSyncIterator is not undefined, then
+        // g. Else if usingSyncIterator is not undefined, then
         else if (using_sync_iterator) {
             // i. Set iteratorRecord to ? CreateAsyncFromSyncIterator(GetIterator(asyncItems, sync, usingSyncIterator)).
             // FIXME: The Array.from proposal is out of date - it should be using GetIteratorFromMethod.
             iterator_record = create_async_from_sync_iterator(vm, TRY(get_iterator_from_method(vm, async_items, *using_sync_iterator)));
         }
 
-        // j. If iteratorRecord is not undefined, then
+        // h. If iteratorRecord is not undefined, then
         if (iterator_record.has_value()) {
-            // i. Let k be 0.
-            // ii. Repeat,
+            GCPtr<Object> array;
+
+            // i. If IsConstructor(C) is true, then
+            if (constructor.is_constructor()) {
+                // 1. Let A be ? Construct(C).
+                array = TRY(JS::construct(vm, constructor.as_function()));
+            }
+            // ii. Else,
+            else {
+                // i. Let A be ! ArrayCreate(0).
+                array = MUST(Array::create(realm, 0));
+            }
+
+            // iii. Let k be 0.
+            // iv. Repeat,
             for (size_t k = 0;; ++k) {
                 // 1. If k ≥ 2^53 - 1, then
                 if (k >= MAX_ARRAY_LIKE_INDEX) {
@@ -471,6 +471,8 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayConstructor::from_async)
             // iii. Let len be ? LengthOfArrayLike(arrayLike).
             auto length = TRY(length_of_array_like(vm, array_like));
 
+            GCPtr<Object> array;
+
             // iv. If IsConstructor(C) is true, then
             if (constructor.is_constructor()) {
                 // 1. Let A be ? Construct(C, « 𝔽(len) »).
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js b/Userland/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js
index 6081c0e1a8b4c09..65c106bd6c701dc 100644
--- a/Userland/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js
+++ b/Userland/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js
@@ -3,13 +3,13 @@ test("length is 1", () => {
 });
 
 describe("normal behavior", () => {
-    function checkResult(promise) {
+    function checkResult(promise, type = Array) {
         expect(promise).toBeInstanceOf(Promise);
         let error = null;
         let passed = false;
         promise
             .then(value => {
-                expect(value instanceof Array).toBeTrue();
+                expect(value instanceof type).toBeTrue();
                 expect(value[0]).toBe(0);
                 expect(value[1]).toBe(2);
                 expect(value[2]).toBe(4);
@@ -57,4 +57,24 @@ describe("normal behavior", () => {
         );
         checkResult(promise);
     });
+
+    test("does not double construct from array like object", () => {
+        let callCount = 0;
+
+        class TestArray {
+            constructor() {
+                callCount += 1;
+            }
+        }
+
+        let promise = Array.fromAsync.call(TestArray, {
+            length: 3,
+            0: Promise.resolve(0),
+            1: Promise.resolve(2),
+            2: Promise.resolve(4),
+        });
+
+        checkResult(promise, TestArray);
+        expect(callCount).toBe(1);
+    });
 });