Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions src/bun.js/VirtualMachine.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2454,31 +2454,10 @@
}
}

if (value.isAggregateError(this.global)) {
const AggregateErrorIterator = struct {
writer: Writer,
current_exception_list: ?*ExceptionList = null,
formatter: *ConsoleObject.Formatter,

pub fn iteratorWithColor(vm: *VM, globalObject: *JSGlobalObject, ctx: ?*anyopaque, nextValue: JSValue) callconv(.c) void {
iterator(vm, globalObject, nextValue, ctx.?, true);
}
pub fn iteratorWithOutColor(vm: *VM, globalObject: *JSGlobalObject, ctx: ?*anyopaque, nextValue: JSValue) callconv(.c) void {
iterator(vm, globalObject, nextValue, ctx.?, false);
}
fn iterator(_: *VM, _: *JSGlobalObject, nextValue: JSValue, ctx: ?*anyopaque, comptime color: bool) void {
const this_ = @as(*@This(), @ptrFromInt(@intFromPtr(ctx)));
VirtualMachine.get().printErrorlikeObject(nextValue, null, this_.current_exception_list, this_.formatter, Writer, this_.writer, color, allow_side_effects);
}
};
var iter = AggregateErrorIterator{ .writer = writer, .current_exception_list = exception_list, .formatter = formatter };
if (comptime allow_ansi_color) {
value.getErrorsProperty(this.global).forEach(this.global, &iter, AggregateErrorIterator.iteratorWithColor) catch return; // TODO: properly propagate exception upwards
} else {
value.getErrorsProperty(this.global).forEach(this.global, &iter, AggregateErrorIterator.iteratorWithOutColor) catch return; // TODO: properly propagate exception upwards
}
return;
}
// AggregateError is handled by printErrorInstance, which prints the
// wrapper (name / message / stack) and then fans out the non-enumerable
// `errors` array via the errors_to_append queue — see the cause block
// there for the same pattern.

was_internal = this.printErrorFromMaybePrivateData(
value,
Expand Down Expand Up @@ -3280,6 +3259,27 @@
try writer.writeAll("\n");
}

