889628 - Destructuring assignment doesn't enforce strict-mode assignment rules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

"use strict";
[eval] = [0];  // allowed, should be SyntaxError

Comment 1

[Jason Orendorff [:jorendorff]]

Not coincidentally, bindDestructuringLHS does not bind anything and has no reason to exist. It should be merged with checkAndMarkAsAssignmentLhs(), which will fix this bug.

Comment 2

[André Bargull [:anba]]

Per the current ES6 draft, `[eval] = [0]` is always a syntax error regardless of the strict mode setting. In general the draft makes "eval" and "arguments" invalid identifier names in all new syntax productions.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Preliminaries

Fold setLvalKid into its only caller, setIncOpKid.

Rename setIncOpKid to checkAndMarkAsIncOperand.

Rename setAssignmentLhsOps to checkAndMarkAsAssignmentLhs.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Destructuring opcode selection

Do not look at lhs->getOp() when selecting opcodes for destructuring assignment, except if lhs is a name and we just called BindNameToSlot.

(That is, looking at getOp() is OK if it was set by BindNameToSlot which after all is part of the emitter.  But it's not the parser's job to select opcodes.)

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Don't even set the pn_op field of PNK_DOT/ELEM nodes

Now that we're not reading them, don't write them either.

Deja vu? We already did this for all the JSOP_GETPROP/GETELEM cases; now do the same for SETPROP/SETELEM cases.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - Remove another redundant BindNameToSlot call.

The very next line calls EmitTree(), which does that as part of emitting the PNK_NAME node. Sheesh.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Fix the bug

Actually fix the bug, implementing the ES6 rules about assigning to eval and arguments (which is to say, don't do that).

The patch is mostly error handling. I'm sad it turned out so inelegant; it was prettier in my head.

Delightfully, 4 tests had to be updated.

Comment 8

[Jason Orendorff [:jorendorff]]

André, thanks for the note! I was unaware of that and would have missed the chance to implement the draft semantics.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 771082 [details] [diff] [review]
Part 1 - Preliminaries

Review of attachment 771082 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 771096 [details] [diff] [review]
Part 5 - Fix the bug

Review of attachment 771096 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +788,3 @@
>          return true;
>  
>      JSAtom *atom = handler.isName(lhs);

This is kind of scary to me, relying on lastAtom being the name here.  :-\  Yay for temporal restrictions on when methods can be called.  Oh well, pre-existing.

::: js/src/frontend/Parser.h
@@ +483,5 @@
>      }
>  
> +    enum AssignmentFlavor {
> +        PlainAssignment,
> +        AssignmentWithOperation,

CompoundAssignment would be a more spec-ful name, and I think clearer too.

::: js/src/jit-test/tests/parser/bug-889628.js
@@ +9,5 @@
> +    "[[], [{}, [_]]]",
> +    "{x:_}",
> +    "{x:y, z:_}",
> +    "{0:_}",
> +    "{_}"

Do ES6 destructuring patterns include "[..._]" as a thing?  Add that if they do.  (If they do but we don't support it, add some sort of canary test that'll trip when we do.)

Comment 11

[Rick Waldron [:rwaldron]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)

> Do ES6 destructuring patterns include "[..._]" as a thing?  Add that if they
> do.  (If they do but we don't support it, add some sort of canary test
> that'll trip when we do.)

To confirm: yes, destructuring includes spread operator support (11.13.1 of latest spec draft):

var [..._] = [1,2,3];

console.log(_); // [1,2,3]


Hope this was helpful!

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> ::: js/src/frontend/Parser.cpp
> >      JSAtom *atom = handler.isName(lhs);
> 
> This is kind of scary to me, relying on lastAtom being the name here.  :-\ 
> Yay for temporal restrictions on when methods can be called.  Oh well,
> pre-existing.

Yeah. Maybe we could fix this whole class of things by requiring the ParseHandler::Node type to store at least the ParseNodeKind plus a few more bits.

The parser could eagerly set those bits to indicate stuff that we now discover lazily by querying the ParseNodes in more complex ways, like the distinction we're making in this code (i.e. whether or not a given PNK_NAME node is the identifier 'eval' or 'arguments'). Or: whether or not a given PNK_ARRAY node is a valid destructuring lhs.

I dunno. What do you think?

> CompoundAssignment would be a more spec-ful name, and I think clearer too.

Excellent! Changed.

> Do ES6 destructuring patterns include "[..._]" as a thing?  Add that if they
> do.  (If they do but we don't support it, add some sort of canary test
> that'll trip when we do.)

Added:

>     "{0:_}",
>-    "{_}"
>+    "{_}",
>+    //"[..._]"
> ];
> 
>+// If the assertion below fails, congratulations! It means you have added
>+// spread operator support to destructuring assignment. Simply uncomment the
>+// "[..._]" case above. Then delete this comment and assertion.
>+assertThrowsInstanceOf(() => Function("[...x] = [1]"), ReferenceError);
>+

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Yeah. Maybe we could fix this whole class of things by requiring the
> ParseHandler::Node type to store at least the ParseNodeKind plus a few more
> bits.
> 
> The parser could eagerly set those bits to indicate stuff that we now
> discover lazily by querying the ParseNodes in more complex ways, like the
> distinction we're making in this code (i.e. whether or not a given PNK_NAME
> node is the identifier 'eval' or 'arguments'). Or: whether or not a given
> PNK_ARRAY node is a valid destructuring lhs.
> 
> I dunno. What do you think?

Plausible.  The Node type could be some sort of tagged struct with fields mutated only when necessary, and a minimal operator=, to preserve speed.  Although, we'd probably want a field for PropertyName* here, but that being a GC thing might cause complications clobbering them (wrt write barriers).  (I'm gonna link-drop http://www.youtube.com/watch?v=1KQ4t9rSNoA because it's on my mind now.)

> >+// If the assertion below fails, congratulations! It means you have added
> >+// spread operator support to destructuring assignment. Simply uncomment the
> >+// "[..._]" case above. Then delete this comment and assertion.
> >+assertThrowsInstanceOf(() => Function("[...x] = [1]"), ReferenceError);

Hm.  |x = 7| creates a new global property with value 7.  Does |[x] = [1]| create a new global with value 1?  If so, why not |[...x] = [42]| with value |[42]|?  Unless I'm being dumb, this canary's dead, Jim.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/26f5a13acac8
https://hg.mozilla.org/mozilla-central/rev/eaa441be57f3
https://hg.mozilla.org/mozilla-central/rev/f102b444db08
https://hg.mozilla.org/mozilla-central/rev/fa4fd499678c
https://hg.mozilla.org/mozilla-central/rev/a0684871506a