console.table: walk an array by its own properties instead of trusting length

src/jsc/ConsoleObject.rs

@@ -572,6 +572,10 @@ pub struct TablePrinter<'a> {
     tabular_data: JSValue,
     properties: JSValue,
 
+    /// Rows come from the iterator protocol (Map, Set, generators and other
+    /// iterables). Otherwise they are the own enumerable properties, like
+    /// Node's `Object.keys`: that includes arrays, so a hole or a `length`
+    /// an object merely reports never becomes a row.
     is_iterable: bool,
     jstype: jsc::JSType,
 
@@ -658,7 +662,7 @@ impl<'a> TablePrinter<'a> {
             global_object,
             tabular_data,
             properties,
-            is_iterable: tabular_data.is_iterable(global_object)?,
+            is_iterable: tabular_data.is_non_array_iterable(global_object)?,
             jstype: tabular_data.js_type(),
             value_formatter: {
                 // `Formatter` has a `Drop` impl, so struct-update

src/jsc/JSValue.rs

@@ -2426,6 +2426,12 @@ impl JSValue {
     pub fn is_iterable(self, global: &JSGlobalObject) -> JsResult<bool> {
         host_fn::from_js_host_call_generic(global, || JSC__JSValue__isIterable(self, global))
     }
+    /// [`is_iterable`](Self::is_iterable), but false for an array (through a
+    /// Proxy too) and for anything that iterates with the intrinsic Array
+    /// iterator, such as an `arguments` object.
+    pub fn is_non_array_iterable(self, global: &JSGlobalObject) -> JsResult<bool> {
+        crate::cpp::Bun__JSValue__isNonArrayIterable(self, global)
+    }
     /// `JSValue.forEach` — invoke `callback` for each iterable element.
     pub fn for_each(
         self,

src/jsc/bindings/bindings.cpp

@@ -4251,6 +4251,38 @@ bool JSC__JSValue__isIterable(JSC::EncodedJSValue JSValue, JSC::JSGlobalObject*
     return JSC::hasIteratorMethod(global, JSC::JSValue::decode(JSValue));
 }
 
+// Like hasIteratorMethod, but false for an array (IsArray, so a Proxy around
+// an array too) and for anything that iterates with the intrinsic Array
+// iterator (Array.prototype.values): an `arguments` object, a plain
+// array-like. That iterator trusts the `length` the object reports and never
+// checks that an element exists, so a caller that wants one step per existing
+// element walks the object's own properties instead.
+extern "C" [[ZIG_EXPORT(check_slow)]] bool Bun__JSValue__isNonArrayIterable(JSC::EncodedJSValue encodedValue, JSC::JSGlobalObject* globalObject)
+{
+    auto& vm = JSC::getVM(globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    JSC::JSValue value = JSC::JSValue::decode(encodedValue);
+    if (!value.isObject())
+        return false;
+    bool isArray = JSC::isArray(globalObject, value);
+    RETURN_IF_EXCEPTION(scope, false);
+    if (isArray)
+        return false;
+
+    JSC::CallData callData;
+    JSC::JSValue method = asObject(value)->getMethod(globalObject, callData, vm.propertyNames->iteratorSymbol, "Symbol.iterator property should be callable"_s);
+    RETURN_IF_EXCEPTION(scope, false);
+    if (method.isUndefined())
+        return false;
+
+    auto* function = dynamicDowncast<JSC::JSFunction>(method);
+    if (function && function == function->globalObject()->arrayProtoValuesFunctionConcurrently())
+        return false;
+
+    return true;
+}
+
 void JSC__JSValue__forEach(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, void* ctx, void (*ArgFn3)(JSC::VM* arg0, JSC::JSGlobalObject* arg1, void* arg2, JSC::EncodedJSValue JSValue3))
 {
     JSC::forEachInIterable(

test/js/bun/console/console-table.test.ts

@@ -363,3 +363,86 @@ console.log("calls=" + calls);`,
     expect({ stdout, stderr, exitCode }).toEqual({ stdout: box("1") + "calls=1\n", stderr: "", exitCode: 0 });
   });
 });
+
+// The rows of an array or array-like are the elements that exist, like Node's
+// console.table (Object.keys). The Array iterator instead trusts whatever
+// `length` the object reports and yields `undefined` for every missing index,
+// so a Proxy that lies about `length`, or a sparse array, would produce one
+// row per reported index (up to 2^32 - 1 of them).
+describe("console.table rows come from the elements that exist, not from length", () => {
+  const table = (...lines: string[]) =>
+    `┌───┬───┐\n│   │ a │\n├───┼───┤\n${lines.map(l => l + "\n").join("")}└───┴───┘\n`;
+
+  test("a Proxy that reports a length far beyond its elements", () => {
+    const proxy = new Proxy([{ a: 1 }, { a: 2 }], {
+      get(t, p, r) {
+        return p === "length" ? 1_000 : Reflect.get(t, p, r);
+      },
+    });
+    expect(Bun.inspect.table(proxy)).toBe(table("│ 0 │ 1 │", "│ 1 │ 2 │"));
+  });
+
+  test("a Proxy around an array still reads its cells through the get trap", () => {
+    const proxy = new Proxy([{ a: 1 }], {
+      get(t, p, r) {
+        return p === "0" ? { a: 2 } : Reflect.get(t, p, r);
+      },
+    });
+    expect(Bun.inspect.table(proxy)).toBe(table("│ 0 │ 2 │"));
+  });
+
+  test("a sparse array shows only the indices that hold an element", () => {
+    const sparse = [{ a: 1 }, , { a: 2 }];
+    sparse.length = 1_000;
+    expect(Bun.inspect.table(sparse)).toBe(table("│ 0 │ 1 │", "│ 2 │ 2 │"));
+  });
+
+  test("an array with only holes is an empty table", () => {
+    expect(Bun.inspect.table(new Array(1_000))).toBe(Bun.inspect.table([]));
+  });
+
+  test("a properties filter applies to the elements that exist", () => {
+    const sparse = [{ a: 1, b: 2 }, , { a: 3, b: 4 }];
+    expect(Bun.inspect.table(sparse, ["a"])).toBe(table("│ 0 │ 1 │", "│ 2 │ 3 │"));
+  });
+
+  // Object.freeze moves every element of an array into its sparse map.
+  test("a frozen array with a hole", () => {
+    const frozen = Object.freeze([{ a: 1 }, , { a: 2 }]);
+    expect(Bun.inspect.table(frozen)).toBe(table("│ 0 │ 1 │", "│ 2 │ 2 │"));
+  });
+
+  test("an array is walked like an object: its named properties are rows too", () => {
+    const arr = Object.assign([{ a: 1 }], { foo: { a: 2 } });
+    expect(Bun.inspect.table(arr)).toBe(Bun.inspect.table({ 0: { a: 1 }, foo: { a: 2 } }));
+    expect(Bun.inspect.table(new Proxy(arr, {}))).toBe(Bun.inspect.table(arr));
+  });
+
+  test("an Array subclass with its own iterator is still walked by its elements", () => {
+    class Rows extends Array {
+      *[Symbol.iterator]() {
+        yield { a: "from the iterator" };
+      }
+    }
+    expect(Bun.inspect.table(Rows.from([{ a: 1 }]))).toBe(table("│ 0 │ 1 │"));
+  });
+
+  test("console.table", async () => {
+    await using proc = Bun.spawn({
+      cmd: [
+        bunExe(),
+        "-e",
+        `const proxy = new Proxy([{ a: 1 }, { a: 2 }], {
+  get(t, p, r) {
+    return p === "length" ? 1_000 : Reflect.get(t, p, r);
+  },
+});
+console.table(proxy);`,
+      ],
+      env: bunEnv,
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect({ stdout, stderr, exitCode }).toEqual({ stdout: table("│ 0 │ 1 │", "│ 1 │ 2 │"), stderr: "", exitCode: 0 });
+  });
+});