Add regression test for iterator and spread TypeError source locations - #32442
Add regression test for iterator and spread TypeError source locations#32442robobun wants to merge 1 commit into
Conversation
WalkthroughThe PR updates the ChangesWebKit Pin Update and Issue
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 1:03 AM PT - Jul 1st, 2026
❌ @robobun, your commit 842047e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32442That installs a local version of the PR into your bun-32442 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the nits — test looks good now. Deferring to a human since this bumps WEBKIT_VERSION (and to a temporary preview pin that still needs swapping to a main autobuild SHA before merge).
Extended reasoning...
Overview
This PR changes two files:
scripts/build/deps/webkit.ts: bumpsWEBKIT_VERSIONfrom09f04cd5a4...toautobuild-preview-pr-255-38cbbb3e, a temporary preview build of oven-sh/WebKit#255. The comment in the file (and the PR description) explicitly says this must be swapped to amainautobuild SHA before merging.test/regression/issue/21134.test.ts: new regression test covering error-location attribution forfor-of,for-await-of, array destructuring,yield*, and array spread overnull/undefined.
The actual fix lives in JavaScriptCore (oven-sh/WebKit#255), not in this repo.
Prior review
I left two nits on the previous revision; both are now resolved:
describe.concurrent— applied in 9856b98.- Call/construct spread coverage — author explained those
OpSpreadpaths are defensive-only and not observable (callee resolution already emits expression info), so a test would pass on unpatched builds and gate nothing. Reasonable.
No new issues found in the current revision.
Security risks
None. The bun-side diff is a version-pin string change plus a read-only test that spawns bun -e subprocesses. No auth, crypto, network, or filesystem-write surface.
Level of scrutiny
High — needs human review. While the bun-side diff is small and the test is well-constructed, WEBKIT_VERSION controls which JavaScriptCore build the entire runtime links against. Any WebKit bump can affect correctness/perf across the whole engine and warrants a maintainer's sign-off, independent of how targeted the upstream patch is. Additionally, the pin is currently a PR preview build, not a merged main SHA — the author flagged this themselves as needing a swap before merge, so the PR is not in its final mergeable state yet.
Other factors
- Test quality is good: uses
evalto bypass Bun's transpiler for the JSC-specific cases, plus one end-to-end case for the original const-inlining repro; handles the debug-buildperformIterationbuiltin frame for spread; usesdescribe.concurrentper repo convention. - CI build #63008 is referenced but I can't verify its status here; a human should confirm it's green across platforms given this is a JSC bump.
9856b98 to
80fb408
Compare
|
Rebased onto |
There was a problem hiding this comment.
The test file looks good and prior nits are resolved, but I'm deferring to a human since the actual fix lives in oven-sh/WebKit#255 (not reviewable from this repo) and WEBKIT_VERSION is still pinned to a preview tag that the description says must be swapped to a main autobuild SHA before merging.
Extended reasoning...
Overview
This PR has two parts: (1) a one-line change to scripts/build/deps/webkit.ts that re-pins WEBKIT_VERSION from a main autobuild SHA to autobuild-preview-pr-255-e08efa3c, a preview build of the not-yet-merged oven-sh/WebKit#255; and (2) a new regression test test/regression/issue/21134.test.ts covering error-location attribution for for-of, for-await-of, array destructuring, yield*, and array spread over null/undefined. The substantive fix — adding emitExpressionInfo calls in JSC's BytecodeGenerator — lives entirely in the WebKit fork and is not part of this diff.
Security risks
None identified. The change is a dependency-version pin plus a test file that spawns bun -e subprocesses with fixed string literals; there is no auth, crypto, network, or untrusted-input handling involved.
Level of scrutiny
High. WEBKIT_VERSION selects the JavaScriptCore build that all of Bun links against — it is effectively the most load-bearing dependency in the project, and the diff being small does not make the change low-risk. More importantly, the code I would actually need to review (the BytecodeGenerator.cpp changes in oven-sh/WebKit#255) is not in this repository, so I cannot independently assess correctness of the fix from here. A human who can look at the WebKit-side diff should sign off.
Other factors
- The pin is explicitly temporary: both the PR description and the inline comment in
webkit.tssay to swap it for amainautobuild SHA once oven-sh/WebKit#255 merges. The PR is therefore not in a mergeable state by its own stated criteria, which by itself rules out bot approval. - Both of my earlier inline nits are resolved:
describe.concurrentwas applied in 9856b98, and the author gave a convincing explanation for why call/construct spread tests would not gate anything (those paths already inherit correct expression info and the WebKit-side change there is defensive only). - The regression test itself is well-constructed — it uses
evalto bypass Bun's transpiler for the JSC-only cases, asserts one.line/ stack frames rather than brittle full-message snapshots, handles theperformIterationbuiltin frame in debug builds, and includes the original const-inlining repro as an end-to-end check. I have no further concerns about the test file. - No bugs were found by the bug-hunting system on this revision.
80fb408 to
51f6f28
Compare
|
Rebased onto main again to resolve the conflict. main's CI here will 404 on the WebKit download until that preview build publishes. |
|
Build 65945 failed only on |
|
The WebKit preview ( |
|
Build 66096 is the first run against the published preview, and the diff itself is green: 244 jobs passed, and The remaining red is unrelated to this PR:
I am not pushing further empty commits to re-roll CI. To land this: re-run the expired lanes (or rebuild), merge oven-sh/WebKit#255, and swap |
JavaScriptCore attributed the TypeError thrown while getting [Symbol.iterator] (for-of, for-await-of, array destructuring, yield*) or spreading a null/undefined subject to the previous statement's expression range whenever the subject emitted no expression info of its own, such as a literal, including the one Bun produces by inlining `const a = undefined`. The fix landed upstream in WebKit/WebKit#67762 and is included in the WEBKIT_VERSION this branch now sits on (c9ad5813fd23), so this adds the Bun-side regression coverage for all five constructs plus the original report's repro. Fixes #21134
abb8033 to
842047e
Compare
robobun
changed the title
|
Resolved the conflict by taking main's Verified on this branch: |
There was a problem hiding this comment.
LGTM — now a test-only addition since the WebKit pin was dropped after the fix landed on main.
Extended reasoning...
Overview
This PR now touches a single file: test/regression/issue/21134.test.ts, a new 99-line regression test covering error source-location reporting for for-of, for-await-of, array destructuring, yield*, and array spread over null/undefined. The scripts/build/deps/webkit.ts change that earlier revisions carried (the temporary autobuild-preview-pr-255-* pin) is no longer in the diff — the branch was rebased onto a main whose WEBKIT_VERSION already contains the oven-sh/WebKit#255 fix, so this is purely a test addition with zero production/src/ changes.
Security risks
None. The change adds test code only; it spawns bunExe() subprocesses with fixed inline source strings via -e, uses bunEnv, and touches no auth, crypto, network, filesystem, or parsing surface.
Level of scrutiny
Low. Test-only additions that follow harness conventions and don't modify runtime behavior are the lowest-risk category. The tests follow repo conventions: describe.concurrent for independent subprocess spawns, await using proc, Promise.all to drain stdout/stderr/exit concurrently, exact line-number assertions (not just "doesn't crash"), and a debug-build accommodation for the performIteration builtin frame in the spread case. The exit-code assertion comes last per the CLAUDE.md guidance.
Other factors
My two prior nits are resolved: describe.concurrent was applied, and the author gave a well-reasoned explanation for why call/construct spread tests would be vacuous (those paths already inherit expression info from callee resolution and pass on the unpatched build). The bug-hunting system found no issues. The substantive change since my last review — dropping the temporary WebKit preview pin — strictly reduces risk. The actual behavioral fix lives in oven-sh/WebKit and was reviewed there; this PR just pins the regression coverage on the Bun side.
|
CI result for the test-only head (
I have already used my one empty retrigger commit, so I am leaving CI as is rather than pushing more. The PR is a single regression test on top of main. |
1 participant
Fixes #21134
Repro
Bun 1.4.0 and earlier report the previous statement:
Bun built from current main reports the
for-of:Cause
When the subject of
for-of,for-await-of, array destructuring,yield*, or an array spread emits no expression info of its own (a literal such asnullorundefined, including the literal Bun's transpiler produces by inliningconst a = undefined), JavaScriptCore's bytecode generator emitted the[Symbol.iterator]get_by_id(andOpSpread) without a precedingemitExpressionInfo. Those ops inherited the previous statement's expression range, so the TypeError pointed at the wrong line and theevaluating '...'text quoted the previous statement.Where the fix landed
The JavaScriptCore fix (emit expression info from the enclosing node before the
[Symbol.iterator]get and beforeOpSpread) landed upstream in WebKit/WebKit#67762, and main already contains it through the WebKit upgrade in #33133 (WEBKIT_VERSIONisc9ad5813fd23, whose tree has the newemitExpressionInfocalls at all of these sites). oven-sh/WebKit#255, the same change on the fork, was closed as superseded by the upstream patch.This PR is therefore the Bun regression coverage only:
test/regression/issue/21134.test.ts, covering all five constructs plus the original const-inlining repro. It changes no source and no longer touchesWEBKIT_VERSION.Verification
On this branch:
bun bd test test/regression/issue/21134.test.ts: 6 pass.USE_SYSTEM_BUN=1 bun test test/regression/issue/21134.test.ts(released 1.4.0, which predates the WebKit upgrade): 6 fail.Since current main already contains the JavaScriptCore fix, the test passes on main without any Bun source change from this PR; the failing state it guards is any Bun built before the
c9ad5813fd23WebKit pin.