Reality and spec differ on property key resolution timing for `o[p] = f()` #3295

rkirsling · 2024-03-08T09:10:08Z

This test for evaluation order of the assignment operator is quite interesting, as no browser-hosted engines pass it.

Specifically, we see the following:

var o = {};
var p = { toString() { print('p'); return 3; } };
var f = () => { print('f'); return 4; };
o[p] = f();

#### JavaScriptCore, SpiderMonkey, V8
f
p

But the above is a bit misleading. It's not really the case that engines aren't evaluating subexpressions from left to right; what actually differs is the timing of property key resolution:

var o = {};
var p = { toString() { print('resolve property key'); return 3; } };
var f = () => { print('eval rhs'); return 4; };
var pf = () => { print('eval lhs subscript'); return p; };

o[pf()] = f();

#### JavaScriptCore, SpiderMonkey, V8
eval lhs subscript
eval rhs
resolve property key

That is, engines perform ToPropertyKey as part of (get-by-value and) put-by-value, whereas the spec expects it to be performed as part of LHS evaluation.

For comparison, if we change the expression to o[pf()] += f(), we get:

#### V8
eval lhs subscript
resolve property key
eval rhs
resolve property key

#### SpiderMonkey
eval lhs subscript
resolve property key
eval rhs

#### JavaScriptCore
<omitted; we currently match V8 but will match SM soon>

Again, from the engine perspective, property key resolution is part of get- and put-by-value operations, so if we approach this naively, it'll happen twice. To fix this, we front (RequireObjectCoercible and) ToPropertyKey, such that get- and put-by-value ops can be provided with a pre-resolved property key. Regardless, we can consider reality and spec in alignment for "compound" assignment, because the get-by-value precedes RHS evaluation.

To summarize, when encountering the expression o[p] = f():

The spec effectively sees put(getReference(o, p), f())
Browser-hosted engines effectively see put(o, p, f())

And I'm not sure that I'd call either of these "unintuitive" per se! But I do think the most important question is whether the current spec is web compatible—if it is, it's probably most sensible not to change it.

The text was updated successfully, but these errors were encountered:

bergus · 2024-03-08T13:36:33Z

I'd be very hesitant to change the evaluation order in the spec, introducing an inconsistency in the standard left-to-right order observed everywhere else. It would make the language harder to understand and learn.

I doubt this is a "web-reality issue" as in "websites would break if browser engines decided to fix this bug in their implementations".

rkirsling · 2024-03-08T14:15:20Z

I mean, it's 100% reality that this has been the behavior that JS users could consistently rely upon; whether it's compatible to overturn it is an open question (which seems worth finding an answer to).

devsnek · 2024-03-08T17:08:14Z

non-browser engines seem to get this correct, would be unfortunate to change this up on them.

rkirsling · 2024-03-11T07:58:05Z

I've rewritten the original post to incorporate the findings of my previous comment. Should be much clearer now, I hope. 🙇🤞

iainireland · 2024-03-11T17:17:37Z

From an implementation perspective, resolving the property key as part of the put-by-value allows us to represent them as a single operation, instead of splitting them up. It isn't likely to matter in higher tiers, but the current browser behaviour is likely a little bit faster in interpreters / baseline compilers. All else being equal, I think SpiderMonkey would lean towards updating the spec to match web reality.

rkirsling · 2024-03-18T02:50:27Z

@syg Any thoughts from V8 here?

syg · 2024-03-20T23:10:23Z

@syg Any thoughts from V8 here?

+1 to @iainireland's comment above. Would prefer spec to match engine reality. The V8 bug for compound assignment is a longstanding one, and we (V8) should fix it, though I can't promise it'll be high priority to do so.

rkirsling · 2024-03-21T09:18:24Z

Thanks for the feedback! I guess the question then is how we'd want the proposed spec change to look.

Basically, we need to pull ToPropertyKey out of EvaluatePropertyAccessWithExpressionKey. But if we literally do just that, it seems we'll need to redefine what a Reference Record is, such that it's always able to hold a value which has yet to be resolved to a property key...even though this is only needed for = itself.

Alternatively, we could special-case Evaluation of = such that in the non-destructuring, non-"named anonymous function" path, we perform some new "SpecialEvaluation" of LeftHandSideExpression.

One way or another, one of these two sentences can't remain true:

... For example, the left-hand operand of an assignment is expected to produce a Reference Record.

A Reference Record is a resolved name or property binding; ...

michaelficarra · 2024-03-28T18:52:11Z

Okay I've confirmed that it's fine to just loosen the type of [[ReferencedName]] in Reference Records and then unconditionally force the value everywhere it's accessed. It doesn't introduce any "interesting" new user code annotation points. I still think it's cleaner to have a separate kind of Reference Record (or a boolean) to track whether the value has been forced to a property key, but either is fine.

syg · 2024-03-28T20:42:19Z

For more context, @bakkot and I prefer loosening the type of [[ReferencedName]] in Reference Records.

woess · 2024-04-16T02:14:00Z

