Skip to content

error printer: size the own-property column from the names it actually prints - #38393

Open
robobun wants to merge 1 commit into
mainfrom
farm/7601de24/error-printer-property-width
Open

robobun wants to merge 1 commit into
mainfrom
farm/7601de24/error-printer-property-width

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • The own-property block printed under an error's message is right-aligned to a width taken from every own enumerable property name, but the loop that prints the block skips several of those names, so the lines it does print can be padded to a width nothing on screen uses:
    $ bun -e 'const e = new Error("boom"); e.a = 1; e.someCause = new Error("inner"); console.error(e)'
    error: boom
             a: 1,        <- padded to the width of someCause, which is printed after the stack trace
    
    The skipped names are message / name / stack (an own enumerable name is what every this.name = "MyError" constructor produces), the own code entry when the string code line is printed instead, Error-valued properties (printed in full after the stack trace) and enumerable accessors (never yielded by the iterator).
  • The opposite case is also misaligned: a string code that lives on the prototype or is a non-enumerable own property is printed as the last line of the block but does not count toward the width, so id: 1 is printed narrower than the code: "E_X" line under it.
  • Cause: print_error_instance_body (src/jsc/VirtualMachine.rs) takes the width from Bun__JSPropertyIterator__getLongestPropertyName (src/jsc/bindings/JSPropertyIterator.cpp), which looks at the whole property name array and knows nothing about which names the printer is about to skip or add. Same output for uncaught errors, console.error(err) and Bun.inspect(err), since they share this function. Pre-existing (the Zig version behaved the same), reproduced on 1.4.0.

Fix

  • The decision of what happens to a property (hidden, appended after the stack trace, printed inline) is moved into one local classify closure, and the iterator is run twice over the names it already collected: the first pass takes the width from the names classify reports as inline, starting from "code".len() when the code line will be printed; the second pass prints exactly as before. JSPropertyIterator::reset replaces get_longest_property_name, and the C++ helper, which had no other caller, is removed.
  • Correct because both passes ask the same function, so the width is by construction the maximum over the lines the block ends up containing; the prev_had_errors branch (inside a nested error, Error-valued properties are printed inline) is covered the same way because it is part of classify. The second pass is safe to run because the iterator uses observable: false (VM-inquiry reads, no getters or proxies run between the passes, so the object cannot change) and the name array is collected once at init. The .min(10) cap and the bytes written per line are unchanged, so errors whose longest own name was already a printed one (for example the ENOENT dump in stack.test.ts) print byte for byte as before.
  • Verified with test/js/bun/test/stack.test.ts: Bun.inspect, with and without colors, for each skipped kind of name (Error-valued property, Error-valued property longer than the cap, accessor, own enumerable name / message / stack), for a code line coming from an own enumerable, prototype and non-enumerable own code, for code plus an Error-valued property, for two unaffected cases (two printed properties, the 10 character cap), for the Error-valued property still being printed after the trace, for the nested-error case, and for an uncaught error in a child process. 10 of the new cases fail on the released binary (USE_SYSTEM_BUN=1), all pass with bun bd test. The expectations strip the trailing comma so they are independent of error printer: don't leave a trailing comma on the last own property line #38290.
  • Also ran test/js/bun/util/{inspect-error,reportError,inspect}.test with the debug build; the only failures are the two pre-existing minified-file snapshots that pick up a debug-only at require frame, which fail identically without this change.
  • Related open PRs in the same block: error printer: don't leave a trailing comma on the last own property line #38290 (trailing comma on the last line) and error printer: render non-Error cause values #35172 (non-Error cause line); whichever lands second needs a small rebase. error printer: render non-Error cause values #35172 would add "cause".len() to the width the same way code is added here.

Background

  • print_error_instance_body is the native printer behind uncaught exceptions, unhandled rejections, console.error(err) and Bun.inspect(err). For an Error instance it prints the source preview, the name: message line, then the block this PR is about: one right-aligned name: value, line per own enumerable data property (names capped at 10 columns), the string code as the last line, a blank line, the stack frames, and finally any Error-valued properties and cause, each printed as a full error of its own. Inside one of those nested prints (prev_had_errors), Error-valued properties are instead rendered inline in the block, to stop the recursion.
  • code is looked up separately (getCodePropertyVMInquiry, own or inherited data property); when it is a string it is printed as the dedicated last line and the own code entry, if any, is skipped by the loop.
  • JSPropertyIterator (src/jsc/JSPropertyIterator.rs over bindings/JSPropertyIterator.cpp) collects the property names once at init and yields (name, value) pairs on next(). With observable: false the value is read through a VM-inquiry slot, which runs no user code and yields nothing for accessors, so those properties are skipped before the printer sees them.

