console.table: propagate errors thrown while printing the table - #37402
console.table: propagate errors thrown while printing the table#37402robobun wants to merge 1 commit into
Conversation
message_with_type_and_level_ discarded the JsResult returned by TablePrinter::print_table, so a getter, Proxy trap or custom inspect hook that threw while the cells were being read left the exception pending on the VM while the host function reported success, and an OutOfMemory result was dropped without any exception being raised. Return the result instead, which lets the caller raise the OOM error and keeps the function's return value consistent with the VM state. Adds console.table coverage for throwing getters, Proxy traps and custom inspect hooks, including that nothing is printed.
Walkthrough
Changesconsole.table error handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ConsoleObject.rs`:
- Line 510: Update the flush handling in print_table so a successful print_table
followed by writer.flush() failure returns the flush error instead of discarding
it. Preserve any print_table error as the primary returned error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55f3417c-b917-45e6-8cf3-7bc272d47a7d
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/bun/console/console-table.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — trivially correct JsResult propagation, matching every other error path in this function.
Extended reasoning...
Overview
Two-file change: src/jsc/ConsoleObject.rs stops discarding print_table's JsResult in the console.table branch (still flushing the writer first), and test/js/bun/console/console-table.test.ts adds four tests locking in that exceptions from getters / Proxy traps / custom-inspect hooks propagate out of console.table.
Security risks
None. This only changes whether an already-pending JS exception's error state is reflected in the Rust return value (and whether an Err(OutOfMemory) from the printer gets converted to a thrown OOM by the outer wrapper). No new inputs, no new I/O.
Level of scrutiny
Low. The outer message_with_type_and_level at src/jsc/ConsoleObject.rs:361-368 already handles Err(Thrown) (no-op, exception pending) and Err(OutOfMemory) (calls throw_out_of_memory_value); TablePrinter::init(...)? on line 502 and the Dir branch's opts.get(...)? calls already propagate through the same path, so this change just brings the print_table call in line with its siblings. The let _ = writer.flush() before the return matches the existing pattern at lines 429-430. Bun.inspect.table already propagates the same result, so the Node/WHATWG-facing behavior was already established.
Other factors
The inline nit is informational: the author is upfront in the PR description that the new tests pass before and after the Rust change (the Thrown case already surfaced via the pending-exception slot; only the untestable-from-JS OutOfMemory case is observably different). The tests are still useful regression coverage for the throw contract, and the Rust change is obviously correct on inspection — "don't discard a JsResult" needs no further justification. Test structure follows harness conventions (bunExe()/bunEnv, await using on the spawn, concurrent pipe drain, exitCode asserted last).
|
Updated 9:34 PM PT - Aug 10th, 2026
✅ @robobun, your commit 3f1e37b8ae642f1e14604ff55fd7de58c6e44ed1 passed in 🧪 To try this PR locally: bunx bun-pr 37402That installs a local version of the PR into your bun-37402 --bun |
2 participants
What
message_with_type_and_level_wrotelet _ = table_printer.print_table(...)and returnedOk(()), discarding theJsResult.When a getter, Proxy trap or custom inspect hook throws while the cells are read,
print_tablereturnsErr(Thrown)with the exception pending on the VM, and the host function still reported success. The exception did reach JS through the pending-exception path, but the function's return value disagreed with the VM state, and anErr(OutOfMemory)from the printer was dropped outright (no exception raised at all, since it is the caller's job to throw it).Fix
Return the
print_tableresult from the table branch (still flushing the writer first), so the outermessage_with_type_and_levelhandles it like every other error from this function.Bun.inspect.tablealready propagates the same result.Tests
test/js/bun/console/console-table.test.ts:console.tablewith a throwing getter, a throwing ProxyownKeystrap and a throwing custom inspect hook rethrows the original error, and a script hitting it prints nothing to stdout and exits 1. These lock in the behaviour; they pass before this change as well, since the thrown case already surfaced via the pending exception.