Skip to content

Normative: new Annex B.3 clause to specify "AssignmentTargetType of LeftHandSideExpression : CallExpression" web reality. #2193

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

Closed
wants to merge 2 commits into from

Conversation

rwaldron
Copy link
Contributor

The intention of this change is to allow JS engines which run in browsers to be spec conformant in their handling of assignment to CallExpression Arguments. Both Chrome and Firefox have attempted to make the following a Syntax Error:

f() = 1

...However they were unable to do so without breaking the web! As it turns out, every engine that runs in a web browser has implemented the same behavior:

$ eshost -x 'let f = () => {}; f() = 1'
#### ChakraCore

ReferenceError: Invalid left-hand side in assignment

#### JavaScriptCore

ReferenceError: Left side of assignment is not a reference.

#### SpiderMonkey

ReferenceError: cannot assign to function call

#### V8

ReferenceError: Invalid left-hand side in assignment

Note that non-browser embedded engines, which do not bear the same burden as the above engines, implement this according to the current specification:

$ eshost -x 'let f = () => {}; f() = 1' --no-color
#### engine262

SyntaxError: Invalid assignment target

#### graaljs

SyntaxError: Invalid left hand side for assignment

#### hermes

SyntaxError: invalid assignment left-hand side

#### Moddable XS

SyntaxError: no reference

#### qjs

SyntaxError: invalid assignment left-hand side

I'd like to propose that TC39 codify the web reality. If there is a better way to define this, please advise!

cc @syg @jorendorff

@rwaldron rwaldron added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. web reality labels Sep 25, 2020
@bakkot
Copy link
Contributor

bakkot commented Sep 25, 2020

See older discussion in #257.

Note that both SpiderMonkey and JavaScriptCore make this an early error in strict code, so it should be safe to make this legal only in non-strict code. V8 has use counters for this pattern in both strict and non-strict code, which show almost no usage in strict code and a nontrivial amount of usage in non-strict code.

@syg
Copy link
Contributor

syg commented Sep 25, 2020

I agree with @bakkot in #2193 (comment). Seems like a good idea to clean this up for strict, but too risky for non-strict.

@devsnek
Copy link
Member

devsnek commented Sep 25, 2020

Do we instead want to put non-strict in the main body with an "icky" note?

@rwaldron rwaldron force-pushed the annexb-lhs-callexp-assignment branch from 6f80175 to 4db9963 Compare October 5, 2020 15:01
@rwaldron
Copy link
Contributor Author

rwaldron commented Oct 5, 2020

I've added the limitation to non-strict code, however I'm not sure the language is exactly right.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 8, 2020

The Early Errors in https://tc39.es/ecma262/#sec-assignment-operators-static-semantics-early-errors also need to be modified by Annex B to allow CallExpression in sloppy mode.

@devsnek

This comment was marked as duplicate.

@littledan
Copy link
Member

Thanks for following up with this PR, @rwaldron . It's too bad we all dropped it on the floor earlier. LGTM in its current form.

Note that this version does not include logical assignment operators, and as such, V8 is banning this construct. (Apparently there are already test262 tests for this.) https://chromium-review.googlesource.com/c/v8/v8/+/2423721 . This prohibition seems appropriate to me, since it's not needed for compatibility.

@rkirsling
Copy link
Member

Hmm, I always feel like it's better not to make an already-unfortunate rule more complicated than it has to be. We can certainly get away with allowing f() *= and banning f() &&= at parse time, but this requires awareness of &&= being a recent feature, in spite of the fact that it could have easily been added to the spec many years ago.

@bakkot
Copy link
Contributor

bakkot commented Oct 26, 2020

this requires awareness

Awareness by whom?

@rkirsling
Copy link
Member

Spec readers, I guess? I understand that users won't write this, and if they do they'll get an error sooner or later, but it seems odd to create two special rules when one will do—no one seems to benefit from the added complexity.

@rkirsling
Copy link
Member

Whew, four years have gone by. Am I correct in understanding that this is still what we'd want, it was just never presented in plenary?

(I guess I was the most recent commenter here but I honestly don't feel that strongly about what I said at that time.)

@rkirsling
Copy link
Member

Okay, lemme state that I'm happy to champion this, though I imagine I'll need to do so in a new PR.
(May as well keep using this one in lieu of an issue until then though.)

@michaelficarra
Copy link
Member

@rkirsling You should follow the conventions being established in #2952.

@rkirsling
Copy link
Member

Hmm, want to make note of an interesting thing here. Although browsers are in alignment about throwing a ReferenceError at runtime, SM/V8 perform the function call first, while JSC does not.

function f() { print("hi"); }

try { f() *= 1; } catch {}
try { f()++; } catch {}
try { for (f() of [1,2,3]) {} } catch {}
#### JavaScriptCore


#### SpiderMonkey, V8
hi
hi
hi

I think SM/V8 should be considered correct though, since presumably we want the ReferenceError to come from step 1 of PutValue.

@rkirsling
Copy link
Member

Closing this PR now that #3568 is active.

@rkirsling rkirsling closed this Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants