diff --git a/src/jsc/ConsoleObject.rs b/src/jsc/ConsoleObject.rs index 1335268276bb..9e2a44e80e53 100644 --- a/src/jsc/ConsoleObject.rs +++ b/src/jsc/ConsoleObject.rs @@ -643,10 +643,64 @@ impl Default for Column { } enum RowKey { - Str(BunString), + /// Property-name UTF-8 slice + visible width (plain-object tabular data). + /// `to_utf8` refs the WTF impl (or owns a transcoded copy) and Drop + /// releases it, so the slice is safe to keep past the property iterator. + Str { + text: bun_core::ZigStringSlice, + width: u32, + }, + /// Row index (array / iterable tabular data). Rendered on demand. Num(u32), } +impl RowKey { + fn str(name: &BunString) -> Self { + Self::Str { + width: u32::try_from(name.visible_width_exclude_ansi_colors(false)).expect("int cast"), + text: name.to_utf8(), + } + } + + fn width(&self) -> u32 { + match self { + RowKey::Str { width, .. } => *width, + RowKey::Num(value) => bun_core::fmt::digit_count(*value) as u32, + } + } +} + +/// One formatted cell: the byte range of its rendered text in the shared +/// `cell_text` scratch buffer, plus that text's visible terminal width. +#[derive(Clone, Copy)] +struct CellRef { + /// Byte offset into `cell_text`. + offset: usize, + /// Byte length in `cell_text`. + len: usize, + /// Visible width of the text, excluding ANSI escape sequences. + width: u32, +} + +impl CellRef { + fn text<'t>(&self, cell_text: &'t [u8]) -> &'t [u8] { + &cell_text[self.offset..self.offset + self.len] + } +} + +/// One table row, collected during the width pass so the render pass never +/// re-reads a property (which would re-invoke its getter) and never +/// re-formats a value (which would re-run a custom inspect hook). +struct CollectedRow { + key: RowKey, + /// Indexed by `column_index - 1`. `None` is an absent cell. May be + /// shorter than `columns.len() - 1` when later rows added columns this + /// row lacks. + cells: Vec>, + /// Cell routed to the trailing "Values" column (primitives, Map values). + values_cell: Option, +} + const PADDING: u32 = 1; impl<'a> TablePrinter<'a> { @@ -677,69 +731,65 @@ impl<'a> TablePrinter<'a> { values_col_idx: usize::MAX, }) } -} -/// A `Write` sink that counts visible terminal columns instead of bytes. -struct VisibleCharacterCounter<'a> { - width: &'a mut usize, -} - -impl bun_io::Write for VisibleCharacterCounter<'_> { - fn write_all(&mut self, bytes: &[u8]) -> bun_io::Result<()> { - *self.width += strings::immutable::visible::width::exclude_ansi_colors::utf8(bytes); - Ok(()) - } -} - -impl<'a> TablePrinter<'a> { - /// Compute how much horizontal space a `JSValue` will take when printed. - fn get_width_for_value(&mut self, value: JSValue) -> JsResult { - let mut width: usize = 0; - // PERF: writes straight into the counter with no buffering between - // the generic writer adapter and the counter. Profile if hot. - let mut counter = VisibleCharacterCounter { width: &mut width }; + /// Format `value` exactly once (bare for strings, quoted otherwise), + /// appending its rendered bytes to the shared `cell_text` scratch, and + /// return the recorded byte range plus its visible width. + fn format_cell( + &self, + cell_text: &mut Vec, + value: JSValue, + ) -> JsResult { + let offset = cell_text.len(); let mut value_formatter = self.value_formatter.shallow_clone(); - let tag = formatter::Tag::get(value, self.global_object)?; value_formatter.quote_strings = !(matches!( tag.tag, TagPayload::String | TagPayload::StringPossiblyFormatted )); - let _ = value_formatter.format::(tag, &mut counter, value, self.global_object); - // VisibleCharacterCounter write cannot fail. - let _ = bun_io::Write::flush(&mut counter); + value_formatter.format::(tag, cell_text, value, self.global_object)?; - Ok(width as u32) + let text = &cell_text[offset..]; + Ok(CellRef { + offset, + len: text.len(), + width: strings::immutable::visible::width::exclude_ansi_colors::utf8(text) as u32, + }) } - /// Update the sizes of the columns for the values of a given row, and - /// create any additional columns as needed. - fn update_columns_for_row( + /// Read and format the row's cell values exactly once, size the columns + /// (creating them on demand), and return the collected row for the + /// render pass. + fn collect_row( &mut self, + cell_text: &mut Vec, columns: &mut Vec, - row_key: &RowKey, + row_key: RowKey, row_value: JSValue, - ) -> JsResult<()> { - // update size of "(index)" column - let row_key_len: u32 = match &row_key { - RowKey::Str(value) => { - u32::try_from(value.visible_width_exclude_ansi_colors(false)).expect("int cast") - } - RowKey::Num(value) => bun_core::fmt::digit_count(*value) as u32, + ) -> JsResult { + columns[0].width = columns[0].width.max(row_key.width()); + + let mut row = CollectedRow { + key: row_key, + cells: Vec::new(), + values_cell: None, }; - columns[0].width = columns[0].width.max(row_key_len); // special handling for Map: column with idx=1 is "Keys" if self.jstype.is_map() { - let entry_key = row_value.get_index(self.global_object, 0)?; - let entry_value = row_value.get_index(self.global_object, 1)?; - columns[1].width = columns[1].width.max(self.get_width_for_value(entry_key)?); - self.values_col_width = Some( - self.values_col_width - .unwrap_or(0) - .max(self.get_width_for_value(entry_value)?), - ); - return Ok(()); + let key_cell = self.format_cell::( + cell_text, + row_value.get_index(self.global_object, 0)?, + )?; + let value_cell = self.format_cell::( + cell_text, + row_value.get_index(self.global_object, 1)?, + )?; + columns[1].width = columns[1].width.max(key_cell.width); + self.values_col_width = Some(self.values_col_width.unwrap_or(0).max(value_cell.width)); + row.cells.push(Some(key_cell)); + row.values_cell = Some(value_cell); + return Ok(row); } if let Some(obj) = row_value.get_object() { @@ -751,7 +801,11 @@ impl<'a> TablePrinter<'a> { if !self.properties.is_undefined() { for column in columns[1..].iter_mut() { if let Some(value) = row_value.get_own(self.global_object, &column.name)? { - column.width = column.width.max(self.get_width_for_value(value)?); + let cell = self.format_cell::(cell_text, value)?; + column.width = column.width.max(cell.width); + row.cells.push(Some(cell)); + } else { + row.cells.push(None); } } } else { @@ -768,14 +822,14 @@ impl<'a> TablePrinter<'a> { let value = cols_iter.value; // find or create the column for the property - let column: &mut Column = 'brk: { + let col_idx: usize = 'brk: { let col_str = BunString::init(col_key); // reshaped for borrowck — split find/append. if let Some(idx) = columns[1..].iter().position(|col| col.name.eql(&col_str)) { - break 'brk &mut columns[1 + idx]; + break 'brk 1 + idx; } // Need to ref this string because JSPropertyIterator @@ -787,22 +841,25 @@ impl<'a> TablePrinter<'a> { name: col_str, width: 1, }); - let last = columns.len() - 1; - break 'brk &mut columns[last]; + break 'brk columns.len() - 1; }; - column.width = column.width.max(self.get_width_for_value(value)?); + let cell = self.format_cell::(cell_text, value)?; + columns[col_idx].width = columns[col_idx].width.max(cell.width); + let slot = col_idx - 1; + if row.cells.len() <= slot { + row.cells.resize(slot + 1, None); + } + row.cells[slot] = Some(cell); } } } else if self.properties.is_undefined() { // not object -> the value will go to the special "Values" column - self.values_col_width = Some( - self.values_col_width - .unwrap_or(1) - .max(self.get_width_for_value(row_value)?), - ); + let cell = self.format_cell::(cell_text, row_value)?; + self.values_col_width = Some(self.values_col_width.unwrap_or(1).max(cell.width)); + row.values_cell = Some(cell); } - Ok(()) + Ok(row) } fn write_string_n_times( @@ -819,28 +876,24 @@ impl<'a> TablePrinter<'a> { Ok(()) } - fn print_row( - &mut self, + fn print_row( + &self, writer: &mut dyn bun_io::Write, - columns: &mut [Column], - row_key: &RowKey, - row_value: JSValue, - ) -> JsResult<()> { + columns: &[Column], + row: &CollectedRow, + cell_text: &[u8], + ) { writer.write_all("│".as_bytes()).ok(); { - let len: u32 = match &row_key { - RowKey::Str(value) => value.visible_width_exclude_ansi_colors(false) as u32, - RowKey::Num(value) => bun_core::fmt::digit_count(*value) as u32, - }; - let needed = columns[0].width.saturating_sub(len); + let needed = columns[0].width.saturating_sub(row.key.width()); // Right-align the number column writer .splat_byte_all(b' ', (needed + PADDING) as usize) .ok(); - match &row_key { - RowKey::Str(value) => { - write!(writer, "{value}").ok(); + match &row.key { + RowKey::Str { text, .. } => { + writer.write_all(text.slice()).ok(); } RowKey::Num(value) => { write!(writer, "{value}").ok(); @@ -854,74 +907,29 @@ impl<'a> TablePrinter<'a> { writer.write_all("│".as_bytes()).ok(); - let mut value = JSValue::ZERO; - if col_idx == 1 && self.jstype.is_map() { - // is the "Keys" column, when iterating a Map? - value = row_value.get_index(self.global_object, 0)?; - } else if col_idx == self.values_col_idx { - // is the "Values" column? - if self.jstype.is_map() { - value = row_value.get_index(self.global_object, 1)?; - } else if !row_value.is_object() { - value = row_value; - } - } else if row_value.is_object() { - value = row_value - .get_own(self.global_object, &col.name)? - .unwrap_or(JSValue::ZERO); - } - - if value.is_empty() { - writer - .splat_byte_all(b' ', (col.width + PADDING * 2) as usize) - .ok(); + let cell = if col_idx == self.values_col_idx { + row.values_cell } else { - let len: u32 = self.get_width_for_value(value)?; - let needed = col.width.saturating_sub(len); - writer.splat_byte_all(b' ', PADDING as usize).ok(); - let tag = formatter::Tag::get(value, self.global_object)?; - let mut value_formatter = self.value_formatter.shallow_clone(); - - value_formatter.quote_strings = !(matches!( - tag.tag, - TagPayload::String | TagPayload::StringPossiblyFormatted - )); + row.cells.get(col_idx - 1).copied().flatten() + }; - // Release pooled visit map after formatting. - // `shallow_clone()` guarantees the source's `map_node` is - // `None`, so only the local clone needs draining. - // `Formatter::Drop` does the same release, so a plain scope - // is sufficient. - { - let result = value_formatter.format::( - tag, - writer, - value, - self.global_object, - ); - if let Some(mut node) = value_formatter.map_node.take() { - self.value_formatter.map_node = None; - let data = formatter::visited::node_data_mut(&mut node); - if data.capacity() > 512 { - data.deinit(); - } else { - data.clear(); - } - // SAFETY: `node` was obtained from `Pool::get_node()` and is - // exclusively owned by this formatter; `Map::INIT` is `Some`, - // so `data` is initialized. Ownership returns to the pool. - unsafe { formatter::visited::Pool::release(node.as_mut()) }; - } - result?; + match cell { + None => { + writer + .splat_byte_all(b' ', (col.width + PADDING * 2) as usize) + .ok(); + } + Some(cell) => { + let needed = col.width.saturating_sub(cell.width); + writer.splat_byte_all(b' ', PADDING as usize).ok(); + writer.write_all(cell.text(cell_text)).ok(); + writer + .splat_byte_all(b' ', (needed + PADDING) as usize) + .ok(); } - - writer - .splat_byte_all(b' ', (needed + PADDING) as usize) - .ok(); } } writer.write_all("│\n".as_bytes()).ok(); - Ok(()) } pub fn print_table( @@ -964,24 +972,32 @@ impl<'a> TablePrinter<'a> { } } - // rows first pass - calculate the column widths + // Width pass: format each cell exactly once, appending its bytes to + // `cell_text` and sizing columns. The render pass replays those byte + // ranges, so no property is re-read and no value is re-formatted. + let mut cell_text: Vec = Vec::new(); + let mut rows: Vec = Vec::new(); { if self.is_iterable { struct Ctx<'c, 'a> { this: &'c mut TablePrinter<'a>, + cell_text: &'c mut Vec, columns: &'c mut Vec, + rows: &'c mut Vec, idx: u32, - err: bool, + err: Option, } // Capture before constructing `ctx` (which mutably borrows `*self`). let tabular_data = self.tabular_data; let mut ctx = Ctx { this: self, + cell_text: &mut cell_text, columns, + rows: &mut rows, idx: 0, - err: false, + err: None, }; - extern "C" fn callback( + extern "C" fn callback( _: *mut jsc::VM, _: &JSGlobalObject, ctx: *mut c_void, @@ -989,22 +1005,30 @@ impl<'a> TablePrinter<'a> { ) { // SAFETY: ctx points to the stack `Ctx` above. let ctx = unsafe { bun_ptr::callback_ctx::>(ctx) }; - if ctx - .this - .update_columns_for_row(ctx.columns, &RowKey::Num(ctx.idx), value) - .is_err() - { - ctx.err = true; + // Once a cell failed, a JS exception may be pending (or + // the VM terminating); don't re-enter user code for the + // remaining elements. + if ctx.err.is_some() { + return; + } + match ctx.this.collect_row::( + ctx.cell_text, + ctx.columns, + RowKey::Num(ctx.idx), + value, + ) { + Ok(row) => ctx.rows.push(row), + Err(err) => ctx.err = Some(err), } ctx.idx += 1; } tabular_data.for_each_with_context( global_object, (&raw mut ctx).cast::(), - callback, + callback::, )?; - if ctx.err { - return Err(jsc::JsError::Thrown); + if let Some(err) = ctx.err { + return Err(err); } } else { let tabular_obj = self.tabular_data.to_object(global_object)?; @@ -1018,11 +1042,14 @@ impl<'a> TablePrinter<'a> { )?; while let Some(row_key) = rows_iter.next()? { - self.update_columns_for_row( + let key = RowKey::str(&BunString::init(row_key)); + let row = self.collect_row::( + &mut cell_text, columns, - &RowKey::Str(BunString::init(row_key)), + key, rows_iter.value, )?; + rows.push(row); } } } @@ -1093,77 +1120,9 @@ impl<'a> TablePrinter<'a> { writer.write_all("┤\n".as_bytes()).ok(); } - // rows second pass - print the actual table rows - { - if self.is_iterable { - struct Ctx<'c, 'a> { - this: &'c mut TablePrinter<'a>, - columns: &'c mut Vec, - writer: &'c mut dyn bun_io::Write, - idx: u32, - err: bool, - } - // Capture before constructing `ctx` (which mutably borrows `*self`). - let tabular_data = self.tabular_data; - let mut ctx = Ctx { - this: self, - columns, - writer, - idx: 0, - err: false, - }; - extern "C" fn callback( - _: *mut jsc::VM, - _: &JSGlobalObject, - ctx: *mut c_void, - value: JSValue, - ) { - // SAFETY: ctx points to the stack `Ctx` above. - let ctx = unsafe { bun_ptr::callback_ctx::>(ctx) }; - if ctx - .this - .print_row::(ctx.writer, ctx.columns, &RowKey::Num(ctx.idx), value) - .is_err() - { - ctx.err = true; - } - ctx.idx += 1; - } - tabular_data.for_each_with_context( - global_object, - (&raw mut ctx).cast::(), - callback::, - )?; - if ctx.err { - return Err(jsc::JsError::Thrown); - } - } else { - let Some(cell) = self.tabular_data.to_cell() else { - return Err(global_object.throw_type_error(format_args!( - "tabular_data must be an object or array" - ))); - }; - // `JSCell` is an `opaque_ffi!` ZST handle; `opaque_ref` is the - // centralised non-null deref proof (live JSC heap cell). - let row_obj = jsc::JSCell::opaque_ref(cell).to_object(global_object); - let mut rows_iter = jsc::JSPropertyIterator::init( - global_object, - row_obj, - jsc::PropertyIteratorOptions { - skip_empty_name: false, - include_value: true, - }, - )?; - - while let Some(row_key) = rows_iter.next()? { - self.print_row::( - writer, - columns, - &RowKey::Str(BunString::init(row_key)), - rows_iter.value, - )?; - } - } + // render pass: replay each row's pre-formatted cell bytes + for row in rows.iter() { + self.print_row(writer, columns, row, &cell_text); } // print the table bottom border diff --git a/test/js/bun/console/console-table.test.ts b/test/js/bun/console/console-table.test.ts index 653994bc14a5..32fee910127d 100644 --- a/test/js/bun/console/console-table.test.ts +++ b/test/js/bun/console/console-table.test.ts @@ -217,3 +217,149 @@ test("console.table repeat 50", () => { expect(renderTable([{ n: 8 }])).toBe(expected); } }); + +// Every cell must be read exactly once, matching Node. The table is built in +// two logical passes (column sizing, then rendering); re-reading in the second +// pass doubles getter side effects and renders the second call's value. +describe("console.table reads each cell once", () => { + const box = (v: string) => `┌───┬───┐\n│ │ x │\n├───┼───┤\n│ 0 │ ${v} │\n└───┴───┘\n`; + + test("enumerable getter on an array row", () => { + let calls = 0; + const row = {}; + Object.defineProperty(row, "x", { get: () => ++calls, enumerable: true }); + const out = Bun.inspect.table([row]); + expect({ calls, out }).toEqual({ calls: 1, out: box("1") }); + }); + + test("enumerable getter with an explicit properties list", () => { + let calls = 0; + const row = {}; + Object.defineProperty(row, "x", { get: () => ++calls, enumerable: true }); + const out = Bun.inspect.table([row], ["x"]); + expect({ calls, out }).toEqual({ calls: 1, out: box("1") }); + }); + + test("getter on a plain-object row key", () => { + let calls = 0; + const data = {}; + Object.defineProperty(data, "r", { + get() { + calls++; + return { a: calls }; + }, + enumerable: true, + }); + const out = Bun.inspect.table(data); + expect({ calls, out }).toEqual({ + calls: 1, + out: `┌───┬───┐\n│ │ a │\n├───┼───┤\n│ r │ 1 │\n└───┴───┘\n`, + }); + }); + + test("a generator is not consumed twice", () => { + function* rows() { + yield { a: 1 }; + yield { a: 2 }; + } + expect(Bun.inspect.table(rows())).toBe(`┌───┬───┐\n│ │ a │\n├───┼───┤\n│ 0 │ 1 │\n│ 1 │ 2 │\n└───┴───┘\n`); + }); + + test("getter on a primitive routed to the Values column", () => { + let calls = 0; + const data = {}; + Object.defineProperty(data, "a", { get: () => ++calls, enumerable: true }); + const out = Bun.inspect.table(data); + expect({ calls, out }).toEqual({ + calls: 1, + out: `┌───┬────────┐\n│ │ Values │\n├───┼────────┤\n│ a │ 1 │\n└───┴────────┘\n`, + }); + }); + + // String-ifying a cell runs user code. It must run exactly once per cell, + // and the table must show that single call's result, not a later one's. + test("a custom inspect on a cell value is invoked exactly once", () => { + let calls = 0; + const out = Bun.inspect.table([ + { + x: { + [Bun.inspect.custom]() { + return "C" + ++calls; + }, + }, + }, + ]); + expect({ calls, out }).toEqual({ + calls: 1, + out: `┌───┬────┐\n│ │ x │\n├───┼────┤\n│ 0 │ C1 │\n└───┴────┘\n`, + }); + }); + + test("a throwing custom inspect in a cell still propagates", () => { + const boom = new Error("boom"); + expect(() => + Bun.inspect.table([ + { + x: { + [Bun.inspect.custom]() { + throw boom; + }, + }, + }, + ]), + ).toThrow(boom); + }); + + // Each getter runs arbitrary user code, including a full GC. The cell must + // still render the value that its single read returned. + test("cell values survive a full GC between the width and render passes", () => { + const N = 64; + const rows = Array.from({ length: N }, (_, i) => ({ + get x() { + Bun.gc(true); + return { id: i }; + }, + })); + const out = Bun.inspect.table(rows); + const missing: number[] = []; + for (let i = 0; i < N; i++) if (!out.includes(`{ id: ${i} }`)) missing.push(i); + expect(missing).toEqual([]); + }); + + // Cells are keyed by column index in the width pass. A row that revisits an + // already-discovered column after creating a later one must not displace or + // truncate the cells it already captured. + test("a row whose key order differs from the column order", () => { + expect(Bun.inspect.table([{ a: 1 }, { b: 2, a: 3 }])).toBe( + `┌───┬───┬───┐\n│ │ a │ b │\n├───┼───┼───┤\n│ 0 │ 1 │ │\n│ 1 │ 3 │ 2 │\n└───┴───┴───┘\n`, + ); + }); + + // A single read per cell means the column is sized from the same value that + // gets rendered: the [[Get]] result, matching Node. The old render pass + // re-read through [[GetOwnProperty]], which a Proxy can observably diverge. + test("a Proxy row renders the [[Get]] value the width pass saw", () => { + const p = new Proxy({ x: "FROM_TARGET" }, { get: () => "FROM_GET" }); + expect(Bun.inspect.table([p])).toBe( + `┌───┬──────────┐\n│ │ x │\n├───┼──────────┤\n│ 0 │ FROM_GET │\n└───┴──────────┘\n`, + ); + }); + + test("console.table", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `let calls = 0; +const row = {}; +Object.defineProperty(row, "x", { get: () => ++calls, enumerable: true }); +console.table([row]); +console.log("calls=" + calls);`, + ], + 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: box("1") + "calls=1\n", stderr: "", exitCode: 0 }); + }); +});