Closed Bug 889628 Opened 7 years ago Closed 7 years ago

Destructuring assignment doesn't enforce strict-mode assignment rules

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(5 files)

"use strict";
[eval] = [0];  // allowed, should be SyntaxError
Not coincidentally, bindDestructuringLHS does not bind anything and has no reason to exist. It should be merged with checkAndMarkAsAssignmentLhs(), which will fix this bug.
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.
Fold setLvalKid into its only caller, setIncOpKid.

Rename setIncOpKid to checkAndMarkAsIncOperand.

Rename setAssignmentLhsOps to checkAndMarkAsAssignmentLhs.
Assignee: general → jorendorff
Attachment #771082 - Flags: review?(jwalden+bmo)
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.)
Attachment #771084 - Flags: review?(jwalden+bmo)
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.
Attachment #771086 - Flags: review?(jwalden+bmo)
The very next line calls EmitTree(), which does that as part of emitting the PNK_NAME node. Sheesh.
Attachment #771091 - Flags: review?(jwalden+bmo)
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.
Attachment #771096 - Flags: review?(jwalden+bmo)
André, thanks for the note! I was unaware of that and would have missed the chance to implement the draft semantics.
Comment on attachment 771082 [details] [diff] [review]
Part 1 - Preliminaries

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

Nice.
Attachment #771082 - Flags: review?(jwalden+bmo) → review+
Attachment #771084 - Flags: review?(jwalden+bmo) → review+
Attachment #771086 - Flags: review?(jwalden+bmo) → review+
Attachment #771091 - Flags: review?(jwalden+bmo) → review+
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.)
Attachment #771096 - Flags: review?(jwalden+bmo) → review+
(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!
(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);
>+
(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.
You need to log in before you can comment on or make changes to this bug.