// `AggregateError.errors` is a non-enumerable own property, so the
// property iterator above skips it. Walk it manually and queue each
// entry (of any type — Promise.any can reject with non-Error values)
// so the wrapper gets printed first and the children follow, matching
// Node's `[errors]: [...]` format. Queue errors BEFORE cause so the
// drain loop emits them in Node's order (wrapper → errors → cause).
if (error_instance.isAggregateError(this.global)) {
const AggregateErrorsIterator = struct {
errors_to_append: *std.array_list.Managed(JSValue),

pub fn visit(_: *VM, _: *JSGlobalObject, ctx: ?*anyopaque, next_value: JSValue) callconv(.c) void {
const self: *@This() = @ptrCast(@alignCast(ctx.?));
next_value.protect();
bun.handleOom(self.errors_to_append.append(next_value));
}
Comment thread
robobun marked this conversation as resolved.
};
var errors_iter = AggregateErrorsIterator{ .errors_to_append = &errors_to_append };
const errors_array = error_instance.getErrorsProperty(this.global);
try errors_array.forEach(this.global, &errors_iter, AggregateErrorsIterator.visit);

Check failure on line 3280 in src/bun.js/VirtualMachine.zig

View check run for this annotation

Claude / Claude Code Review

AggregateError drain loop loses DOMWrapper BuildMessage/ResolveMessage specialized formatting

The drain loop that replays queued `errors_to_append` children calls `this.printErrorInstance(.js, err, ...)` for every entry, but `printErrorInstance` has no DOMWrapper branch — BuildMessage/ResolveMessage children produced by `processFetchLog` (when 2+ compile errors occur) fall through to the generic `jsc.Formatter.Tag.getAdvanced()` path, losing their specialized `msg.writeFormat()` output (file path, line/col, diagnostic text). Commit d3dc440 explicitly reverted the cd7672b fix that had rou
Comment on lines +3268 to +3280
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.

🔴 The drain loop that replays queued errors_to_append children calls this.printErrorInstance(.js, err, ...) for every entry, but printErrorInstance has no DOMWrapper branch — BuildMessage/ResolveMessage children produced by processFetchLog (when 2+ compile errors occur) fall through to the generic jsc.Formatter.Tag.getAdvanced() path, losing their specialized msg.writeFormat() output (file path, line/col, diagnostic text). Commit d3dc440 explicitly reverted the cd7672b fix that had routed drain-loop children through printErrorFromMaybePrivateData; the drain loop should call printErrorFromMaybePrivateData instead of printErrorInstance for each queued child so DOMWrapper children retain their specialized formatting.

Extended reasoning...

What the bug is and how it manifests

The new AggregateErrorsIterator.visit() callback (VirtualMachine.zig ~line 3272–3276) correctly appends every child from AggregateError.errors into errors_to_append without type filtering. However, the drain loop that consumes this queue calls this.printErrorInstance(.js, err, ...) for each entry. For DOMWrapper children — specifically BuildMessage and ResolveMessage objects — printErrorInstance does not have a DOMWrapper handling branch. These values fall through to the else if (mode == .js and error_instance \!= .zero) branch (~line 3292), which uses jsc.Formatter.Tag.getAdvanced() / formatter.format() for generic object inspection output instead of the human-readable diagnostic format.

The specific code path that triggers it

processFetchLog (lines 1961–1991) creates an AggregateError whose .errors array is filled with BuildMessage / ResolveMessage DOMWrapper objects when there are 2+ compile errors:

current.* = switch (msg.metadata) {
    .build => bun.api.BuildMessage.create(...),
    .resolve => bun.api.ResolveMessage.create(...),
};
ret.* = ErrorableResolvedSource.err(err, globalThis.createAggregateError(errors, ...));

When printErrorInstance processes this AggregateError, AggregateErrorsIterator.visit() appends each DOMWrapper child to errors_to_append. The drain loop then calls this.printErrorInstance(.js, domWrapperValue, ...). Inside printErrorInstance, is_error_instance = (error_instance.jsType() == .ErrorInstance) evaluates to false for DOMWrapper types, so the entire property-iteration block is skipped and execution falls to the generic formatter.

Why existing code doesn't prevent it

The specialized formatting path for BuildMessage/ResolveMessage lives in printErrorFromMaybePrivateData (lines 2484–2519), which checks value.jsType() == .DOMWrapper and then calls build_error.msg.writeFormat(writer, allow_ansi_color) / resolve_error.msg.writeFormat(writer, allow_ansi_color). This path is only reachable via printErrorFromMaybePrivateDataprintErrorInstance never dispatches through it for its drain-loop children.

History of the regression

Commit cd7672b had correctly fixed the drain loop to call printErrorFromMaybePrivateData so DOMWrapper children hit their specialized formatters. Commit d3dc440 (message: 'revert drain-loop DOMWrapper routing, use distinctive child messages') explicitly reverted this fix, stating the DOMWrapper case is 'out of scope'. The regression is therefore present at current HEAD.

Impact

Any user who runs Bun on a script that dynamically imports a module with 2+ syntax errors (or uses Bun.build with multiple diagnostics) will see generic object inspection output like BuildMessage { msg: { ... } } instead of the proper human-readable format (file path, line/col, error text). This is a real-world regression from pre-PR behavior.

Step-by-step proof

  1. processFetchLog creates an AggregateError with two BuildMessage DOMWrapper children.
  2. printErrorInstance is invoked for this AggregateError. is_error_instance = true for the wrapper; it prints name/message/stack.
  3. AggregateErrorsIterator.visit() is called twice; both DOMWrapper children are appended to errors_to_append.
  4. The drain loop iterates: calls this.printErrorInstance(.js, buildMsgWrapper, ...).
  5. Inside: is_error_instance = (buildMsgWrapper.jsType() == .ErrorInstance) = false.
  6. Falls to else if (mode == .js and error_instance \!= .zero)jsc.Formatter.Tag.getAdvanced() → generic object output.
  7. printErrorFromMaybePrivateData is never reached; msg.writeFormat() is never called.
  8. User sees BuildMessage { msg: { ... } } instead of a formatted diagnostic with source location.

Fix

In the drain loop (~line 3342), replace this.printErrorInstance(.js, err, ...) with this.printErrorFromMaybePrivateData(err, ...) (or a wrapper that dispatches correctly for both ErrorInstance and DOMWrapper types). This was the approach taken in cd7672b before it was reverted.

}
Comment thread
robobun marked this conversation as resolved.

