Skip to content

BytecodeGenerator: emit expression info before [Symbol.iterator] get and spread #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

BytecodeGenerator: emit expression info before [Symbol.iterator] get and spread #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3497,10 +3497,12 @@ RegisterID* BytecodeGenerator::emitNewArrayWithSpread(RegisterID* dst, ElementNo
unsigned i = 0;
for (ElementNode* node = elements; node; node = node->next()) {
if (node->value()->isSpreadExpression()) {
ExpressionNode* expression = static_cast<SpreadExpressionNode*>(node->value())->expression();
auto* spread = static_cast<SpreadExpressionNode*>(node->value());
ExpressionNode* expression = spread->expression();
RefPtr<RegisterID> tmp = newTemporary();
emitNode(tmp.get(), expression);

emitExpressionInfo(spread->divot(), spread->divotStart(), spread->divotEnd());
OpSpread::emit(this, argv[i].get(), tmp.get());
} else {
ExpressionNode* expression = node->value();
Expand Down Expand Up @@ -3781,8 +3783,10 @@ RegisterID* BytecodeGenerator::emitCall(RegisterID* dst, RegisterID* func, Expec
if (expression->isArrayLiteral()) {
auto* elements = static_cast<ArrayNode*>(expression)->elements();
if (elements && !elements->next() && elements->value()->isSpreadExpression()) {
ExpressionNode* expression = static_cast<SpreadExpressionNode*>(elements->value())->expression();
auto* spread = static_cast<SpreadExpressionNode*>(elements->value());
ExpressionNode* expression = spread->expression();
RefPtr<RegisterID> argumentRegister = tempDestination(emitNode(callArguments.argumentRegister(0), expression));
emitExpressionInfo(spread->divot(), spread->divotStart(), spread->divotEnd());
OpSpread::emit(this, argumentRegister.get(), argumentRegister.get());

return emitCallVarargs<typename VarArgsOp<CallOp>::type>(dst, func, callArguments.thisRegister(), argumentRegister.get(), newTemporary(), 0, divot, divotStart, divotEnd, debuggableCall);
Expand Down Expand Up @@ -3993,11 +3997,14 @@ RegisterID* BytecodeGenerator::emitConstructImpl(RegisterID* dst, RegisterID* fu
if (expression->isArrayLiteral()) {
auto* elements = static_cast<ArrayNode*>(expression)->elements();
if (elements && !elements->next() && elements->value()->isSpreadExpression()) {
ExpressionNode* expression = static_cast<SpreadExpressionNode*>(elements->value())->expression();
auto* spread = static_cast<SpreadExpressionNode*>(elements->value());
ExpressionNode* expression = spread->expression();
RefPtr<RegisterID> argumentRegister = tempDestination(emitNode(callArguments.argumentRegister(0), expression));

if (!isDefaultDerivedConstructorCall)
if (!isDefaultDerivedConstructorCall) {
emitExpressionInfo(spread->divot(), spread->divotStart(), spread->divotEnd());
OpSpread::emit(this, argumentRegister.get(), argumentRegister.get());
}

move(callArguments.thisRegister(), lazyThis);
return emitCallVarargs<typename VarArgsOp<ConstructOp>::type>(dst, func, callArguments.thisRegister(), argumentRegister.get(), newTemporary(), 0, divot, divotStart, divotEnd, DebuggableCall::No);
Expand Down Expand Up @@ -4980,6 +4987,7 @@ void BytecodeGenerator::emitEnumeration(ThrowableExpressionData* node, Expressio
RefPtr<RegisterID> nextOrIndex = newTemporary();
RefPtr<RegisterID> iterator = newTemporary();
{
emitExpressionInfo(node->divot(), node->divotStart(), node->divotEnd());
RefPtr<RegisterID> iteratorSymbol = emitGetById(newTemporary(), iterable.get(), propertyNames().iteratorSymbol);
CallArguments args(*this, nullptr, 0);
move(args.thisRegister(), iterable.get());
Expand Down Expand Up @@ -5408,6 +5416,7 @@ void BytecodeGenerator::emitIteratorNext(RegisterID* done, RegisterID* value, Re

RegisterID* BytecodeGenerator::emitGetGenericIterator(RegisterID* argument, ThrowableExpressionData* node)
{
emitExpressionInfo(node->divot(), node->divotStart(), node->divotEnd());
RefPtr<RegisterID> iterator = emitGetById(newTemporary(), argument, propertyNames().iteratorSymbol);
emitCallIterator(iterator.get(), argument, node);

Expand Down Expand Up @@ -5467,6 +5476,7 @@ void BytecodeGenerator::emitIteratorGenericClose(RegisterID* iterator, const Thr

RegisterID* BytecodeGenerator::emitGetAsyncIterator(RegisterID* argument, ThrowableExpressionData* node)
{
emitExpressionInfo(node->divot(), node->divotStart(), node->divotEnd());
RefPtr<RegisterID> iterator = emitGetById(newTemporary(), argument, propertyNames().asyncIteratorSymbol);
Ref<Label> asyncIteratorNotFound = newLabel();
Ref<Label> asyncIteratorFound = newLabel();
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6099,6 +6099,7 @@
RefPtr<RegisterID> iterator = generator.newTemporary();
RefPtr<RegisterID> nextOrIndex = generator.newTemporary();
{
generator.emitExpressionInfo(divot(), divotStart(), divotEnd());

Check warning on line 6102 in Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

View check run for this annotation

Claude / Claude Code Review

ObjectPatternNode::bindValue has the same misattribution bug 

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 si

@claude claude Bot Jun 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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;
  1. console.log("here") emits its call; the last emitExpressionInfo points at line 1, column 9 (console.log).
  2. DeclarationStatement → DestructuringAssignmentNode::emitBytecode runs.
  3. emitNode(initializer, NullNode) → emitLoad(jsNull()). No expression info emitted.
  4. m_bindings->bindValue(generator, rhs) → ObjectPatternNode::bindValue.
  5. emitRequireObjectCoercibleForDestructuring(rhs, &"a") emits OpJnundefinedOrNull (not taken — rhs is null) then OpThrowStaticError.
  6. The throw_static_error op's expression range is still line 1, column 9.
  7. Stack trace: TypeError: Cannot destructure property 'a' from null or undefined value at test.js:1:9 instead 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.

RefPtr<RegisterID> iteratorSymbol = generator.emitGetById(generator.newTemporary(), iterable.get(), generator.propertyNames().iteratorSymbol);
CallArguments args(generator, nullptr, 0);
generator.move(args.thisRegister(), iterable.get());
Expand Down
Loading