Repository navigation
Conversation
TablePrinter converted every entry of `properties` with ToString. ToString throws a TypeError for a Symbol. Node uses each entry as a property key and lists only the string keys, so an entry whose key is a Symbol names no column. Add Bun__ConsoleObject__tableColumnName. It applies ToPropertyKey semantics: ToPrimitive with a string hint, then no column for a Symbol and ToString for any other value. This covers a Symbol, a boxed Symbol, and an object whose Symbol.toPrimitive, toString or valueOf returns a Symbol, for console.table and Bun.inspect.table when the data is an object.
|
Updated 9:12 AM PT - Oct 26th, 2026
✅ @robobun, your commit f5fc9797f6ba063487cb4ba80b292dcfc0611496 passed in 🧪 To try this PR locally: bunx bun-pr 46986That installs a local version of the PR into your bun-46986 --bun |
|
Caution CodeRabbit couldn't post its review summary. Error details
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the new Dead out-param sentinel in Bun__ConsoleObject__tableColumnName could be confused with a failed string conversion: Bun::toStringRef returns Dead only when toWTFString yields a null string, which happens only with a pending exception, and that path is covered by the RETURN_IF_EXCEPTION immediately after it, so a non-Symbol column cannot be silently dropped. The toPrimitive call that can run user JS is also exception-checked before its result is used, and the WTFStringImpl ref leaked by toStringRef is owned by the Rust BunString pushed into Column, matching the previous to_bun_string ownership.
Extended reasoning...
The change replaces a ToString coercion of console.table properties entries in src/jsc/ConsoleObject.rs with a new C++ export that runs ToPrimitive(PreferString) and reports a Symbol result via a Dead BunString so the column is skipped, plus a new test block in test/js/bun/console/console-table.test.ts. It touches no security-sensitive surface. Two confirmed Node-compat divergences are posted inline, so a human should weigh those; this note only records what else was examined in the new FFI helper.
The control for entries with a string key listed `1n` and `-0` after other names. Node prints integer-like keys first. With them first and ascending, the expected header is the same under both orders.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
LGTM — small, self-contained fix, and the follow-up commit reworks the control test so it no longer pins a column order Node does not print.
What was reviewed:
- src/jsc/bindings/ConsoleObject.cpp: ToPrimitive(PreferString) then the Symbol check matches ToPropertyKey; RETURN_IF_EXCEPTION follows both the ToPrimitive and the toStringRef call under one throw scope, and
*nameis written on everytruepath. - src/jsc/ConsoleObject.rs:
namestarts asBunString::DEAD, the FFI result is?-propagated beforenameis read, the Dead case is skipped with nothing to release, and the +1 WTF ref fromBun::toStringRefmoves intoColumnon the push path — no leak on the error or skip paths. - Non-Symbol entries keep the old ToString result (ToString(obj) is ToString(ToPrimitive(obj, string))), which the string-hint, coerced-once and still-throws controls pin.
Extended reasoning...
The change adds one false_is_throw C++ export and replaces a single to_bun_string call in TablePrinter::print_table with it, plus 18 tests in test/js/bun/console/console-table.test.ts. It touches no security-sensitive surface; the native code is a column-name coercion for console.table / Bun.inspect.table. Exception checks, out-param writes and BunString ownership across the FFI boundary were traced and are balanced on the success, skip and error paths. Since the prior review the author pushed one test-only commit addressing the column-order control I had flagged; no CHANGES_REQUESTED reviews are open, and no CODEOWNERS entry covers the changed files.
Problem
console.table(data, properties)andBun.inspect.table(data, properties)throw when apropertiesentry has a Symbol key. The error isTypeError: Cannot convert a symbol to a string. Node leaves that column out.TablePrinter::print_table(src/jsc/ConsoleObject.rs:899) converts each entry with ToString. Node uses the entry as a property key and lists only the string keys.Fix
Bun__ConsoleObject__tableColumnNameconverts an entry asobj[entry]does. A Symbol key gets no column.Object(Symbol()). Bun's JSConsole#tablealready prints these inputs as Node does.test/js/bun/console/console-table.test.ts(18 new tests, 14 fail on main). Also ranbun-inspect-table.test.tsand the console test directories.Background
TablePrinteris the native printer behindconsole.tableandBun.inspect.table. Its first loop turnspropertiesinto the column list.Symbol.toPrimitive, thentoString, thenvalueOf. A property key keeps a Symbol result. ToString throws for it.is_symbol()skip of console.table: skip Symbol entries in the properties filter #36150:Object(Symbol())still throws. Considered Node's column model: it adds work to calls that pass no Symbol.Notes
Repro
Each line throws on bun 1.4.2 and on main (449877f). Each line prints a table with the one column
aon Node v26.3.0 (theconsole.tablelines) and with this PR.Measurements (linux x64, release builds of main 449877f and of this PR)
properties: 176 instructions and 3 calls per entry (gdb stepi overprint_table::<false>, 16 names, 0 rows). main 449877f: 171 and 3. The call toBunString__fromJSbecomes the call to the new export.properties, 1 row x 2 keys: 10,544 instructions and 262 calls inprint_table(main: 10,547 and 262), the same call sequence.Bun.inspect.table([{a:1,b:2}], ['a','b'])on an--lto=offrelease build: entry coercion 2000 (main 2000),contiguousVectorIsStillValid2000 (main 2000),getOwn2000 (main 2000). The counts for[42, 'a']are the same. One call of 3 rows x 16 names: 16, 16 and 48 on both builds.print_table(mi_malloc/mi_realloc/mi_free): 1 row x 2 names 5/5/9 (main 5/5/9), 3 rows x 16 names 7/18/24 (main 7/18/24), 100 rows x 3 names 104/21/124 (main 104/21/124), 100 x 3 with noproperties304/21/324 (main 304/21/324).[42, {}]gives 4/6/9 on both. The JS heap object counts after 1000 calls and a full GC are equal on both builds..text: +512 B (llvm-size, main 449877f vs PR).Bun__ConsoleObject__tableColumnName469 B,print_table::<false>3,650 B (was 3,629),print_table::<true>3,730 B (was 3,709). cppbind exports +1 (114 to 115). Host functions +0. The stripped binary has the same file size.size_ofTablePrinter192 B (main 192),Column32 B (main 32).propertiesentry over 3 rows: 1 (main 1, Node v26.3.0 13). Reads per cell: 1 (main 1).BUN_JSC_validateExceptionChecks=1: 0. A build without the check after ToPrimitive aborts in that mode, so the mode is active.bloaty,valgrindandperfare not installed here. The numbers come fromllvm-size,llvm-nmand gdb.Not ported from Node
row[S]in each row for an entry whose key is a Symbol, and drops the result. This PR does not read it. The printed table is the same.propertiesfor zero rows, a Map or a Set.["b", "b", "a"]printsb,b,a. Node printsb,a.Scope
console.table, also in a webWorker, andBun.inspect.table, whendatais an array, an object, a Map, a Set or another iterable.new console.Console()andnode:worker_threadsworkers. They use the JSConsoleclass, which uses each entry as a property key (src/js/builtins/ConsoleObject.ts:723,:730). node:console: fix Console#table throwing on object cells, Maps and Sets #33452 corrects a different throw of that class.node:worker_threadsworkers to the native console, and its branch has the ToString loop (src/jsc/ConsoleObject.rs:1019at 58577cd). Those workers print the table today. They keep it if this change is in the base of worker_threads: keep the native console in workers, routed through process.stdout/stderr #39592. The two diffs do not overlap.%swith a Symbol in a format string is a different call site: console:%sprints a Symbol and an object thatString()cannot convert #45337.wrapTable) converts each entry withString(). For an object entry thatString()cannot convert, the inspector receives no table message, and a debug build stops at the assertion ininspector/ConsoleMessage.cpp:276. This occurs on main and is not changed here. A release build with this PR prints the table to the terminal.History
console.table.Object(Symbol())still throws.initial_columns(). Find console.table columns by the address of the property-name atom #38472, console.table: propagate errors thrown while printing the table #37402 and console.table: walk an array by its own properties instead of trusting length #41119 append tests at the same place inconsole-table.test.ts.Workaround for released versions
Tests
PreferNumber,NoPreference) each fail 2 of the new tests.console-table.test.ts50 pass, 19 snapshots unchanged.bun-inspect-table.test.ts38 pass, 26 snapshots unchanged.test/js/bun/console/,test/js/web/console/andtest/js/node/console/in one process: 131 pass.properties. Alone it takes 3.4 s to 4.3 s on this debug build, and 4.1 s on a debug build of main.Downsides
propertiesruns 5 more instructions per entry (176, main 171)..textgrows by 512 bytes.console.table("%s", [Symbol()])still throws. console.table: log only the first argument when it is not an object #46313 changes that path (datais not an object).