console.table: match Node's column model (symbol keys, properties dedup, Set, depth, key order) - #34241
console.table: match Node's column model (symbol keys, properties dedup, Set, depth, key order)#34241robobun wants to merge 8 commits into
Conversation
Fixes five divergences from Node.js in TablePrinter's column model:
(a) Symbol-keyed properties on rows became columns named by the symbol's
description, so two symbols with the same description collided. Node
uses ObjectKeys which skips symbols. Add an include_symbols option to
JSPropertyIterator (default true, preserving existing callers) and set
it false for both the per-row column scan and the outer row scan.
(b) The properties filter array was appended verbatim, so repeated entries
rendered the same column multiple times. Dedupe on append.
(c) Set entries that are objects were exploded into per-property columns.
Node routes every Set entry through a single Values column. Add a
dedicated is_set() branch in collect_row.
(d) Cell values were formatted at max_depth 5, so one deep value could
blow the table arbitrarily wide. Node uses depth 0 per cell. Change
the formatter default to max_depth 0 and fix Bun.inspect.table to
apply its depth option to max_depth (it was writing to the current
depth, which happened to give the same one-level output via a
different accident).
(e) The multi-row column union was first-seen order. Node builds the
union in a null-proto object and reads it back via ObjectKeys, so the
result follows OrdinaryOwnPropertyKeys: array-index names sorted
numerically first, then string names in first-seen order. After
collecting all rows, stably partition the data columns accordingly
and remap each row's cells.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 1:13 PM PT - Jul 15th, 2026
❌ @robobun, your commit be529eb has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34241That installs a local version of the PR into your bun-34241 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…pertyIterator doc comments Address review: is_set() matches Set|WeakSet but not SetIterator, so Set#values()/keys() still fell through to the object-explode path. Also update stale counts in JSPropertyIterator.rs doc comments now that there are six option fields.
Node returns from its map/setlike branches before consulting the properties argument, so Map and Set inputs always render fixed Key/Values columns regardless of the filter. Gate the properties pre-population on !setlike so we do the same instead of emitting always-blank columns.
…akSet) Node's isSet() tests for [[SetData]] so WeakSet is not setlike and is handled as a plain object. JSType::is_set() matches WeakSet too; switch to an is_setlike() helper that checks Set|SetIterator exactly so a WeakSet with expando own properties still explodes into columns as it did before this PR.
…o combined assertion
There was a problem hiding this comment.
All five rounds of inline feedback have been addressed and this run found no new issues, but I'm leaving the final call to a human: the PR changes Bun.inspect.table's default depth from 5 → 0 (a user-visible Bun-native API default), adds a parameter to the Bun__JSPropertyIterator__create FFI signature, and introduces a non-trivial column-reorder/remap pass — each individually fine but together past my auto-approve bar.
What was reviewed:
include_symbolsthreaded through Rust ↔ C++ FFI: extern-C signature andPropertyNameModeselection match; all four pre-existing callers passtrueso their behaviour is unchanged.parse_array_indexagainst ES array-index semantics (leading-zero, 2^32-1 boundary) — covered by tests.- Column reorder applying to user-supplied
propertiesfilter — verified this matches Node (Node also reorders the filter viaObjectAssigninto a null-proto map). is_setlike()now matches exactlySet | SetIterator(WeakSet excluded), and the properties pre-population is gated on!setlike && !is_map().
Extended reasoning...
Overview
Node-compat fix for console.table's column model across five behaviours: symbol keys excluded, properties filter deduped, Set/SetIterator entries kept whole in a single Values column, cell depth clamped to 0, and column order matching OrdinaryOwnPropertyKeys (array-index names sorted numerically first). Touches src/jsc/ConsoleObject.rs (TablePrinter), src/jsc/JSPropertyIterator.rs + .cpp (new include_symbols flag), src/runtime/api/BunObject.rs (Bun.inspect.table default depth and option wiring), plus mechanical include_symbols: true additions at three existing call sites (VirtualMachine.rs, JSTranspiler.rs, expect.rs), and a substantial new test block in console-table.test.ts.
Security risks
None. This is debug-output formatting; no auth/crypto/permissions/parsing-of-untrusted-network-input involved. The only external input is user-controlled JS values already in the VM, and the only outputs are strings written to stdout.
Level of scrutiny
Medium. console.table itself is low blast radius (debug output), but the PR crosses an FFI boundary (new bool parameter appended to Bun__JSPropertyIterator__create — verified the Rust extern declaration and C++ definition match in count/order/type), changes a Bun-native API default (Bun.inspect.table cells now render at max_depth = 0 instead of 5, and the depth option now writes to max_depth rather than depth), and adds ~40 lines of column-permutation logic that remaps every row's cells. These are the kinds of changes that benefit from a maintainer confirming the API-default change is intentional and acceptable.
Other factors
This PR has been through five prior review passes from me; every inline finding (SetIterator under-match, WeakSet over-match, properties filter on setlike inputs, stale doc comments, test-helper duplication, dead {depth: 0} option, bare stderr assertion) was addressed in follow-up commits, and all threads are resolved. The bug-hunting system raised and refuted one candidate this run (column reorder applying to the user-supplied properties filter — Node does the same via ObjectAssign into a null-proto object, so it's correct). Test coverage is thorough: 10 new tests covering all five behaviours plus array-index edge cases (4294967295, "01", "-1", numeric sort), one exercised via a spawned subprocess against console.table itself. The PR description states existing console-table and bun-inspect-table snapshots continue to match. Given the API-default change and FFI edit, I'm deferring rather than approving.
|
CI summary for be529eb (#73334):
Remaining red is unrelated to this diff:
Ready for review. |
|
Heads up: #36150 adds an |
|
Heads up: #37010 makes this iterator strings-only at the binding level. The same symbol leak also affected spawn env, the TOML/YAML/JSON5 serializers, and node:http2 wire headers, and no caller turned out to want symbols (inspect renders them via forEachProperty). If #37010 lands first, the include_symbols option and the table opt-outs here become unnecessary; the dedup, Set, depth, and column-order parts are independent and unaffected. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-15, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…ins (#35288) ### Problem - `console.log` of a nested `Map`, `Set`, `Array`, `MessageEvent`, Error `cause` chain, or `AggregateError` prints every level. A 1000-deep `Map` prints 2 MB. Deeper chains throw `RangeError: Maximum call stack size exceeded` out of `console.log`. A nested `AggregateError` chain overflows the native stack. - Cause: only `print_object` (`src/jsc/ConsoleObject.rs`) compares `depth` to `max_depth`. The other container printers never compare it. The `cause` loop and `agg_iter` (`src/jsc/VirtualMachine.rs`) do not track depth. ### Fix - Each container printer returns `[Array ...]`, `[Map ...]`, `[Set ...]`, `[MapIterator ...]`, or `[MessageEvent ...]` past the cap, like `[Object ...]`, through one `print_depth_exceeded_marker`. An empty container still prints `[]`, `Map {}`, or `Set {}`. - The `cause` loop and `agg_iter` bump `depth` and print `[Error ...]` past the cap. `agg_iter` also checks the native stack guard (`stack_check`). The error property dump narrows `max_depth`, so it keeps the caller's cap in `outer_max_depth` for these walks. - `Bun.inspect.table` passes its `depth` option as the cell start depth, against a fixed `max_depth` of 5. `TablePrinter::set_start_depth` now clamps it, so a `depth` above 5 prints cells like the default. - Verified: `inspect.test.js` (13 new cases, 12 fail on the released bun), `bun-inspect-table.test.ts` (2 new), and the related console suites. ### Background - `Formatter` (`ConsoleObject.rs`) is the native walker behind `console.log` and `Bun.inspect`. `max_depth` comes from `--console-depth`, `Bun.inspect(x, {depth})`, or the default of 2. - `print_error_instance_body` (`VirtualMachine.rs`) prints an Error, not `Formatter`. It dumps the error's properties one level deep and walks `cause` and `AggregateError.errors` itself. - `{depth: Infinity}` sets `max_depth` to `u16::MAX`, and `depth` saturates there. A depth comparison alone cannot stop that walk. <details><summary>Notes</summary> Sizes before and after, on this branch: | input | before | after | node | |-|-|-|-| | 100-deep `cause` chain | ~20 KB | 711 B | 575 B | | 1000-deep `Map` | ~2 MB | 167 B | 60 B | | 1000-deep `Set` | ~2 MB | 152 B | 40 B | | 100-deep `Array` | ~20 KB | 129 B | 20 B | | 1000-deep `MessageEvent` | ~3 MB | 261 B | n/a | | 3000 causes, 12000 Maps, 20000 AggregateErrors | `RangeError` or SIGSEGV | truncates | truncates | Tables. A container nested inside a cell now prints as a marker, like a nested plain object already did: `{ x: [Object ...] }`, `[ [Array ...] ]`, `Map(1) { 1: [Map ...] }`. Node's `console.table` prints the same shape (`[ [Array] ]`, `Map(1) { 1 => [Map] }`). For a `depth` option above 5, the released bun starts every cell past the cap: a plain object cell prints ` [Object ...]` and the new gates would have done the same to Array, Map, and Set cells. The clamp makes such a depth print cells like the default, object cells included. `{ depth: 0 }` keeps its meaning (`console-table.test.ts` uses it to mirror `console.table`). The option is not redefined as a `max_depth`: #34241 did that and was closed without a merge. AggregateError at the cap. When the members are one level past the cap, the AggregateError prints like any other error first (name, message, stack), then one `[Error ...]` per member. Without that, `Bun.inspect(agg, { depth: 0 })` printed only the markers. Node prints the same shape: the header, then `[errors]: [Array]`. The normal output is unchanged: members only, as #39633 left it. Errors inside Error properties. With `err.details = { inner }`, `err.list = [inner]`, or an `AggregateError` inside a property, the cause and the members of the nested error follow the caller's depth: `depth: Infinity` prints them all, `depth: 2` prints `[Error ...]`. Plain object properties keep the one-level cap. Cause depth per entry point, measured with a 50-deep chain. `console.log`: Node prints root + 2 causes, this branch prints root + 2 causes. Uncaught `throw`: Node prints root + 5 causes, this branch prints root + 8 (the error handler's `Formatter::new` depth), released bun prints all 50. #39637 changes the error handler's formatter to the console depth. Combined with this PR as written, an uncaught error would print root + 2 causes, and #39637's three-cause test would fail. The cause loop reads the depth cap on purpose so `{depth}` and `--console-depth` control it. Whichever PR lands second has to pick: the console depth for causes, or #39637 applying its depth only on the non-Error branch. `test/js/web/console/console-log.expected.txt` changes for `[[[[Array(1000).fill(4)]]]]`, which now stops at the fourth level like Node. The removed text (a long array wrapped at indent 10, then `... 900 more items`) is asserted again by `long arrays get cutoff at a nested indent` in `console-log.test.ts`, through `Bun.inspect(x, { depth: 5 })`. `stack_check` is the formatter's native stack guard. Container printers reach it in `print_as_prelude`. The error walks do not, so `agg_iter` calls it directly. Suites run on the merged branch: `inspect.test.js`, `inspect-error.test.js`, `reportError.test.ts`, `console-log.test.ts`, `console-depth.test.ts`, `bun-inspect.test.ts`, `bun-inspect-table.test.ts`, `console-table.test.ts`, `build-error.test.ts` (244 pass, 1 skip that is also skipped on main). JSX and Proxy have the same unbounded recursion. #29709 is open with that fix, so it is left out here. Review history: the `AggregateError` gate, its `stack_check`, the `print_event` gate, the relative `max_depth`, the state restore before `?` in the cause loop, the empty-container order, `outer_max_depth`, the table clamp, and the AggregateError header at the cap each came from review and each has a test. The branch merges main as of 2026-09-16. The only textual conflict was in `inspect.test.js`, where both sides appended a describe block. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
What
console.table's column model diverged from Node.js in five ways that change which data a table shows, not just how it looks.Cause
Node builds the column union by assigning into a null-proto object keyed by
ObjectKeys(row), then reading it back viaObjectKeys(map). That gives it three properties for free: symbol keys are excluded, duplicate keys dedupe, and the final order isOrdinaryOwnPropertyKeys(array-index names sorted numerically first, then string names in insertion order). Set/SetIterator go through a dedicatedsetlikebranch that never explodes per-property columns, and cell values are inspected at depth 0.Bun's
TablePrinter(src/jsc/ConsoleObject.rs) instead kept columns in a first-seenVeckeyed by the stringified property name from aJSPropertyIteratorthat yields symbols, appended thepropertiesfilter verbatim, let Set rows fall through to the generic object-explode path, and formatted cells atmax_depth = 5.Fix
include_symbolsflag toJSPropertyIterator(defaulttrueso every existing caller keeps its behaviour) and set itfalsefor both the per-row column scan and the outer row scan inTablePrinter.propertiesfilter when pre-populating columns.is_set()branch incollect_rowthat routes every Set entry to the Values column.max_depthto 0.Bun.inspect.tableis updated to apply itsdepthoption tomax_depth(it was writing to the formatter's currentdepth, which happened to give the same one-level output via a different accident) with a matching default of 0.Verification
New
column model (node compat)describe block intest/js/bun/console/console-table.test.tscovering all five cases plus the integer-index edge cases (4294967295,01,-1, numeric sort1/2/10). 8 of the 9 new tests fail onUSE_SYSTEM_BUN=1; all 9 pass underbun bd. All pre-existingconsole-table.test.tsandbun-inspect-table.test.tssnapshots continue to match.Not addressed here (cell-rendering cosmetics outside the column model): Node additionally renders cells whose value is a non-array object with more than two own keys as a bare
[Object](depth -1), and clamps arrays atmaxArrayLength: 3. Those are formatter-level refinements and would need separate work inFormatter.[review] gate passed · iteration 2 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 2
evidence per changed file