[es6] Ensure that for-in/of loops have a proper TDZ for their boundnames

[es6] Ensure that for-in/of loops have a proper TDZ for their lexically-bound variables

The enumerable expression in a for-in/of loop is supposed to have a TDZ for any
lexically bound names in that loop (there can be more than one with destructuring).

This patch accomplishes this with an almost-correct desugaring. The only thing missing
is proper debugger support (the let declarations added by the desugaring, while invisible
to code due to shadowing, are visible to the debugger).

BUG=v8:4210
LOG=n

Committed: https://crrev.com/62572e011e73da2669be6e724243405e42304d78
Cr-Commit-Position: refs/heads/master@{#29396}

[adamk, 2015-06-26 21:14:05]

Patchset #1 (id:1) has been deleted

[adamk, 2015-06-26 21:16:03]

adamk@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[adamk, 2015-06-26 21:16:03]

This is ready for review, though I'll need to fix the TODOs about the debugger
before landing.

[arv (Not doing code reviews), 2015-06-29 14:48:44]

LGTM

I think the extra variable visible to the debugger can be done later.

https://codereview.chromium.org/1218543003/diff/40001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1218543003/diff/40001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:735: assertThrows("'use strict'; let x =
{}; for (let [y, x] of {x});", ReferenceError);
What is the motivation for repeating the test?

[adamk, 2015-06-29 18:20:13]

I'm not sure the debugger stuff can wait, I wouldn't want to ship a thing that
showed an extra phantom variable in devtools.

Andreas, any thoughts on me adding some sort of new VariableMode that's lexical
but internal?

https://codereview.chromium.org/1218543003/diff/40001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1218543003/diff/40001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:735: assertThrows("'use strict'; let x =
{}; for (let [y, x] of {x});", ReferenceError);
On 2015/06/29 14:48:43, arv wrote:
> What is the motivation for repeating the test?

To ensure that we're giving all the bound names TDZs, not just the first one.

[rossberg, 2015-06-30 12:27:03]

LGTM

On 2015/06/29 18:20:13, adamk wrote:
> I'm not sure the debugger stuff can wait, I wouldn't want to ship a thing that
> showed an extra phantom variable in devtools.

I'm fine with that. Correctness trumps devtools user experience. If we can still
fix devtools, we should try, though.

> Andreas, any thoughts on me adding some sort of new VariableMode that's
lexical
> but internal?

Sounds plausible, but may be a substantial amount of work.

Instead of having two different sorts of internal variables we could perhaps
make all of them lexical, and just make sure that we allocate the relevant in
the current DeclarationScope (where VAR goes) explicitly. That would avoid any
need to distinguish them downstream?

[adamk, 2015-06-30 21:27:25]

On 2015/06/30 12:27:03, rossberg wrote:
> LGTM
> 
> On 2015/06/29 18:20:13, adamk wrote:
> > I'm not sure the debugger stuff can wait, I wouldn't want to ship a thing
that
> > showed an extra phantom variable in devtools.
> 
> I'm fine with that. Correctness trumps devtools user experience. If we can
still
> fix devtools, we should try, though.
> 
> > Andreas, any thoughts on me adding some sort of new VariableMode that's
> lexical
> > but internal?
> 
> Sounds plausible, but may be a substantial amount of work.
> 
> Instead of having two different sorts of internal variables we could perhaps
> make all of them lexical, and just make sure that we allocate the relevant in
> the current DeclarationScope (where VAR goes) explicitly. That would avoid any
> need to distinguish them downstream?

Just learned how this actually works, and it's crazy:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/scopeinfo.c...

it's just based on starting with '.'!

So I could fix this by putting a bit on variables in ScopeInfo...

[adamk, 2015-06-30 21:32:28]

Patchset #3 (id:60001) has been deleted

[adamk, 2015-06-30 21:52:25]

On 2015/06/30 21:27:25, adamk wrote:
> On 2015/06/30 12:27:03, rossberg wrote:
> > LGTM
> > 
> > On 2015/06/29 18:20:13, adamk wrote:
> > > I'm not sure the debugger stuff can wait, I wouldn't want to ship a thing
> that
> > > showed an extra phantom variable in devtools.
> > 
> > I'm fine with that. Correctness trumps devtools user experience. If we can
> still
> > fix devtools, we should try, though.
> > 
> > > Andreas, any thoughts on me adding some sort of new VariableMode that's
> > lexical
> > > but internal?
> > 
> > Sounds plausible, but may be a substantial amount of work.
> > 
> > Instead of having two different sorts of internal variables we could perhaps
> > make all of them lexical, and just make sure that we allocate the relevant
in
> > the current DeclarationScope (where VAR goes) explicitly. That would avoid
any
> > need to distinguish them downstream?
> 
> Just learned how this actually works, and it's crazy:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/scopeinfo.c...
> 
> it's just based on starting with '.'!
> 
> So I could fix this by putting a bit on variables in ScopeInfo...

Ah, and the reason we don't have a bit for this is that there's no place to put
a bit for stack locals.

[adamk, 2015-06-30 23:55:33]

Okay, I've convinced myself that fixing the debugger here is going to be tricky,
and that there's something _almost_ semantically correct about showing this
extra block scope in the debugger (it represents the state of the loop-declared
variables just outside the loop).

I'll still aim to fix the debugger asap, but I agree that correctness here is
the most important thing (especially because Edge gets this right, and I've
found almost-real code in our unittests which ran afoul of this tdz).

[adamk, 2015-06-30 23:55:41]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-06-30 23:55:42]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1218543003/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2015-06-30 23:55:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218543003/80001

[commit-bot: I haz the power, 2015-07-01 00:27:21]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-07-01 00:27:46]

Patchset 3 (id:??) landed as
https://crrev.com/62572e011e73da2669be6e724243405e42304d78
Cr-Commit-Position: refs/heads/master@{#29396}