-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Reality and spec differ on property key resolution timing for o[p] = f()
#3295
Comments
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". |
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). |
non-browser engines seem to get this correct, would be unfortunate to change this up on them. |
This comment was marked as outdated.
This comment was marked as outdated.
o[p] = f()
o[p] = f()
This comment was marked as outdated.
This comment was marked as outdated.
I've rewritten the original post to incorporate the findings of my previous comment. Should be much clearer now, I hope. 🙇🤞 |
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. |
@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. |
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 Alternatively, we could special-case One way or another, one of these two sentences can't remain true:
|
Okay I've confirmed that it's fine to just loosen the type of |
For more context, @bakkot and I prefer loosening the type of [[ReferencedName]] in Reference Records. |
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: I suppose this is intentional? |
Note that everyone disagrees on the former test:
...but SM and V8 do agree on the latter:
...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)
Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)
Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)
Ensure that the following cases are covered: - tc39/ecma262#3295 (comment) - tc39/ecma262#2659 (comment)
This test for evaluation order of the assignment operator is quite interesting, as no browser-hosted engines pass it.
Specifically, we see the following:
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:
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: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()
:put(getReference(o, p), f())
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: