Print the AggregateError header before iterating its members - #35174
Print the AggregateError header before iterating its members#35174robobun wants to merge 4 commits into
Conversation
The native error printer (console.log/error of an Error, uncaught throw, unhandled rejection, Bun.inspect) handled AggregateError by iterating .errors and printing each member, then returning. The aggregate's own 'AggregateError: <message>' line and stack were never printed, so a Promise.any() rejection or any thrown AggregateError showed only the member errors with no indication of what actually failed. Move the aggregate-branch iteration in print_errorlike_object to after print_error_from_maybe_private_data so the aggregate itself is rendered first (name, message, stack), then each .errors member follows with a blank-line separator, matching how the cause chain is already printed.
WalkthroughChangesAggregateError printing now initializes exception data earlier and separates child errors with newlines. Tests update stack normalization, location snapshots, AggregateError subprocess coverage, and JSX diagnostic snapshots. AggregateError rendering
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:49 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 88317f7 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35174That installs a local version of the PR into your bun-35174 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Same bug as #35050 / #34497 (both by the same author, identical diff, CI blocked on fork). This PR takes a different route: instead of reading |
The transpiler's multi-error AggregateError now prints its '2 errors building ...' header before the individual BuildMessages.
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 `@test/js/bun/util/inspect-error.test.js`:
- Around line 198-201: Strengthen the assertions in
test/js/bun/util/inspect-error.test.js at lines 198-201 to verify header <
“error: m1” < “RangeError: m2” and require blank-line separation at both member
boundaries. At lines 214-216, assert the Promise.any header and message are
present, verify r1 precedes r2, and assert the required blank-line separation;
replace containment-only checks with order- and boundary-sensitive assertions.
🪄 Autofix (Beta)
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: b0fa66c1-db60-4ef6-98cd-b5e471022728
📒 Files selected for processing (3)
src/jsc/VirtualMachine.rstest/js/bun/util/inspect-error.test.jstest/regression/issue/jsx-template-string-crash.test.ts
|
CI on 88317f7 (build #78084): the remaining failures are unrelated to this change. Pre-existing on main:
Flaky (passed on retry elsewhere):
The tests touched by this PR ( |
There was a problem hiding this comment.
Beyond the two inline nits, I also checked: the early return in the aggregate branch now skips the was_internal tail after computing it — safe, since an AggregateError is a JS ErrorInstance and print_error_from_maybe_private_data returns false for it; the exception_list.as_deref_mut() reborrow still threads the same list into AggCtx for member errors; and the widened normalizeError regex only strips path-less at name (N:N) frames, so real [dir]/file:N:N frames survive.
Extended reasoning...
The core change (moving print_error_from_maybe_private_data above the is_aggregate_error branch) is sound and well-covered by the new subprocess tests. The two inline findings are both non-blocking nits — one pre-existing (.cause recursion path), one cosmetic (double blank between BuildMessage members, now certified by the regenerated snapshot). Leaving the approve/merge call to a human given the snapshot-spacing question and the two open duplicate PRs (#35050 / #34497) that need reconciling.
|
Superseded by #36602, which moves the |
|
Closing in favor of #36602, which contains this change (the .errors walk now runs after the aggregate's own header, from print_error_instance_body so cause-chain aggregates are covered too) together with the same test cases (console.error, Bun.inspect, uncaught throw, unhandled rejection, Promise.any) and the cycle/depth/tampered-property guards. |
The native error printer (used by
console.log/console.errorof anError, an uncaughtthrow, an unhandled rejection, andBun.inspect) handledAggregateErrorby iterating.errorsand printing each member, then returning. The aggregate's ownAggregateError: <message>header and stack were never printed, so aPromise.any()rejection or any thrownAggregateErrorshowed only the member errors with no top-level context.util.inspect(the Node-compat path) was already correct.Repro
// bun -e 'throw new AggregateError([new Error("m1"), new RangeError("m2")], "TOP-AGG-MESSAGE")'Before:
After:
Cause
VirtualMachine::print_errorlike_objectentered theis_aggregate_errorbranch first, iterated.errorsviafor_each->agg_iter->print_errorlike_object, and returned. The normalprint_error_from_maybe_private_datapath (which renders name, message, and stack) was never reached for the aggregate itself. The.errorsproperty isDontEnum, so the own-property dump insideprint_error_instance_bodynever sees it either.Fix
src/jsc/VirtualMachine.rs: move the.errorsiteration inprint_errorlike_objectto run afterprint_error_from_maybe_private_data, so the aggregate prints its own header and stack first, then each member follows with a blank-line separator (matching the existingcausechain layout).Tests
New
AggregateErrorsuite intest/js/bun/util/inspect-error.test.jscoveringBun.inspect,console.error, an uncaughtthrow, an unhandled rejection, andPromise.any. All five fail on main (no header in the output) and pass with this change. Existing inline snapshots in that file are regenerated for the added import line;normalizeErroris widened to also strip the newerat require (N:N)debug-only frame shape so the minified-file snapshots stay stable across debug/release.The duplicate render of a primitive
.errorsmember (error: xfollowed byx) is pre-existing behaviour ofprint_error_instance_bodyfor non-ErrorInstancevalues (also visible viareportError("x")) and is left unchanged here.Not addressed here
An
AggregateErrorreached via.cause(e.g.throw new Error("outer", { cause: new AggregateError([...], "agg") })) now printsAggregateError: aggthanks to this change, but its.errorsmembers are still omitted. That path recurses throughprint_error_instance_js(theerrors_to_appendloop inprint_error_instance_body), which never reaches the.errorsiteration inprint_errorlike_object. Pre-existing and left for a follow-up, likely by moving the iteration intoprint_error_instance_bodyso both entry points share it and reuse the existing circular-reference guard there.Related: #34892 guards the same branch against deep recursion; the two changes compose (whichever lands second has a trivial rebase).
Fixes #21528
[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file