Conversation
When a macro function threw synchronously, the raw JSC Exception cell was handed to the result coercion switch, which has no arm for it and reported "cannot coerce Exception (JSType(0)) to Bun's AST. Please return a simpler type" with no mention of the user's error message or location. The byte-equivalent async function (promise rejection) reported correctly via unhandled_rejection. Route the sync-call exception through uncaught_exception (matching the existing T::Error arm) and return MacroFailed (matching the T::Promise rejection arm), so a sync throw and an async throw now report identically.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesMacro exception handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)test/bundler/transpiler/macro-test.test.tsFile contains syntax errors that prevent linting: Line 1: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 1: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 13: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 13: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 15: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 15: Expected a semicolon or an implicit semicolon after a statement, but found none Comment |
|
Updated 5:17 PM PT - Jul 13th, 2026
✅ @autofix-ci[bot], your commit 147fac1fff5c57ff4b9d45261250d68ad78abfe7 passed in 🧪 To try this PR locally: bunx bun-pr 34087That installs a local version of the PR into your bun-34087 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — sync-throw path now mirrors the established report_active_exception_as_unhandled pattern.
Checked that take_exception(e) + is_termination_exception() guard + uncaught_exception matches JSGlobalObject::report_active_exception_as_unhandled byte-for-byte, and that returning MacroFailed routes to the existing "macro threw exception" log entry via __bun_macro_context_call. Verified the T::Error and T::Promise rejection arms already use the same reporting/return shape, so behavior is now consistent across sync/async. Tests follow harness conventions (concurrent subprocess spawns, pipes drained via Promise.all, tempDir) and cover the variant matrix.
Extended reasoning...
Overview
Fixes error reporting when a bundler macro throws synchronously. Previously the JSC Exception cell was fed into the AST-coercion switch as if it were the macro's return value, producing an opaque "cannot coerce Exception (JSType(0))" message. Now the exception is taken off the VM and reported via uncaught_exception, matching how async rejections and returned Error objects were already handled. Two files touched: a ~10-line change in src/js_parser_jsc/Macro.rs:583-592 and four new test cases in test/bundler/transpiler/macro-test.test.ts.
Security risks
None. This is error-message reporting on an already-failing path; no new inputs are parsed, no new resources acquired, and the exception is only printed (not evaluated).
Level of scrutiny
Low-to-medium. The change is small and mechanically follows an existing in-tree helper — the new Err(e) arm is byte-equivalent to JSGlobalObject::report_active_exception_as_unhandled (src/jsc/JSGlobalObject.rs:1043), including the termination-exception guard. The MacroFailed return matches the T::Promise rejection arm and the ResolveMessage/BuildMessage arm in the same function, so downstream error-log handling in __bun_macro_context_call is unchanged. No lifetime, ownership, or GC-rooting changes.
Other factors
Test coverage is solid: four spawned-subprocess cases covering sync/async throw, non-Error throw, and both bun build and bun run entry points. Tests assert the user's message appears, the old "cannot coerce" message does not, and exitCode == 1. The PR description confirms the sync cases fail on 1.4.0 and pass with the change, and that 03830.test.ts still passes. No prior reviewer comments to address.
Printing a macro's stack trace caches an Arc<ParsedSourceMap> on the bundler-worker macro VM. bun build never tears that VM down on the failure-exit path (Global::exit is called directly), so the cache is reachable only via the worker's TLS, which LSan does not root-scan. With the ASAN runner's detect_leaks=1 inherited through bunEnv the child aborts with 134 instead of exiting 1. This is pre-existing (the async case, unchanged by this PR, leaks the same way) and unrelated to the error-reporting fix being tested; follow the pattern used by other tests that spawn bun build under ASAN.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js_parser_jsc/Macro.rs`:
- Around line 583-594: In the Err branch handling macro_callback.call within the
VM API-lock flow, replace the duplicated take_exception, termination check, and
uncaught_exception logic with the existing
JSGlobalObject::report_active_exception_as_unhandled helper, while preserving
the MacroError::MacroFailed return.
In `@test/bundler/transpiler/macro-test.test.ts`:
- Around line 154-166: Scope the ASAN_OPTIONS override in the Bun.spawn setup to
only the bun build invocation, leaving bun run executions with the original
bunEnv.ASAN_OPTIONS unchanged. Use the existing command value represented by cmd
to conditionally append detect_leaks=0 for the bundler-worker build cases.
🪄 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: ee59c968-7fe5-4216-945a-73bbddcfd55c
📒 Files selected for processing (2)
src/js_parser_jsc/Macro.rstest/bundler/transpiler/macro-test.test.ts
There was a problem hiding this comment.
LGTM — small, focused fix that routes sync macro throws through the same reporting path async rejections already use.
What was reviewed:
- The new
Errbranch usesreport_active_exception_as_unhandledexactly as documented inJSGlobalObject.rs:1038-1041and matches the pattern inevent_loop.rs/ipc.rs. MacroError::MacroFailedreturn matches the existingT::Promiserejection arm;__bun_macro_context_callskips its own log entry forMacroFailedso the caller's "macro threw exception" fallback still fires (asserted by the tests).- Both CodeRabbit review items (reuse helper; scope
detect_leaks=0tobun buildonly) are addressed and resolved; the LSan opt-out is a pre-existing bundler-worker teardown leak, not introduced here. - Tests follow harness conventions (tempDir, concurrent pipe drain,
test.concurrent.each) and the evidence gate shows 3 cases fail on main / pass with fix.
Extended reasoning...
Overview
Two files: a ~9-line change in src/js_parser_jsc/Macro.rs (Run::run_async) and four new parameterized test cases in test/bundler/transpiler/macro-test.test.ts. Previously, when a macro callback threw synchronously, the exception JSValue was pulled off the VM and fed into the AST-coercion switch as if it were the macro's return value, producing the useless "cannot coerce Exception (JSType(0)) to Bun's AST" message. Now the Err arm reports the exception via JSGlobalObject::report_active_exception_as_unhandled and returns MacroError::MacroFailed, matching the async-rejection path so sync and async throws produce identical output.
Security risks
None. This is error-message reporting for build-time macros; no auth, crypto, permissions, or untrusted-input parsing is touched. The exception is handed to the existing centralized reporter; termination exceptions are already filtered out by that helper.
Level of scrutiny
Low-to-medium. The change is small, self-contained, and mechanically substitutes a broken pattern with the exact idiom the helper's own doc comment prescribes (JSGlobalObject.rs:1036-1051). The MacroFailed return value plugs into the existing caller flow (__bun_macro_context_call special-cases it so visit_expr prints the "macro threw exception" line — verified by the test asserting both the user's message and that string appear in stderr). No new state, no lifetime changes, no allocation.
Other factors
- The PR's evidence gate confirms the three sync-throw tests fail on 1.4.0 with the old "cannot coerce" message and pass with the fix, in both ASAN-debug and release profiles; the async-throw case is a regression guard.
- Both CodeRabbit review comments were addressed in c36b1fa and marked resolved. The
detect_leaks=0override is scoped only tobun build(where the pre-existing bundler-worker macro-VM teardown leak lives) and is well-commented;bun runretains full LSan coverage. - Tests follow the repo's harness conventions:
tempDirwithusing,bunEnv/bunExe, concurrent stdout/stderr/exited drain,test.concurrent.each, output asserted before exit code, and a negative assertion (not.toContain("cannot coerce")) guarding the specific regression. - No CODEOWNERS-gated paths, no API surface change, no design decisions.
|
Closing in favor of #40059, which moves every macro onto one dedicated VM thread and decides this behavior as part of the rework. #40059 states: "A macro that throws (or returns an Verified on a debug build of the #40059 branch with this PR's four cases (sync throw, async throw, sync throw of a string, and sync throw under If #40059 does not land, this PR can be reopened. |
1 participant
Reproduction
Before this change,
bun build c.tsreports:The user's error message and location are completely lost. The byte-equivalent async function (
asyncThrow) already reports correctly:Cause
In
Run::run_async(src/js_parser_jsc/Macro.rs), whenmacro_callback.call()returnsErr, the exception is taken off the VM and fed directly into the result-coercion switch as if it were the macro's return value. The JSCExceptioncell matches none of the known tags and falls through to the "cannot coerce" fallback. The promise-rejection path (T::Promiseincoerce) already reports the rejection viaunhandled_rejectionand returnsMacroFailed.Fix
When the sync call fails, take the exception, report it through
uncaught_exception(matching the existingT::Errorarm) and returnMacroError::MacroFailed(matching theT::Promiserejection arm). A sync throw and an async throw now produce identical output.After this change,
bun build c.tsreports:Verification
test/bundler/transpiler/macro-test.test.tsgains four cases covering sync throw, async throw (regression guard), sync throw of a non-Error value, and sync throw underbun run. The three sync cases fail on 1.4.0 with "cannot coerce Exception (JSType(0))" and pass with this change; the whole file andtest/regression/issue/03830.test.tspass withbun bd test.[review] gate passed · iteration 1 · 2 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