// "cause" is not enumerable, so the above loop won't see it.
if (!saw_cause) {
if (try error_instance.getOwn(this.global, "cause")) |cause| {
Expand Down
1 change: 0 additions & 1 deletion test/internal/ban-limits.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
"EXCEPTION_ASSERT(!scope.exception())": 0,
"JSValue.false": 0,
"JSValue.true": 0,
"TODO: properly propagate exception upwards": 114,
"alloc.ptr !=": 0,
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
"alloc.ptr ==": 0,
"allocator.ptr !=": 1,
Expand Down
1 change: 0 additions & 1 deletion test/internal/ban-words.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ const words: Record<string, { reason: string; regex?: boolean }> = {
"globalThis.hasException": { reason: "Incompatible with strict exception checks. Use a CatchScope instead." },
"EXCEPTION_ASSERT(!scope.exception())": { reason: "Use scope.assertNoException() instead" },
" catch bun.outOfMemory()": { reason: "Use bun.handleOom to avoid catching unrelated errors" },
"TODO: properly propagate exception upwards": { reason: "This entry is here for tracking" },
};
const words_keys = [...Object.keys(words)];

Expand Down
87 changes: 87 additions & 0 deletions test/regression/issue/29157.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// https://github.com/oven-sh/bun/issues/29157

import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

test("console.dir on AggregateError prints the wrapper, not just the inner errors", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`const err = new AggregateError([new Error("child1"), new Error("child2")], "wrapper");
console.dir(err);`,
],
env: bunEnv,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("AggregateError");
expect(stdout).toContain("wrapper");
Comment thread
robobun marked this conversation as resolved.
expect(stdout).toContain("child1");
expect(stdout).toContain("child2");
expect(exitCode).toBe(0);
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Outdated
});

test("Promise.any rejection surfaces as an AggregateError, not a stray inner error", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `await Promise.any([Promise.reject(new Error("inner"))]).catch(console.dir);`],
env: bunEnv,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("AggregateError");
expect(stdout).toContain("inner");
expect(exitCode).toBe(0);
});

test("uncaught AggregateError rejection still shows the wrapper", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`await Promise.any([Promise.reject(new Error("child_aaa")), Promise.reject(new Error("child_bbb"))]);`,
],
env: bunEnv,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toContain("AggregateError");
// Distinctive messages so the assertion can only pass when the inner
// errors are actually printed — single-char strings would match the
// wrapper name or stack-frame content even if the children were dropped.
expect(stderr).toContain("child_aaa");
expect(stderr).toContain("child_bbb");
expect(exitCode).not.toBe(0);
Comment thread
claude[bot] marked this conversation as resolved.
});

test("AggregateError with a cause chain prints wrapper + errors + cause", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`const cause = new Error("root cause");
const err = new AggregateError([new Error("child")], "wrapper", { cause });
console.dir(err);`,
],
env: bunEnv,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("AggregateError");
expect(stdout).toContain("wrapper");
expect(stdout).toContain("child");
expect(stdout).toContain("root cause");
// Node's util.inspect emits "[errors]: [ ... ]" with the children before
// the cause. Assert that ordering so we don't regress to wrapper → cause →
// children (the pre-fix queueing order in errors_to_append).
const childIdx = stdout.indexOf("child");
const causeIdx = stdout.indexOf("root cause");
expect(childIdx).toBeGreaterThan(-1);
expect(causeIdx).toBeGreaterThan(-1);
expect(childIdx).toBeLessThan(causeIdx);
expect(exitCode).toBe(0);
});
Loading