--print: share one body between the entry point promise reactions - #39128
--print: share one body between the entry point promise reactions#39128robobun wants to merge 2 commits into
Conversation
|
Updated 12:49 PM PT - Aug 15th, 2026
❌ @robobun, your commit 2795638 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39128That installs a local version of the PR into your bun-39128 --bun |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
LGTM — pure dedup of two byte-identical function bodies into one shared helper.
What was reviewed:
- Confirmed
Global::exitreturns!, so the-> !helper coerces cleanly into the wrappers'JsResult<JSValue>return type. - Checked the
HOST_EXPORTscraper ingenerate-host-exports.ts— markers still sit where the regex expects (immediately abovepub fn, with attribute/blank skipping), so both symbols still export. - New test drains pipes concurrently, asserts stdout before exit code, and covers both resolve/reject reactions; passing on the release binary is expected for a no-behavior-change refactor.
Extended reasoning...
Overview
This PR deduplicates two identical function bodies in src/runtime/hw_exports.rs. on_resolve_entry_point_result and on_reject_entry_point_result had byte-identical bodies (print argument 0 via ConsoleObject::message_with_type_and_level, then Global::exit with the VM's exit code). The fix extracts a private print_entry_point_result_and_exit(global, callframe) -> ! helper and reduces both exported functions to one-line calls. The two HOST_EXPORT symbols remain separate because C++ keys its PromiseFunctions table on function pointer identity. The corresponding mordant-baseline.toml lint-suppression entry is removed, and a test is added to test/cli/run/run-eval.test.ts locking in that both reactions print the settled value and exit 1 when reached via the pending-promise-after-loop-stopped path.
Security risks
None. This is a pure refactor of two identical bodies into a shared helper on the bun --print output path. No parsing, no auth, no untrusted input handling changes.
Level of scrutiny
Low. The diff is mechanical: the helper body is a verbatim move of the pre-existing code, the wrappers are one-liners, and the -> ! return type is sound because bun_core::Global::exit is declared -> ! (verified at src/bun_core/Global.rs:698). Rust's never type coerces to JsResult<JSValue>, so the wrappers still satisfy the codegen-expected signature. I checked src/codegen/generate-host-exports.ts — the marker regex requires // HOST_EXPORT(...) followed (after skipping attribute/comment/blank lines) by pub fn, and both markers still sit in that position.
Other factors
The added test follows harness conventions: describe.concurrent.each for the resolve/reject matrix, await using for the spawned process, concurrent Promise.all draining of stdout/stderr/exited, exact stdout assertion before exit-code assertion, and a comment explaining why the settling timer is nested inside the throwing callback (so it can't fire in the same timer batch). The PR description acknowledges the test passes on the release binary too — expected and appropriate for a no-behavior-change dedup that's locking in existing behavior rather than proving a fix. The removed baseline entry is the only reimplemented_helper entry for this file, matching the single lint site being resolved.
|
Local mordant run at the pinned revision is clean for this site: with the baseline entry removed, |
|
Second commit (55a5fa3) only touches the test, after the first CI run:
The remaining failures in that run ( |
There was a problem hiding this comment.
LGTM — pure dedupe of two byte-identical function bodies into one helper, no behavior change.
What was reviewed:
- Verified the helper body is identical to both removed bodies;
-> !is sound sinceGlobal::exitreturns!, which coerces into the wrappers'JsResult<JSValue>. - Confirmed both
// HOST_EXPORTmarkers still sit directly abovepub fnper thegenerate-host-exports.tsscraper's grammar, so the exported C symbols are unchanged. - New test drains stdout/stderr/exited concurrently, asserts output before exit code, and comments the Windows skip and
detect_leaks=0override.
Extended reasoning...
Overview
This PR collapses two byte-identical HOST_EXPORT function bodies in src/runtime/hw_exports.rs (on_resolve_entry_point_result / on_reject_entry_point_result) into a single private print_entry_point_result_and_exit(global, callframe) -> ! helper. Both exported wrappers become one-line calls to it. The corresponding reimplemented_helper:src/runtime/hw_exports.rs entry is removed from mordant-baseline.toml, and test/cli/run/run-eval.test.ts gains a two-case test.each covering the resolve and reject reactions of --print on a late-settling promise.
Security risks
None. This is a pure refactor of an internal helper on the --print CLI path — no auth, crypto, parsing of untrusted input, or new user-facing surface. The exported C-ABI symbols and their signatures are unchanged.
Level of scrutiny
Low. The diff is mechanical: the new helper body is line-for-line the same as both removed bodies (only the trailing ; on Global::exit(...) becomes a tail expression to satisfy -> !). I confirmed bun_core::Global::exit is declared -> ! at src/bun_core/Global.rs:698, so the never-type coerces cleanly into the wrappers' JsResult<JSValue> return. The generate-host-exports.ts scraper skips comment/attribute lines before matching pub fn, and both // HOST_EXPORT(...) markers remain directly above their pub fn — so codegen output is unaffected and the two distinct function pointers C++ keys on in PromiseFunctions are preserved.
Other factors
The added test follows repo conventions: describe.concurrent, async Bun.spawn with await using, concurrent draining of stdout/stderr/exited via Promise.all, stdout asserted before exit code, skipIf(isWindows) with a comment naming the libuv-poll timing reason, and the ASAN_OPTIONS: detect_leaks=0 override commented with why (the reaction calls Global::exit without VM teardown). The PR description states the test passes on both the release binary and the debug build, which is expected for a no-behavior-change refactor. The mordant baseline removal was verified live by the author with a control run. No outstanding reviewer comments.
55a5fa3 to
f61f1c8
Compare
|
Rebased onto main (force-push, now f61f1c8): the neighbouring |
There was a problem hiding this comment.
LGTM — pure dedupe of two byte-identical promise-reaction bodies into one private helper; both HOST_EXPORT symbols and their C++/run_command.rs consumers are unchanged.
What was reviewed:
- Confirmed the helper body is byte-identical to both originals and
Global::exitreturns!, so the-> !helper coerces into the wrappers'JsResult<JSValue>with no behavior change. - Checked
generate-host-exports.tsmarker grammar — each// HOST_EXPORT(...)still sits directly above apub fn, and the private helper is not scraped. - New test follows harness conventions (concurrent async spawn, pipes drained together,
bunEnvspread); the Windows skip anddetect_leaks=0override are commented with the pre-existing cause.
Extended reasoning...
Overview
This PR resolves a reimplemented_helper mordant lint by extracting the shared body of on_resolve_entry_point_result and on_reject_entry_point_result in src/runtime/hw_exports.rs into a single private print_entry_point_result_and_exit(global, callframe) -> ! helper. Both HOST_EXPORT wrappers become one-line calls. The corresponding mordant-baseline.toml entry is removed, and test/cli/run/run-eval.test.ts gains a two-case test that actually reaches these reactions (a promise still pending after an uncaught exception stops the event loop).
Security risks
None. The change is a mechanical dedupe with no new inputs, no parsing, and no control-flow change. The only external surface touched is the existing --print CLI path, whose behavior is unchanged.
Level of scrutiny
Low. The helper body is byte-for-byte the original body of both functions (confirmed against the removed lines in the diff). bun_core::Global::exit returns ! (src/bun_core/Global.rs:698), so declaring the helper -> ! is sound and the wrappers still type-check as JsResult<JSValue> via never-type coercion. The // HOST_EXPORT(...) markers remain directly above each pub fn, satisfying the generate-host-exports.ts scraper (which skips only attribute/comment/blank lines before matching pub fn). The private fn helper is not pub, so it is invisible to the scraper. Both exported symbols are still consumed unchanged by ZigGlobalObject.cpp (PromiseFunctions slots) and run_command.rs (then2).
Other factors
The new test uses describe.concurrent, async Bun.spawn with await using, drains stdout/stderr/exited via Promise.all, spreads bunEnv, and asserts stdout before exit code — all matching repo conventions. The skipIf(isWindows) and detect_leaks=0 override each carry a comment naming the pre-existing cause (libuv poll timing; Global::exit skipping VM teardown), and the PR description records that the LSan/exit-path issue predates this change and is tracked separately. Since this is a no-behavior-change dedupe, the test is expected to pass on both the release binary and the debug build, which the author verified. The mordant baseline removal was verified live with a control run. No prior human review comments to address.
Bun__onResolveEntryPointResult and Bun__onRejectEntryPointResult had identical bodies (as did the Zig originals). Both now call one private print-and-exit helper; the two exported symbols stay because C++ keys its PromiseFunctions table on the function pointer. No behavior change. Drops the reimplemented_helper baseline entry for hw_exports.rs and adds a test that reaches both reactions.
On Windows the extra libuv poll can return before the settling timer is due, so the still-pending promise is printed instead of reaching the reactions. Under LSan the reactions' direct exit (no VM teardown) reports still-live state as leaks and aborts, which also takes several seconds to symbolize; disable leak detection for the child.
f61f1c8 to
2795638
Compare
|
Status: the previous Buildkite build (98446, head f61f1c8) ran 177 jobs green, with the only failures being retried flakes in files this change does not touch ( |
There was a problem hiding this comment.
LGTM — pure dedupe of two byte-identical reaction bodies into one -> ! helper, no behavior change.
What was reviewed:
- Confirmed both
HOST_EXPORTsymbols remain and are still referenced separately byZigGlobalObject.cpp(PromiseFunctions slots) andrun_command.rs(then2). Global::exitreturns!, so the helper's diverging return type coerces cleanly into the wrappers'JsResult<JSValue>.- New test follows harness conventions (concurrent, drains pipes, stdout-before-exitCode); the
detect_leaks=0override matches an established pattern for pre-existing exit-path leaks and is commented.
Extended reasoning...
Overview
This PR deduplicates two byte-identical functions in src/runtime/hw_exports.rs — on_resolve_entry_point_result and on_reject_entry_point_result — by extracting their shared body into a private print_entry_point_result_and_exit helper with return type !. Both exported wrappers become one-line calls. The corresponding reimplemented_helper entry is removed from mordant-baseline.toml, and a new test in test/cli/run/run-eval.test.ts exercises both reactions.
Security risks
None. This is a pure refactor of an internal --print code path with no user-controllable input handling, auth, crypto, or trust boundaries touched.
Level of scrutiny
Low. The change is mechanical: the helper body is byte-for-byte the same code that was previously duplicated in both functions, minus one trailing semicolon (now a tail expression, since the helper returns !). I verified bun_core::Global::exit returns !, so the never-type coercion into JsResult<JSValue> in the wrappers is sound. Both HOST_EXPORT markers remain directly above their pub fn declarations as the codegen scraper requires, and both symbols are still consumed separately by ZigGlobalObject.cpp:4130-4133 and run_command.rs:1510-1511.
Other factors
- The new test uses
describe.concurrent,test.each,await using, concurrent pipe draining, and asserts stdout before exit code — all matching the repo's harness conventions. - The Windows skip and
detect_leaks=0override are both commented with the specific reason. The LSan override addresses a pre-existing exit-path leak (the reactions callGlobal::exitdirectly, skipping VM teardown) that predates this change and is documented in the PR description as tracked separately; the same override pattern appears in 10+ other test files. - The author verified the mordant baseline removal both locally and in CI (with a control run proving the lint is live), and confirmed identical behavior between release binary and debug build.
- No prior reviews from me or outstanding human reviewer comments.
|
Build 98578 (head 2795638): the one test that is red rather than retried is |
|
Closing in favor of #39175, which fixes the exit-path problem noted in the details section here. That change deletes |
Problem
reimplemented_helperonsrc/runtime/hw_exports.rs:on_reject_entry_point_resulthas the same signature and body ason_resolve_entry_point_result(print argument 0 withconsole.logformatting, thenGlobal::exitwith the VM's exit code). A change to one would miss the other.Bun__onResolveEntryPointResult/Bun__onRejectEntryPointResultinbun_js.zig, added in improve--printwith promises #9407) were identical too, so the port did not lose a reject-specific behavior. Printing both outcomes the same way matches the already-settled branch insrc/runtime/cli/run_command.rs(promise.result()is printed the same way whether the promise fulfilled or rejected), so this is duplication, not a missing behavior.Fix
print_entry_point_result_and_exithelper; bothHOST_EXPORTfunctions are one-line calls to it. No behavior change.ZigGlobalObject.cppmaps each function pointer to its ownPromiseFunctionsslot, andrun_command.rspasses both tothen2.mordant-baseline.toml: removed thereimplemented_helper:src/runtime/hw_exports.rsentry. The one-line wrappers are below the lint's minimum body size, so the site no longer reports.test/cli/run/run-eval.test.tsgains a case per reaction. An uncaught exception in a timer stops the event loop while the printed promise is still pending, so--printattaches these reactions; a timer created inside the throwing callback then fulfills or rejects the promise. Both print the settled value and exit 1 (from the uncaught exception). Passes with this change and with the released binary, as expected for a pure dedupe.detect_leaks=0(the reactions exit without VM teardown, so LSan reports the still-live state as leaks and aborts the child, which also spends about 4s symbolizing; see the details below). Neither caveat is introduced by this change; the reactions simply had no test before.bun bd test test/cli/run/run-eval.test.ts(39 pass); the same--printscripts produce identical stdout and exit codes on the release binary and the debug build with this change.bun run rust:mordantat the pinned revision reports nothing over the baseline with this entry removed (target/mordant/over-baseline.txtis not written). As a control, removing the still-validabi_type.rsentry from the baseline makes the same run report exactly that site, so the pass is live onbun_runtimeandhw_exports.rsis clean.Background
bun --print <expr>prints the value of the last expression. If that value is a promise,run_command.rsprints its settled result. If it is still pending after the event loop has drained, it attaches these two reactions withthen2and ticks once more; whichever reaction runs prints the value and exits.// HOST_EXPORT(Symbol)markers insrc/runtimeare scraped bysrc/codegen/generate-host-exports.ts, which emits theextern "C"thunks that C++ links against. The marker must sit directly above apub fn, which is why both exported functions remain as thin wrappers rather than one function with two names.Unrelated issue noticed on this path (not changed here)
Both reactions call
Global::exitdirectly, so they skip theANY_UNHANDLEDexit-code step, theprocess.on("exit")dispatch, and the VM teardown thatRun::startperforms afterwards. Three visible consequences:bun --print 'Promise.reject(new Error("early")); new Promise(r => setTimeout(() => r(5), 30))'reports the unhandled rejection and then exits 0, while the same script underbun -eexits 1.process.on("exit")listeners do not run when a reaction prints the value.detect_leaks=1, as the ASAN CI lanes set), the child aborts at exit with a report for state that the normal exit path frees, e.g. theArc<ParsedSourceMap>created while printing the uncaught exception. The normal--printpaths (already-settled promise, or a promise that never settles) are clean under the same options.This predates the port (the Zig code behaved the same) and is shared by both reactions, so it is tracked separately rather than folded into this dedupe. The new test is a ready-made trigger for that fix, which can then drop the
detect_leaks=0override.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-eval.test.ts