[es6] Check declaration conflicts between non-simple parameters and the function body

[es6] Check declaration conflicts between non-simple parameters and the function body

Also, more tests for parameters containing functions or eval or both.

R=adamk@chromium.org, caitpotter88@gmail.com, littledan@chromium.org
BUG=v8:811
LOG=N

Committed: https://crrev.com/fa94ca3ea422bbdc5a2b0d2ad42f7438456a95a9
Cr-Commit-Position: refs/heads/master@{#29752}

[rossberg, 2015-07-15 13:25:14]

Patchset #1 (id:1) has been deleted

[adamk, 2015-07-15 21:12:30]

https://codereview.chromium.org/1225413005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/parser.cc#newcode4435
src/parser.cc:4435: if (inner_scope)
CheckConflictingVarDeclarations(inner_scope, CHECK_OK);
CHECK_OK shouldn't be used in a single-statement if like this (even if it would
turn out to do the right thing). Better to add a block here.

You could also guard the above inner_block->set_scope() call with this check
while you're at it. (and nit: change the condition to inner_scope != nullptr to
match surrounding code)

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc#newcode582
src/scopes.cc:582: // If the declaration scope is a (declaration) block scope,
also continue
This weird duality of "declaration" scopes is beginning to worry me. Do you have
reason to think there aren't more places that depend on there being a single
declaration scope for a given scope?

[rossberg, 2015-07-16 14:26:40]

https://codereview.chromium.org/1225413005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/parser.cc#newcode4435
src/parser.cc:4435: if (inner_scope)
CheckConflictingVarDeclarations(inner_scope, CHECK_OK);
On 2015/07/15 21:12:30, adamk wrote:
> CHECK_OK shouldn't be used in a single-statement if like this (even if it
would
> turn out to do the right thing). Better to add a block here.
> 
> You could also guard the above inner_block->set_scope() call with this check
> while you're at it. (and nit: change the condition to inner_scope != nullptr
to
> match surrounding code)

Done.

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc#newcode582
src/scopes.cc:582: // If the declaration scope is a (declaration) block scope,
also continue
On 2015/07/15 21:12:30, adamk wrote:
> This weird duality of "declaration" scopes is beginning to worry me. Do you
have
> reason to think there aren't more places that depend on there being a single
> declaration scope for a given scope?

I admit this is hacky, but I'm not sure I fully understand your question. There
should always be a single declaration scope. But in this particular case, we
also want to check that it doesn't overlap with the surrounding one.

[adamk, 2015-07-16 17:05:14]

lgtm

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc#newcode582
src/scopes.cc:582: // If the declaration scope is a (declaration) block scope,
also continue
On 2015/07/16 14:26:40, rossberg wrote:
> On 2015/07/15 21:12:30, adamk wrote:
> > This weird duality of "declaration" scopes is beginning to worry me. Do you
> have
> > reason to think there aren't more places that depend on there being a single
> > declaration scope for a given scope?
> 
> I admit this is hacky, but I'm not sure I fully understand your question.
There
> should always be a single declaration scope. But in this particular case, we
> also want to check that it doesn't overlap with the surrounding one.

What I meant was that functions now can have two declaration scopes, where
previously they only had one.

[rossberg, 2015-07-17 08:19:46]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-07-17 08:19:56]

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

[commit-bot: I haz the power, 2015-07-17 08:20:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 08:20:54]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/7753)
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/6790)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

(exceeded global retry quota)

[rossberg, 2015-07-17 08:27:41]

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1225413005/diff/20001/src/scopes.cc#newcode582
src/scopes.cc:582: // If the declaration scope is a (declaration) block scope,
also continue
On 2015/07/16 17:05:13, adamk wrote:
> On 2015/07/16 14:26:40, rossberg wrote:
> > On 2015/07/15 21:12:30, adamk wrote:
> > > This weird duality of "declaration" scopes is beginning to worry me. Do
you
> > have
> > > reason to think there aren't more places that depend on there being a
single
> > > declaration scope for a given scope?
> > 
> > I admit this is hacky, but I'm not sure I fully understand your question.
> There
> > should always be a single declaration scope. But in this particular case, we
> > also want to check that it doesn't overlap with the surrounding one.
> 
> What I meant was that functions now can have two declaration scopes, where
> previously they only had one.

Ah, I see. No, I don't think it matters anywhere else. The notion of declaration
scope is irrelevant for anything but placing var declarations during parsing.
And this parameters-may-not-shadow-local-decls thing is an unnatural
irregularity in the spec that shouldn't exist in the first place (certainly not
for lexical bindings).

[rossberg, 2015-07-20 13:25:36]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-07-20 13:25:37]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1225413005/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-20 13:25:41]

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

[commit-bot: I haz the power, 2015-07-20 13:49:02]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-07-20 13:49:24]

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

[caitp (gmail), 2015-09-02 02:16:17]

On 2015/07/20 13:49:24, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/fa94ca3ea422bbdc5a2b0d2ad42f7438456a95a9
> Cr-Commit-Position: refs/heads/master@{#29752}

While debugging some failures (from https://codereview.chromium.org/1272673003/)
in tests added by this CL (particularly `f34()` --- probably others), I found a
problem:

We're lazily compiling the default parameter initializer function, which becomes
totally separated from the parameter scope where `...a` gets declared. This
works great for eager compilation, but not so great for lazy compilation.

Just to test that this wasn't caused by a problem in my CL, I tried an
experiment on master (29d793728abf24d7078809f128d3f82437855ff2) with no patches
applied: I tried removing the `...` from the rest parameter, making it a simple
parameter. It hangs the process.

Simple repro case:

``` test.js
function f({x = function defaultFn() { return a }}, a) { return x()[0] }
f({})
```

``` runner
out/x64.release/d8 --harmony-destructuring --harmony-default-parameters test.js
```

So, even if the rest parameter issue is my bug, I think it's at least partially
related to this simple parameter version of the bug

[caitp (gmail), 2015-09-02 04:36:54]

On 2015/09/02 02:16:17, caitp wrote:
> On 2015/07/20 13:49:24, commit-bot: I haz the power wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/fa94ca3ea422bbdc5a2b0d2ad42f7438456a95a9
> > Cr-Commit-Position: refs/heads/master@{#29752}
> 
> While debugging some failures (from
https://codereview.chromium.org/1272673003/)
> in tests added by this CL (particularly `f34()` --- probably others), I found
a
> problem:
> 
> We're lazily compiling the default parameter initializer function, which
becomes
> totally separated from the parameter scope where `...a` gets declared. This
> works great for eager compilation, but not so great for lazy compilation.
> 
> Just to test that this wasn't caused by a problem in my CL, I tried an
> experiment on master (29d793728abf24d7078809f128d3f82437855ff2) with no
patches
> applied: I tried removing the `...` from the rest parameter, making it a
simple
> parameter. It hangs the process.
> 
> Simple repro case:
> 
> ``` test.js
> function f({x = function defaultFn() { return a }}, a) { return x()[0] }
> f({})
> ```
> 
> ``` runner
> out/x64.release/d8 --harmony-destructuring --harmony-default-parameters
test.js
> ```
> 
> So, even if the rest parameter issue is my bug, I think it's at least
partially
> related to this simple parameter version of the bug

hmmm, nevermind this --- I think the hanging was a local system problem of some
kind maybe, I can't reproduce it now