bun -p: return module completion value, not first yielded await#30208
Conversation
For a module with top-level `await`, JSC compiles the body as a
generator and the first call into moduleLoader->evaluate() yields the
awaited value — not the module's final completion value.
EvalGlobalObject::moduleLoaderEvaluate captured that yielded value as
the --print result, and the async resume path in WebKit bypasses this
hook, so it was never corrected.
bun -p '(await 1) + 1' # was 1, now 2
bun -p 'await Promise.resolve("hi") + " y"' # was hi, now hi y
Fix: when the module record's generator state indicates it yielded
instead of completing, store its async capability promise instead of
the yielded value. The capability resolves to the module's final
completion value, and the --print loop already unwraps promises via
asAnyPromise + Bun__onResolveEntryPointResult.
Fixes #30207
|
Updated 5:26 PM PT - May 3rd, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 30208That installs a local version of the PR into your bun-30208 --bun |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWhen a module evaluated via ChangesTop-Level Await in Eval Expressions
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 45 seconds. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/cli/run/run-eval.test.ts`:
- Around line 113-127: The test uses test.each([...])("bun -p $expr →
$expected", ...) but repo convention requires table-driven cases be wrapped with
describe.each([...]) and then individual assertions run inside an it/test block;
replace the top-level test.each usage with describe.each([...]) to create a
describe block for each row, and inside that describe add an it or test that
calls Bun.spawnSync with bunExe() and bunEnv and asserts stderr, stdout, and
exitCode as before so the same matrix and expectations are preserved.
🪄 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: ae2f92fe-a122-4f9e-ab55-031b6b990596
📒 Files selected for processing (2)
src/bun.js/bindings/ZigGlobalObject.cpptest/cli/run/run-eval.test.ts
Coderabbit nit — switch to describe.each(...)+test pattern to match the wider repo convention.
|
Windows 2019 x64-baseline shard [83/506] failed on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this leans on JSC internals (generator State values and asyncCapability() semantics in the async-module resume path) that I'd want a maintainer familiar with the WebKit module loader to sanity-check before merging.
Extended reasoning...
Overview
This PR changes EvalGlobalObject::moduleLoaderEvaluate in src/bun.js/bindings/ZigGlobalObject.cpp so that when the bun --print entry-point module contains top-level await, the captured eval result is the module's asyncCapability() promise (whose resolution is the module's final completion value) rather than the first yielded awaited value returned from evaluateNonVirtual. It also adds a 4-case describe.each block to test/cli/run/run-eval.test.ts covering TLA and non-TLA expressions.
Security risks
None apparent. The change is read-only inspection of an existing AbstractModuleRecord plus a different JSValue passed to Bun__VM__setEntryPointEvalResultESM. It is gated behind Bun__VM__specifierIsEvalEntryPoint, so it only affects the bun -e/bun -p entry module, not general module loading. No new untrusted-input parsing, auth, or permission surfaces.
Level of scrutiny
Medium-high. Although the diff is small (~20 LOC) and the non-TLA path is provably unchanged (falls through to the original result), the correctness of the TLA path depends on JSC implementation details: that a yielded-but-not-finished async module leaves its State internal field as a number other than JSGenerator::State::Executing, that asyncCapability() is populated at this point, and that its resolution carries the module body's completion value (vs. undefined). These are vendor-WebKit invariants rather than public API, so a reviewer who knows the JSC async-module machinery should confirm them.
Other factors
- The fix is defensive:
dynamicDowncastand the nullasyncCapability()check both fall back to the previous behavior, so the worst realistic failure mode is the original (wrong) value rather than a crash. - Tests verify the user-visible behavior end-to-end via
bun -psubprocesses and include a non-TLA control case. - The only review feedback (CodeRabbit's
describe.eachsuggestion) was addressed in 1e920a2. - robobun's CI comment shows widespread
build-zigfailures on the earlier commit 1227cdd; these look infrastructure-related (this PR touches no Zig), but CI should be green before merge.
|
Build #50684 final: 4 failures, all pre-existing flakes unrelated to this PR:
This PR only changes |
…-sh#30208) ## Repro ```console $ bun -p '(await 1) + 1' 1 $ bun -p 'await Promise.resolve("hello") + " world"' hello ``` Expected: `2` and `hello world`. ## Cause `--print` uses ESM module evaluation and captures the last expression value via `EvalGlobalObject::moduleLoaderEvaluate` in `src/bun.js/bindings/ZigGlobalObject.cpp`. For a module with top-level `await`, JSC generator-ifies the body; the first call into `moduleLoader->evaluate()` yields the awaited value (`1`), not the module's final completion value (`2`). That yielded value was stored as the eval result. The async resume path (`asyncModuleExecutionResume` in `vendor/WebKit/.../JSMicrotask.cpp`) calls `module->evaluate()` directly and bypasses the `moduleLoaderEvaluate` hook, so the hook could never observe the final value and correct itself. ## Fix After the initial `evaluateNonVirtual` call, inspect the module record's generator state. If it yielded (state is a number other than `Executing`), the module still has work left and `result` is the awaited value. Store the module's `asyncCapability()` promise instead — its eventual resolution is the module's actual completion value. The `bun -p` loop in `src/bun.js.zig` already unwraps promises via `asAnyPromise` + `Bun__onResolveEntryPointResult`, so no Zig-side changes are needed. For non-TLA modules, behavior is unchanged (state is `Executing`, `result` stored as before). ## Verification - `USE_SYSTEM_BUN=1 bun test test/cli/run/run-eval.test.ts -t 'bun -p'` → 3 fail, 1 pass - `bun bd test test/cli/run/run-eval.test.ts -t 'bun -p'` → 4 pass - Full `test/cli/run/run-eval.test.ts` (33 tests) and TLA regression tests still pass. Fixes oven-sh#30207 --------- Co-authored-by: robobun <robobun@bun.sh>
2 participants
Repro
Expected:
2andhello world.Cause
--printuses ESM module evaluation and captures the last expressionvalue via
EvalGlobalObject::moduleLoaderEvaluateinsrc/bun.js/bindings/ZigGlobalObject.cpp. For a module with top-levelawait, JSC generator-ifies the body; the first call intomoduleLoader->evaluate()yields the awaited value (1), not themodule's final completion value (
2). That yielded value was stored asthe eval result.
The async resume path (
asyncModuleExecutionResumeinvendor/WebKit/.../JSMicrotask.cpp) callsmodule->evaluate()directly and bypasses the
moduleLoaderEvaluatehook, so the hookcould never observe the final value and correct itself.
Fix
After the initial
evaluateNonVirtualcall, inspect the modulerecord's generator state. If it yielded (state is a number other than
Executing), the module still has work left andresultis theawaited value. Store the module's
asyncCapability()promise instead— its eventual resolution is the module's actual completion value.
The
bun -ploop insrc/bun.js.zigalready unwraps promises viaasAnyPromise+Bun__onResolveEntryPointResult, so no Zig-sidechanges are needed. For non-TLA modules, behavior is unchanged (state
is
Executing,resultstored as before).Verification
USE_SYSTEM_BUN=1 bun test test/cli/run/run-eval.test.ts -t 'bun -p'→ 3 fail, 1 passbun bd test test/cli/run/run-eval.test.ts -t 'bun -p'→ 4 passtest/cli/run/run-eval.test.ts(33 tests) and TLA regression tests still pass.Fixes #30207