The own-property block under an error's message right-aligned its names
to the longest own enumerable property name, including names the block
never prints: message/name/stack, the own `code` entry when the string
code line is printed instead, Error-valued properties (printed after the
stack trace) and accessors (skipped by the iterator). When one of those
was the longest, the printed lines were padded to a width nothing else
used. Conversely, a string `code` found on the prototype or as a
non-enumerable own property was printed as the last line of the block
without contributing to the width, so the lines above it were narrower
than it.

Classify each property once (hidden / appended error / inline) and run
the iterator twice: the first pass takes the width from the inline
names plus `code` when the code line is printed, the second pass prints.
Bun__JSPropertyIterator__getLongestPropertyName was only used here and
is removed.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b4098e82-21ce-459f-99d8-7934c0365c94

📥 Commits

Reviewing files that changed from the base of the PR and between a0921e1 and 39b1f12.

📒 Files selected for processing (4)
  • src/jsc/JSPropertyIterator.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/JSPropertyIterator.cpp
  • test/js/bun/test/stack.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix and tests pushed, waiting for CI.

Reproduced on bun 1.4.0 with test/js/bun/test/stack.test.ts (USE_SYSTEM_BUN=1 bun test test/js/bun/test/stack.test.ts: 10 of the new cases fail, for example a: 1 next to an Error-valued someCause is printed as a: 1 and id: 1 above a prototype code line as id: 1); all pass with the debug build of this branch.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — cosmetic alignment fix in the error printer, well-scoped and thoroughly tested.

What was reviewed:

  • The two-pass iteration is safe: observable: false uses VM-inquiry reads (no user code runs between passes), the name array is collected once at init, and classify captures only immutable state (code, prev_had_errors).
  • The inline-print body is byte-identical to before, just dedented out of the else if into the Inline match fallthrough; .min(10) cap and saturating_sub padding preserved.
  • Confirmed no remaining callers of the removed Bun__JSPropertyIterator__getLongestPropertyName / get_longest_property_name.
  • reset() clears exactly the cursor state next() reads (i, iter_i, value); len/impl_ are unchanged so the second pass yields the same names.
Extended reasoning...

Overview

Fixes column alignment in the own-property block that print_error_instance_body emits under an error's name: message line. Previously the width came from Bun__JSPropertyIterator__getLongestPropertyName, which measured every enumerable name — including ones the print loop skips (name/message/stack, Error-valued properties deferred to after the trace, the own code when the dedicated code line prints instead) — and excluding the code line when it comes from the prototype or a non-enumerable slot. The fix extracts the skip logic into a local classify closure and runs the iterator twice: pass 1 takes max(field.length()) over names classified Inline (seeded with 4 when a code line will follow), pass 2 prints as before. JSPropertyIterator::reset() is added; the now-unused C++ helper and its Rust FFI wrapper are deleted.

Security risks

None. This is the diagnostic printer for uncaught errors / console.error / Bun.inspect — output-only, no parsing of untrusted input, no allocations sized by user data. The extra iteration pass runs under observable: false (VM-inquiry getPureResult), so no getters, proxies, or user code execute; the object cannot mutate between passes and no re-entrancy hazard is introduced.

Level of scrutiny

Moderate-to-low. The blast radius is cosmetic: worst case is misaligned output, not a crash or data corruption. The ~130-line diff in VirtualMachine.rs is mostly mechanical — the print body is dedented but unchanged, and the Hidden/AppendedError/Inline arms map one-to-one onto the old continue / errors_to_append.push / else-if-print branches (including the implicit fallthrough for non-object/non-primitive values, now explicitly Hidden). reset() clears exactly the three cursor fields next() reads. Grep confirms the removed C++ symbol has no other callers.

Other factors

Test coverage is strong: a test.each matrix over 12 cases covering both bug directions (over-padding from each skipped name kind, under-padding from prototype/non-enumerable code), the prev_had_errors nested-error branch, the 10-char cap, colored and uncolored Bun.inspect, and an uncaught-error subprocess case that drains both pipes concurrently and asserts {stdout, exitCode} as an object. Assertions strip trailing commas to stay independent of #38290. The PR author verified 10 new cases fail on the released binary and all pass with the debug build, and re-ran the neighbouring inspect-error/reportError/inspect suites. The existing ENOENT snapshot in the same file (whose longest name syscall is already inline) is unaffected.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:05 AM PT - Aug 14th, 2026

@robobun, your commit 39b1f12 is building: #95593

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:48 AM PT - Aug 14th, 2026

✅ @robobun, your commit 39b1f12e1c4b6bed0d58ef26f20a8bdd49ac3258 passed in Build #95593! 🎉


🧪   To try this PR locally:

bunx bun-pr 38393

That installs a local version of the PR into your bun-38393 executable, so you can run:

bun-38393 --bun

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant