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
|
Warning Review limit reached
More reviews will be available in 2 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ 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
|
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.
…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.
e08efa3 to
1997f12
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
|
Rebased onto main ( |
| RefPtr<RegisterID> iterator = generator.newTemporary(); | ||
| RefPtr<RegisterID> nextOrIndex = generator.newTemporary(); | ||
| { | ||
| generator.emitExpressionInfo(divot(), divotStart(), divotEnd()); |
There was a problem hiding this comment.
🟡 Same problem exists in the sibling ObjectPatternNode::bindValue (line ~6293): emitRequireObjectCoercibleForDestructuring emits OpThrowStaticError with no preceding emitExpressionInfo, so const {a} = null after another statement still attributes the TypeError to the previous line. This is pre-existing and outside the stated [Symbol.iterator]/spread scope (object destructuring throws via static error, not get_by_id, so only line/column is wrong — no (evaluating ...) suffix), but since you're fixing the array sibling for the same root cause, the same one-liner — generator.emitExpressionInfo(divot(), divotStart(), divotEnd()) before emitRequireObjectCoercibleForDestructuring — would close the gap.
Extended reasoning...
What
ObjectPatternNode::bindValue (NodesCodegen.cpp:6285) begins with:
generator.emitRequireObjectCoercibleForDestructuring(rhs, firstPropertyName);emitRequireObjectCoercibleForDestructuring (BytecodeGenerator.cpp:5276–5291) emits OpJnundefinedOrNull followed by emitThrowTypeError → emitThrowStaticError → OpThrowStaticError::emit, and none of these call emitExpressionInfo. So the throw_static_error op inherits the previous statement's expression range — the exact mechanism this PR fixes for ArrayPatternNode::bindValue at line 6102.
Why nothing else covers it
Both array and object patterns reach bindValue through DestructuringAssignmentNode::emitBytecode (NodesCodegen.cpp:6079–6083): emitNode(initializer, m_initializer) on a NullNode calls emitLoad and emits no expression info, then m_bindings->bindValue() runs with nothing in between. That gap is precisely why the line-6102 fix was needed for arrays; the object path shares it.
ObjectPatternNode is a ThrowableExpressionData (Nodes.h:2553) whose divot is set via finishObjectPattern → setExceptionLocation (ASTBuilder.h:1072, called from Parser.cpp:1451), so divot()/divotStart()/divotEnd() are valid and the same one-line fix applies.
Step-by-step
console.log("here");
const {a} = null;console.log("here")emits its call; the lastemitExpressionInfopoints at line 1, column 9 (console.log).DeclarationStatement→DestructuringAssignmentNode::emitBytecoderuns.emitNode(initializer, NullNode)→emitLoad(jsNull()). No expression info emitted.m_bindings->bindValue(generator, rhs)→ObjectPatternNode::bindValue.emitRequireObjectCoercibleForDestructuring(rhs, &"a")emitsOpJnundefinedOrNull(not taken — rhs is null) thenOpThrowStaticError.- The
throw_static_errorop's expression range is still line 1, column 9. - Stack trace:
TypeError: Cannot destructure property 'a' from null or undefined valueattest.js:1:9instead of line 2.
Impact
Only line/column is wrong. Unlike the [Symbol.iterator] get_by_id case, OpThrowStaticError produces a fixed message with no (evaluating '…') suffix, so the error text itself doesn't quote the wrong source. This is why I'm flagging it as a nit rather than blocking — it's outside the PR's stated [Symbol.iterator]/spread scope and is pre-existing behavior the PR didn't touch.
Fix
void ObjectPatternNode::bindValue(BytecodeGenerator& generator, RegisterID* rhs) const
{
...
generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
generator.emitRequireObjectCoercibleForDestructuring(rhs, firstPropertyName);Directly mirrors what this PR does for ArrayPatternNode::bindValue.
|
The That run could no longer publish a release ( |
|
Suppressed by WebKit#67762 |
2 participants
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.