Add function-var to variables_ so LookupRecursive doesn't need to special-case it

Add function-var to variables_ so LookupRecursive doesn't need to special-case it

After parsing a function, if there's no masking declaration in the function scope, DeclareFunctionVar will also bind the function name to a variable. It will either bind it to the const/const-legacy function_ variable, or to a dynamic non-local if the function calls sloppy eval.

Even if the variable is masked or sloppy eval is called, we still declare the function-var. The client immediately sets up the variable by assigning the resulting function to it.

BUG=v8:5209
Committed: https://crrev.com/65bae443a2ddc1f882e07728fd67cc2178c842b5
Cr-Commit-Position: refs/heads/master@{#39581}

[Toon Verwaest, 2016-08-24 14:28:09]

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

[Toon Verwaest, 2016-08-24 14:28:10]

ptal

[adamk, 2016-08-24 18:15:49]

As noted below, I find the new declaration logic harder to understand than the
old lookup logic. Removing the lookup "special case" also removes some symmetry
between the static lookup logic and Context::Lookup (which still needs special
handling of the function name).

Is there a bigger advantage here in having all variables in Scope::variables_
that I'm missing?

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

https://codereview.chromium.org/2274133002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:480: if (preexists || calls_sloppy_eval()) {
This logic is pretty tricky (I find it trickier than the old lookup logic).

[adamk, 2016-08-24 20:22:48]

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

https://codereview.chromium.org/2274133002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:481: function_ = new (zone())
Maybe we should just skip creating the function_ variable if it's definitely
shadowed in the function? The parser already has to know a lot about how this
stuff works...what if it reacted to a nullptr return value of DeclareFunctionVar
by not creating the assignment?

That lets the calls_sloppy_eval use the Declare() path, with the NonLocal
becoming a DYNAMIC_LOCAL.

WDYT?

[Toon Verwaest, 2016-08-24 20:26:21]

I was thinking about that approach as well. I'll try it out tomorrow.

[Toon Verwaest, 2016-09-19 12:12:01]

ptal again

[adamk, 2016-09-19 18:00:39]

Can you add a test checking how this interacts with sloppy function hoisting? In
particular, this code should return the inner "f":

(function f() {
  {
    function f() { return 42 }
  }
  function g() { return f }
  return g;
})()()

https://codereview.chromium.org/2274133002/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2274133002/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:3661: if (function_scope->Lookup(function_name) ==
nullptr) {
Do you mean LookupLocal? Please add a test that would catch this error!

https://codereview.chromium.org/2274133002/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:3673: statement = factory()->NewEmptyStatement(0);
Why 0 instead of kNoSourcePosition?

[Toon Verwaest, 2016-09-20 05:14:36]

Addressed comments. Nice catch on the Lookup vs LookupLocal!

[adamk, 2016-09-20 16:11:27]

lgtm

[Toon Verwaest, 2016-09-20 17:50:26]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2016-09-20 17:50:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 17:53:26]

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

[commit-bot: I haz the power, 2016-09-20 17:53:27]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13109)
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[Toon Verwaest, 2016-09-21 08:21:18]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-09-21 08:21:18]

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/2274133002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-21 08:21:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 08:24:16]

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

[commit-bot: I haz the power, 2016-09-21 08:24:17]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9296)

[Toon Verwaest, 2016-09-21 08:32:51]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-09-21 08:32:51]

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/2274133002/#ps180001
(title: "functionvar")

[commit-bot: I haz the power, 2016-09-21 08:32:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 08:35:22]

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

[commit-bot: I haz the power, 2016-09-21 08:35:23]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9297)

[Toon Verwaest, 2016-09-21 08:46:43]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-09-21 08:46:44]

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/2274133002/#ps200001
(title: "merge")

[commit-bot: I haz the power, 2016-09-21 08:46:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 09:15:18]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-21 09:15:33]

Description was changed from

==========
Add function-var to variables_ so LookupRecursive doesn't need to special-case
it

After parsing a function, if there's no masking declaration in the function
scope, DeclareFunctionVar will also bind the function name to a variable. It
will either bind it to the const/const-legacy function_ variable, or to a
dynamic non-local if the function calls sloppy eval.

Even if the variable is masked or sloppy eval is called, we still declare the
function-var. The client immediately sets up the variable by assigning the
resulting function to it.

BUG=v8:5209
==========

to

==========
Add function-var to variables_ so LookupRecursive doesn't need to special-case
it

After parsing a function, if there's no masking declaration in the function
scope, DeclareFunctionVar will also bind the function name to a variable. It
will either bind it to the const/const-legacy function_ variable, or to a
dynamic non-local if the function calls sloppy eval.

Even if the variable is masked or sloppy eval is called, we still declare the
function-var. The client immediately sets up the variable by assigning the
resulting function to it.

BUG=v8:5209

Committed: https://crrev.com/65bae443a2ddc1f882e07728fd67cc2178c842b5
Cr-Commit-Position: refs/heads/master@{#39581}
==========

[commit-bot: I haz the power, 2016-09-21 09:15:34]

Patchset 11 (id:??) landed as
https://crrev.com/65bae443a2ddc1f882e07728fd67cc2178c842b5
Cr-Commit-Position: refs/heads/master@{#39581}