fix(jsc): print AggregateError message and stack before its children - #36580
fix(jsc): print AggregateError message and stack before its children#36580TheRodzz wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughChangesAggregateError rendering
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
…ven-sh#21528) Previously, `print_errorlike_object` returned early when handling an `AggregateError`, which completely skipped printing the `AggregateError`'s own message and stack trace. This moves the child error iteration to run after the main error is printed, so the parent `AggregateError` is clearly visible in the console output alongside its children. Signed-off-by: TheRodzz <81969589+TheRodzz@users.noreply.github.com>
52c12c6 to
d5d3c0d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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/VirtualMachine.rs`:
- Around line 4757-4792: Add regression coverage for AggregateError rendering
near the existing error-output tests, using a custom parent name and message
with multiple child errors. Assert the rendered output places the parent name,
message, and stack trace before all child error text, covering the behavior in
the internal-error handling around print_error_from_maybe_private_data and
add_to_error_list.
🪄 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 Plus
Run ID: ad6779d0-51c5-47f9-93d1-164fd518d86c
📒 Files selected for processing (1)
src/jsc/VirtualMachine.rs
| let exception_list_ptr = exception_list | ||
| .as_deref_mut() | ||
| .map(|l| l as *mut ExceptionList) | ||
| .unwrap_or(core::ptr::null_mut()); | ||
|
|
||
| // Note: reborrow so the add-to-error-list tail can still see it after | ||
| // `print_error_from_maybe_private_data`. | ||
| let mut exception_list = exception_list; | ||
| let was_internal = self.print_error_from_maybe_private_data( | ||
| value, | ||
| exception_list.as_deref_mut(), | ||
| formatter, | ||
| writer, | ||
| allow_ansi_color, | ||
| allow_side_effects, | ||
| ); | ||
|
|
||
| if was_internal { | ||
| if let Some(exception_) = exception { | ||
| let mut holder = crate::zig_exception::Holder::init(); | ||
| // Note: `holder.deinit(self)` runs at the tail (for borrowck) | ||
| // — semantics unchanged because | ||
| // `need_to_clear_parser_arena_on_deinit` is false here. | ||
| let zig_exception: &mut ZigException = holder.zig_exception(); | ||
| exception_.get_stack_trace(global_ref, &mut zig_exception.stack); | ||
| if zig_exception.stack.frames_len > 0 { | ||
| let _ = Self::print_stack_trace(writer, &zig_exception.stack, allow_ansi_color); | ||
| } | ||
| if let Some(list) = exception_list { | ||
| let top_level_dir = self.top_level_dir(); | ||
| let _ = | ||
| zig_exception.add_to_error_list(list, top_level_dir, Some(&self.origin)); | ||
| } | ||
| holder.deinit(self); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for AggregateError output order.
Add a rendering test with a custom parent name and message plus multiple child errors. Assert that the parent name, message, and stack trace appear before the child errors. This protects the behavior fixed by issue #21528.
Also applies to: 4843-4851
🤖 Prompt for 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.
In `@src/jsc/VirtualMachine.rs` around lines 4757 - 4792, Add regression coverage
for AggregateError rendering near the existing error-output tests, using a
custom parent name and message with multiple child errors. Assert the rendered
output places the parent name, message, and stack trace before all child error
text, covering the behavior in the internal-error handling around
print_error_from_maybe_private_data and add_to_error_list.
|
Thanks for this, @TheRodzz, you found the same root cause (the early return in print_errorlike_object that skipped the AggregateError's own header and stack). There were several overlapping PRs for #21528, so they are being consolidated into #36602, which takes the same approach one level lower (the .errors members are appended in print_error_instance_body, so an AggregateError reached through a cause chain also prints its members) and adds the tests and the cycle/depth/tampered-property guards. This branch has also picked up a merge conflict with main in the meantime. Closing in favor of #36602, which credits this PR in its description. Sorry for the churn, and thanks again for the contribution. |
2 participants
Fixes #21528
What
Refactored
VirtualMachine::print_errorlike_objectto not return early when printing anAggregateError. Instead, the parent error is printed normally, and then its child.errorsare printed immediately afterwards.Why
When an
AggregateErrorwith a custom message was thrown in Bun, only the children errors were logged to the console sequentially. The parent error's message, stack trace, and even theAggregateErrorname itself were silently ignored. This change aligns Bun's output closer to Node.js, making it obvious that the logged errors are wrapped inside anAggregateError.How it was tested
The rust compilation was verified. The logic was manually verified to ensure the iteration logic runs after the top-level error block is printed.