I think this change (i.e. #3307), by side effect, also affects destructuring assignments. Namely, I believe the following test262 tests would have to be updated to reflect the new evaluation order:

i.e. for 1. (and similarly for 2.):

({[sourceKey()]: target()[targetKey()]} = source());

evaluation order would change from:
source, source-key, source-key-tostring, target, target-key, target-key-tostring, get, set
to:
source, source-key, source-key-tostring, target, target-key, get, target-key-tostring, set
due to ToPropertyKey now being evaluated as part of 6. Return ? PutValue(lref, rhsValue).

I suppose this is intentional? ~~since it appears to match web reality (for V8 and SM at least)~~.
If you agree, please update these test262 tests, too.

rkirsling · 2024-04-17T07:30:02Z

Note that everyone disagrees on the former test:

#### expected
source,source-key,source-key-tostring,target,target-key,target-key-tostring,get,set

#### JavaScriptCore
source,source-key,source-key-tostring,get,target,target-key,target-key-tostring,set

#### SpiderMonkey
source,target,target-key,source-key,source-key-tostring,get,target-key-tostring,set

#### V8
source,source-key,source-key-tostring,target,target-key,get,target-key-tostring,set

...but SM and V8 do agree on the latter:

#### expected
source,iterator,target,target-key,target-key-tostring,iterator-step,iterator-done,set

#### JavaScriptCore
source,iterator,iterator-step,iterator-done,target,target-key,target-key-tostring,set

#### SpiderMonkey, V8
source,iterator,target,target-key,iterator-step,iterator-done,target-key-tostring,set

...and I do think you're correct that V8's behavior should be the newly expected one.

Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)

rkirsling added the web reality label Mar 8, 2024

This comment was marked as outdated.

Sign in to view

rkirsling changed the title ~~Reality and spec differ on eval order of o[p] = f()~~ Reality and spec differ on property key resolution timing for o[p] = f() Mar 9, 2024

This comment was marked as outdated.

Sign in to view

bakkot added the editor call to be discussed in the next editor call label Mar 26, 2024

rkirsling linked a pull request Mar 29, 2024 that will close this issue

Normative: Allow [[ReferencedName]] in Reference Records to be a not-yet-resolved property key #3307

Open

bakkot removed the editor call to be discussed in the next editor call label Apr 2, 2024

ptomato mentioned this issue Apr 15, 2024

Is null[{ toString: () => { console.log('hit') } }] supposed to hit the console.log? #2659

Open

bakkot added the editor call to be discussed in the next editor call label Apr 15, 2024

This was referenced Apr 15, 2024

Patch tests with new evaluation order for member assignment tc39/test262#3824

Closed

Fix value property assignment order boa-dev/boa#2903

Closed

rkirsling added a commit to rkirsling/test262 that referenced this issue Apr 18, 2024

Add additional tests for tc39/ecma262#3307.

953ad44

Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)

rkirsling mentioned this issue Apr 18, 2024

Add additional tests for tc39/ecma262#3307. tc39/test262#4060

Merged

rkirsling added a commit to rkirsling/test262 that referenced this issue Apr 18, 2024

Add additional tests for tc39/ecma262#3307.

6da4659

Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)

rkirsling added a commit to rkirsling/test262 that referenced this issue Apr 18, 2024

Add additional tests for tc39/ecma262#3307.

c8421c4

Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)

ptomato pushed a commit to tc39/test262 that referenced this issue Apr 22, 2024

Add additional tests for tc39/ecma262#3307.

007b333

Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)

woess mentioned this issue Apr 22, 2024

Incorrect tests after change to evaluation order for member assignment tc39/test262#3407

Open

bakkot removed the editor call to be discussed in the next editor call label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reality and spec differ on property key resolution timing for `o[p] = f()` #3295

Reality and spec differ on property key resolution timing for `o[p] = f()` #3295

rkirsling commented Mar 8, 2024 •

edited

bergus commented Mar 8, 2024 •

edited

rkirsling commented Mar 8, 2024

devsnek commented Mar 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

rkirsling commented Mar 11, 2024

iainireland commented Mar 11, 2024

rkirsling commented Mar 18, 2024

syg commented Mar 20, 2024

rkirsling commented Mar 21, 2024

michaelficarra commented Mar 28, 2024

syg commented Mar 28, 2024

woess commented Apr 16, 2024 •

edited

rkirsling commented Apr 17, 2024

Reality and spec differ on property key resolution timing for o[p] = f() #3295

Reality and spec differ on property key resolution timing for o[p] = f() #3295

Comments

rkirsling commented Mar 8, 2024 • edited

bergus commented Mar 8, 2024 • edited

rkirsling commented Mar 8, 2024

devsnek commented Mar 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

rkirsling commented Mar 11, 2024

iainireland commented Mar 11, 2024

rkirsling commented Mar 18, 2024

syg commented Mar 20, 2024

rkirsling commented Mar 21, 2024

michaelficarra commented Mar 28, 2024

syg commented Mar 28, 2024

woess commented Apr 16, 2024 • edited

rkirsling commented Apr 17, 2024

Reality and spec differ on property key resolution timing for `o[p] = f()` #3295

Reality and spec differ on property key resolution timing for `o[p] = f()` #3295

rkirsling commented Mar 8, 2024 •

edited

bergus commented Mar 8, 2024 •

edited

woess commented Apr 16, 2024 •

edited