Skip to content

Fix ToPropertyKey evaluation order for computed keys - #1651

Open
andreasrosdal wants to merge 6 commits into
quickjs-ng:masterfrom
nordstjernen-web:fix-topropkey-evaluation-order
Open

Fix ToPropertyKey evaluation order for computed keys#1651
andreasrosdal wants to merge 6 commits into
quickjs-ng:masterfrom
nordstjernen-web:fix-topropkey-evaluation-order

Conversation

@andreasrosdal

Copy link
Copy Markdown
Contributor

Two places where a computed property key is converted at the wrong moment relative to the value expression. They pull in opposite directions, which is why they are in one PR.

1. obj[key] = val — convert after val

EvaluatePropertyAccessWithExpressionKey builds a Reference Record holding the raw property-name value and leaves ToPropertyKey to PutValue, with a note spelling out why:

NOTE: In most cases, ToPropertyKey will be performed on propertyNameValue immediately after this step. However, in the case of a[b] = c, it will not be performed until after evaluation of c.

get_lvalue() emits an eager OP_to_propkey2 / OP_to_propkey for OP_get_array_el and OP_get_super_value, so the key's toString() runs first:

const key = {toString() { log.push('key'); return 'k'; }};
const val = {valueOf()  { log.push('val'); return 1;   }};
({})[key] = +val;   // key,val — V8: val,key

js_parse_assign_expr2() already worked around this for OP_get_array_el with a run-time branch on whether the base is nullish, at the cost of converting the key twice on the happy path; the FIXME(bnoordhuis) on it asked for something less elaborate.

Undo the eager conversion right after get_lvalue() and redo it once, right before put_lvalue(), for both opcodes. Destructuring assignment targets get the same treatment, so [super[key]] = v no longer converts early either.

2. {[key]: val} — convert before val

PropertyDefinition : PropertyName : AssignmentExpression evaluates the property name first, and PropertyName : ComputedPropertyName ends in ToPropertyKey:

({[key]: +val});   // val,key — V8: key,val

js_parse_object_literal() leaves the key as-is and lets OP_define_array_el convert it once the value is on the stack. Emit OP_to_propkey right after the : instead; the later conversion then sees a string or symbol and is a no-op.

Testing

Six tests fixed, entries dropped from test262_errors.txt:

  • expressions/assignment/target-member-computed-reference.js
  • expressions/assignment/target-super-computed-reference.js
  • expressions/assignment/destructuring/keyed-destructuring-property-reference-target-evaluation-order.js
  • .../keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js
  • .../iterator-destructuring-property-reference-target-evaluation-order.js
  • expressions/object/computed-property-name-topropertykey-before-value-evaluation.js
suite before after
language/expressions/assignment 5/483 0/483
language/expressions/object 1/1170 0/1170
language/expressions/super 0/94 0/94
language/destructuring 1/19 1/19
language/statements/class 2/4353 2/4353

Still divergent, before and after: undefined[key] = val converts the key before throwing, where V8 rejects the nullish base first. That needs OP_put_array_el to check the base ahead of the conversion — a separate change.

🤖 Generated with Claude Code

https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn


Generated by Claude Code

claude and others added 6 commits August 6, 2026 17:49
EvaluatePropertyAccessWithExpressionKey builds a Reference Record holding
the raw property-name value and leaves ToPropertyKey to PutValue, with a
note spelling out why:

    NOTE: In most cases, ToPropertyKey will be performed on
    propertyNameValue immediately after this step. However, in the case of
    a[b] = c, it will not be performed until after evaluation of c.

get_lvalue() emits an eager OP_to_propkey2 / OP_to_propkey for
OP_get_array_el and OP_get_super_value, so the key's toString() runs before
the right-hand side:

    const key = {toString() { log.push('key'); return 'k'; }};
    const val = {valueOf()  { log.push('val'); return 1;   }};
    ({})[key] = +val;   // key,val -- V8: val,key

js_parse_assign_expr2() already worked around this for OP_get_array_el with
a run-time branch on whether the base is nullish, at the cost of converting
the key twice on the happy path; the FIXME on it asked for something less
elaborate.

Undo the eager conversion right after get_lvalue() and redo it once, right
before put_lvalue(), for both opcodes. Destructuring assignment targets get
the same treatment, so `[super[key]] = v` no longer converts early either.

Still divergent, before and after: `undefined[key] = val` converts the key
before throwing, where V8 rejects the nullish base first. That needs
OP_put_array_el to check the base ahead of the conversion, which is a
separate change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
PropertyDefinition : PropertyName `:` AssignmentExpression evaluates the
property name first, and PropertyName : ComputedPropertyName ends in
ToPropertyKey, so a computed key's toString() runs before the value
expression:

    const key = {toString() { log.push('key'); return 'k'; }};
    const val = {valueOf()  { log.push('val'); return 1;   }};
    ({[key]: +val});   // val,key -- V8: key,val

js_parse_object_literal() leaves the key as-is and lets OP_define_array_el
convert it once the value is on the stack. Emit OP_to_propkey right after
the ':' instead; the later conversion in OP_define_array_el then sees a
string or symbol and is a no-op.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Pins the order for both directions: a property reference (obj[key] = val,
super[key] = val, and every computed destructuring target) converts the key
after the assigned value, while a computed key in an object literal
converts before its value.

Also covers the cases that fall out of that: a throwing value means the key
is never converted, a throwing conversion still runs after the value, and
the key is converted exactly once even in the read-modify-write forms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
keyed-destructuring-property-reference-target-evaluation-order-with-bindings
was dropped from the expected failures, but it still fails with exactly the
message recorded there: it covers a *binding* pattern (var {[k]: t = d} = s),
whose target is resolved through js_parse_destructuring_var rather than
get_lvalue, so the deferred conversion does not reach it. Its assignment
pattern namesake, which does go through get_lvalue, is fixed and stays out.

Without this the full test262 run reports one new error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
The conversion now happens at the store, so it is worth pinning where that
is for the shapes with more than one of them. Added several computed
targets in one pattern, a nested pattern, a computed target in an array and
an object for-of head which is re-evaluated every iteration, the same key
object used for both the source and the target, a target whose setter runs
user code, and a default value, which is evaluated before the key it is
stored under.
Deferring ToPropertyKey until the value is evaluated drops the stack
shuffling the old order needed, so qjsc emits shorter bytecode for every
computed key. The precompiled sources in the tree still hold the longer
form, which makes the codegen CI job fail on a dirty tree:

    Dirty git tree
    Binary files a/builtin-iterator-zip-keyed.h and b/... differ
    Binary files a/builtin-iterator-zip.h and b/... differ
    Binary files a/gen/repl.c and b/gen/repl.c differ

Regenerate them with "make codegen". Only the three inputs that contain a
computed key change; the output is byte-for-byte reproducible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants