BytecodeGenerator: emit expression info before [Symbol.iterator] get and spread#255
BytecodeGenerator: emit expression info before [Symbol.iterator] get and spread#255robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
Source-position instrumentation in spread and iterator paths
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![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 issues — the added emitExpressionInfo calls mirror the existing pattern in emitIteratorOpen/emitIteratorNext and only affect error-message source attribution — but since this touches the JSC bytecode generator it's worth a quick human sign-off.
Extended reasoning...
Overview
This PR adds emitExpressionInfo(...) calls immediately before seven sites in the JSC bytecode generator that emit a get_by_id for [Symbol.iterator]/[Symbol.asyncIterator] or an OpSpread: emitNewArrayWithSpread, emitCall (single-spread fast path), emitConstructImpl (single-spread fast path), emitEnumeration, emitGetGenericIterator, emitGetAsyncIterator, and ArrayPatternNode::bindValue. In the spread cases it also hoists the SpreadExpressionNode* cast into a local to read its ThrowableExpressionData divots. No other behavior is changed.
Security risks
None. emitExpressionInfo only records source-position metadata into the unlinked code block's expression-info table; it does not alter the emitted opcode stream, register allocation, or control flow. SpreadExpressionNode already inherits ThrowableExpressionData (Nodes.h:930), so the divot accessors are valid. The node parameter in the iterator helpers is already dereferenced by surrounding code, so no new null-deref surface is introduced.
Level of scrutiny
The diff itself is mechanical and exactly mirrors the established pattern two functions away in emitIteratorOpen / emitIteratorNext (BytecodeGenerator.cpp:5399, 5413). Functionally it only changes which line/column and "evaluating '...'" snippet appear in a thrown TypeError. That said, this is the JavaScriptCore bytecode compiler — a critical code path — so per policy I'm deferring rather than auto-approving.
Other factors
The PR description reports a Bun regression test (oven-sh/bun#21134) covering all five constructs that fails before and passes after, verified on a debug+ASAN build. The bug-hunting system found no issues. There are no prior human reviews or outstanding comments to address.
Preview Builds
|
…and spread When the subject of for-of / for-await-of / array destructuring / yield* / array spread is a constant or other node that does not itself emit expression info (e.g. a null or undefined literal, including one produced by Bun's const inlining), the get_by_id for [Symbol.iterator] (or the OpSpread) inherited the previous statement's expression range. The resulting TypeError was attributed to the wrong line and the "evaluating '...'" text quoted the previous statement. Emit expression info from the enclosing ThrowableExpressionData before each of these ops so the throw is attributed to the iterating construct itself. Fixes oven-sh/bun#21134.
robobun
force-pushed
the
bun/fix-iterator-error-location-21134
branch
from
38cbbb3 to
e08efa3
Compare
…ndefined When the subject of for-of, for-await-of, array destructuring, yield* or array spread is a constant that emits no expression info of its own (a null/undefined literal, including one Bun produces by inlining `const a = undefined`), JSC's bytecode generator emitted the [Symbol.iterator] get_by_id (and OpSpread) without calling emitExpressionInfo. The TypeError inherited the previous statement's divot and was attributed to the wrong line, with the "evaluating '...'" text quoting that previous statement. Fixed in oven-sh/WebKit#255 by emitting expression info from the enclosing ThrowableExpressionData before each of these ops. This PR pins a preview build containing that change and adds a regression test covering all five constructs. Fixes #21134
There was a problem hiding this comment.
LGTM — purely additive expression-info emission following the existing emitIteratorOpen pattern; only affects error-message source attribution.
Extended reasoning...
Overview
This PR adds emitExpressionInfo(...) calls in seven locations in the bytecode compiler immediately before ops that can throw when the iterable is non-iterable: the [Symbol.iterator] / [Symbol.asyncIterator] get_by_id in emitEnumeration, emitGetGenericIterator, emitGetAsyncIterator, and ArrayPatternNode::bindValue, and the OpSpread emission in emitNewArrayWithSpread, emitCall, and emitConstructImpl. Three of the spread sites also hoist the SpreadExpressionNode* into a local so its divot data is reachable — a no-op refactor.
Security risks
None. emitExpressionInfo only appends source-position metadata to the unlinked code block's expression-info table; it does not change emitted instructions, register allocation, control flow, or any runtime semantics. The only observable effect is which line/column and "evaluating '…'" snippet appear in a thrown TypeError.
Level of scrutiny
While BytecodeGenerator.cpp is critical-path, this change is mechanical and follows the exact established pattern already used a few lines away in emitIteratorOpen / emitIteratorNext (emitExpressionInfo(node->divot(), node->divotStart(), node->divotEnd()) before the throwable op). Every node touched (SpreadExpressionNode, ArrayPatternNode, and the ThrowableExpressionData* node parameters) already inherits ThrowableExpressionData and was already being dereferenced for divot data in adjacent code (e.g. passed to emitCallIterator / emitIteratorOpen), so there's no new null-deref or uninitialized-divot risk. The debug ASSERTs in emitExpressionInfo would have caught bad divot data in the author's debug+ASAN run.
Other factors
The bug-hunting system found no issues. The PR includes a downstream regression test (oven-sh/bun test/regression/issue/21134.test.ts) covering all five constructs that fails before and passes after. The diff is small (~15 lines net), self-contained, and the worst-case failure mode is a slightly-off error location — not a correctness or stability regression.
1 participant
What
When the subject of
for-of,for-await-of, array destructuring,yield*, or array spread is a constant or other node that does not itself emit expression info (e.g. anull/undefinedliteral, including one produced by Bun's runtime-transpiler const inlining), theget_by_idfor[Symbol.iterator](and theOpSpread) inherit the previous statement's expression range. The resultingTypeErroris attributed to the wrong line and theevaluating '...'text quotes the previous statement.Fix
Emit expression info from the enclosing
ThrowableExpressionDatabefore each of these ops, matching whatemitIteratorOpen/emitIteratorNextalready do:emitEnumeration: before the[Symbol.iterator]emitGetByIdemitGetGenericIterator/emitGetAsyncIterator: before the[Symbol.iterator]/[Symbol.asyncIterator]emitGetByIdArrayPatternNode::bindValue: before the[Symbol.iterator]emitGetByIdemitNewArrayWithSpread/emitCallspread /emitConstructspread: beforeOpSpread::emitAfter:
Verification
Built Bun against this branch on linux x64 debug+ASAN; oven-sh/bun regression test (
test/regression/issue/21134.test.ts) covering all five constructs fails on the currently pinned commit and passes with this change. Normal for-of / destructuring / spread / yield* execution unaffected.Fixes oven-sh/bun#21134.