[es6] Scoping & initialization for var shadowing non-simple parameters

[es6] Scoping & initialization for var shadowing non-simple parameters

Var-bindings may shadow parameters from a non-simple parameter list. When that happens: they create separate bindings, but are initialised with the respective parameter value. Thus:

(function(x, f = () => x) { var x; var y = x; x = 2; return [x, y, f()] })(1) -->  [2, 1, 1]

This CL implements that by inserting a suitable assignment for every shadwowing var-variable (e.g., x = outer_x above) at the beginning of the function's body block.

R=adamk@chromium.org
BUG=v8:4440, v8:811
LOG=N

Committed: https://crrev.com/2c2848dccf697830dab75f59a2b7a72af48cbc48
Cr-Commit-Position: refs/heads/master@{#31042}

[adamk, 2015-09-29 18:28:29]

Please add more detail to the CL description: what does the patch change, and
why (for future generations of code archaeologists)?

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

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4956
src/parser.cc:4956: Scope* scope = block->scope();
Can you give this a clearer name than "scope"? "body_scope", maybe?

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4958
src/parser.cc:4958: Scope* function_scope = scope->outer_scope();
And this would be "parameter_scope"? Otherwise, to the untrained eye, the below
looks funny, as the "from" variable comes from the "function scope".

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4968
src/parser.cc:4968: VariableProxy* from =
function_scope->NewUnresolved(factory(), name);
Given that you already have the parameter Variable* handy, you could just create
a NewVariableProxy here directly, rather than going through the scope.

https://codereview.chromium.org/1371333004/diff/20001/src/scopes.cc
File src/scopes.cc (left):

https://codereview.chromium.org/1371333004/diff/20001/src/scopes.cc#oldcode623
src/scopes.cc:623: } while (!previous->is_declaration_scope() ||
previous->is_block_scope());
Is this what makes function declarations work properly?

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

https://codereview.chromium.org/1371333004/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:963: (function TestShadowingOfParameters()
{
Can you add tests with multiple parameters that get matching vars? Also a test
where both destructured and simple parameters get matching vars.

[rossberg, 2015-09-30 14:50:49]

Addresed comments and extended CL description.

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

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4956
src/parser.cc:4956: Scope* scope = block->scope();
On 2015/09/29 18:28:29, adamk wrote:
> Can you give this a clearer name than "scope"? "body_scope", maybe?

Done.

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4958
src/parser.cc:4958: Scope* function_scope = scope->outer_scope();
On 2015/09/29 18:28:29, adamk wrote:
> And this would be "parameter_scope"? Otherwise, to the untrained eye, the
below
> looks funny, as the "from" variable comes from the "function scope".

I elsewhere used parameter_scope for the (yet different) scope that has to be
wrapped around individual parameters, so that would be more confusing IMO.

I use function_scope vs inner_scope now, which (kind of) matches with the naming
in the caller ParseEagerFunctionBody.

https://codereview.chromium.org/1371333004/diff/20001/src/parser.cc#newcode4968
src/parser.cc:4968: VariableProxy* from =
function_scope->NewUnresolved(factory(), name);
On 2015/09/29 18:28:29, adamk wrote:
> Given that you already have the parameter Variable* handy, you could just
create
> a NewVariableProxy here directly, rather than going through the scope.

Done.

https://codereview.chromium.org/1371333004/diff/20001/src/scopes.cc
File src/scopes.cc (left):

https://codereview.chromium.org/1371333004/diff/20001/src/scopes.cc#oldcode623
src/scopes.cc:623: } while (!previous->is_declaration_scope() ||
previous->is_block_scope());
On 2015/09/29 18:28:29, adamk wrote:
> Is this what makes function declarations work properly?

No, this is what prevents the previous behaviour of flagging an error. Function
declarations are excluded by the condition in parser.cc:4963.

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

https://codereview.chromium.org/1371333004/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:963: (function TestShadowingOfParameters()
{
On 2015/09/29 18:28:29, adamk wrote:
> Can you add tests with multiple parameters that get matching vars? Also a test
> where both destructured and simple parameters get matching vars.

Done.

[adamk, 2015-09-30 18:54:38]

lgtm

[rossberg, 2015-10-01 10:19:26]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-10-01 10:19:38]

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

[commit-bot: I haz the power, 2015-10-01 10:22:26]

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

[commit-bot: I haz the power, 2015-10-01 10:22:27]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6257)

[rossberg, 2015-10-01 10:25:00]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-10-01 10:25:06]

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

[commit-bot: I haz the power, 2015-10-01 10:42:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-01 10:42:49]

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

[Toon Verwaest, 2016-08-03 11:22:05]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-08-03 11:22:06]

The code snippet in the comment returns [2,1,1], but as far as I can tell this
should be [2,1,2] since the var x is merged with the x from the destructuring
pattern. As another datapoint this is also what ff does.

[adamk, 2016-08-03 17:39:23]

On 2016/08/03 11:22:06, Toon Verwaest wrote:
> The code snippet in the comment returns [2,1,1], but as far as I can tell this
> should be [2,1,2] since the var x is merged with the x from the destructuring
> pattern. As another datapoint this is also what ff does.

Firefox's handling of scoping in ES6 features is...less than ideal. We match JSC
here. And in my reading (and likely rossberg's), we match the spec. See step 27
of https://tc39.github.io/ecma262/#sec-functiondeclarationinstantiation

Which destructuring pattern are you referring to?

[Toon Verwaest, 2016-08-03 19:25:16]

I see. This came from a discussion with neis about it, and we focused too much
on 26. Sorry for the noise.