PreParser: track variable declarations and parameters

PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501
Committed: https://crrev.com/1b5ccb055a6e93f7b45d653cf0bbbb9b047ee31d
Cr-Commit-Position: refs/heads/master@{#41521}

[marja, 2016-11-29 08:56:18]

Description was changed from

==========
PreParser: track parameters.

Do not declare yet, do just enough to not crash.

WIP - DO NOT COMMIT.

BUG=
==========

to

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
}

BUG=v8:5501
==========

[marja, 2016-11-29 09:44:01]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 09:44:11]

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

[marja, 2016-11-29 10:27:22]

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

[marja, 2016-11-29 10:27:23]

ptal

[marja, 2016-11-29 10:34:57]

Description was changed from

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
}

BUG=v8:5501
==========

to

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501
==========

[marja, 2016-11-30 08:01:15]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 08:01:18]

Dry run: 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-11-30 08:19:52]

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

[commit-bot: I haz the power, 2016-11-30 08:19:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13404)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[marja, 2016-11-30 09:43:15]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 09:43:19]

Dry run: 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-11-30 10:08:11]

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

[commit-bot: I haz the power, 2016-11-30 10:08:12]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-12-01 10:50:56]

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

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:56: void VariableMap::DeclareName(Zone* zone, const
AstRawString* name) {
We should probably have a debug mode flag to check that that we don't call this
on non-preparsed scopes; and don't call the regular DeclareVariable on preparsed
scopes.

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:910: // TODO(wingo): Avoid O(n^2) check.
Does it make sense to still add TODO's for wingo?

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:912: has_rest_ = is_rest;
Do we care about rest? Isn't that just used to deal with it at some point later
rewriting the parameters or so?

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:913: if (name == ast_value_factory->arguments_string()) {
Do we care about arguments? Isn't that just used to avoid allocating the
arguments object in the function?

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1653: if (var == kDummyPreParserVariable) return var;
Mmh. We should probably stop sharing LookupRecursive between regular scope
resolution and preparser... Preparser is much simpler, and these additional
checks now make both slower: scope resolution of the preparser and of the
parser.

For the preparser we just need:

bool Scope::LookupRecursive(AstRawString* name, Scope* end) {
  Scope* current = this;
  do {
    if (LookupLocal(name) != nullptr) return true;
    current = current->outer_scope();
  } while (current != end);
  return false;
}

I'm also fine with improving this code in follow-up CLs.

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1821: ParseInfo* info,
That obviously makes this slightly more complicated again; but we can cleanly
separate the code with your flag from the rest. So it might even make it better
overall.

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:157: ZoneList<const AstRawString*>* identifiers =
nullptr) {
What about just using a single-linked-list?

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:760: is_optional(is_optional),
Do we care about is_optional and is_rest? I especially doubt we care about
is_rest?

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:776: bool is_optional;
bool ... : 1;

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1491: (*pattern.identifiers_)[0],
!IsEmptyExpression(initializer),
pattern.identifiers_->at(0)?

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1513: bool is_duplicate = false;
Definitely seems like we don't care about is_duplicate; so we shouldn't do the
O(n^2) work :)

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1536: // parsed eagerly).
What about arrow functions fully parsed inside of
preparse-with-variable-resolution functions?

(function() { function f() { g = (...) => ... } })()

[Toon Verwaest, 2016-12-01 13:33:28]

https://codereview.chromium.org/2407163003/diff/360001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2407163003/diff/360001/src/parsing/preparser....
src/parsing/preparser.h:614: return
PreParserExpression::Assignment(left.identifiers_);
Mmh, actually this isn't correct is it?

var {a:b}={a} just creates an assigned var b on the left side, and a reference
to a on the right side. This would wrongly assume that the a reference on the
right resolves to the "local a"; but there is no local "a". It need to be
context allocated in the outer scope (if this is a nested function with no other
"a" declaration).

[marja, 2016-12-05 16:05:20]

ptal

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

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:56: void VariableMap::DeclareName(Zone* zone, const
AstRawString* name) {
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> We should probably have a debug mode flag to check that that we don't call
this
> on non-preparsed scopes; and don't call the regular DeclareVariable on
preparsed
> scopes.

We already have is_lazily_parsed, but that's set too late (and AnalyzePartially
-> FetchFreeVariables assumes it's not set at that point yet). :/

As a brute force solution I added a is_being_lazily_parsed_.

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:910: // TODO(wingo): Avoid O(n^2) check.
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Does it make sense to still add TODO's for wingo?

Was just copied over; but I deleted this whole function :)

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:912: has_rest_ = is_rest;
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Do we care about rest? Isn't that just used to deal with it at some point
later
> rewriting the parameters or so?

Ditto; the downside is that now preparsed scopes don't have has_rest_ set
consistently which might be surprising if some later feature makes use of it.

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:913: if (name == ast_value_factory->arguments_string()) {
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Do we care about arguments? Isn't that just used to avoid allocating the
> arguments object in the function?

Ditto

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1653: if (var == kDummyPreParserVariable) return var;
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Mmh. We should probably stop sharing LookupRecursive between regular scope
> resolution and preparser... Preparser is much simpler, and these additional
> checks now make both slower: scope resolution of the preparser and of the
> parser.
> 
> For the preparser we just need:
> 
> bool Scope::LookupRecursive(AstRawString* name, Scope* end) {
>   Scope* current = this;
>   do {
>     if (LookupLocal(name) != nullptr) return true;
>     current = current->outer_scope();
>   } while (current != end);
>   return false;
> }
> 
> I'm also fine with improving this code in follow-up CLs.

Ok, added a TODO about that. For that we should somehow track the Scope
"preparsedness" (or life cycle) better:
1. getting preparsed (also for the DCHECKs that you suggested adding;
DeclareVaribleName should only happen at this stage)
2. was preparsed (we have ran analyzePartially)
3. not preparsed

Resoving against a kDummyPreParserVariable can only happen when we are running
AnalyzePartially for a Scope which we just preparsed.

At the moment we don't have a good way tracking that. And the different code
paths merge in FetchFreeVariables which is called both by AnalyzePartially and
the debugger... yeah, I'll need to think about how to make this cleaner.

https://codereview.chromium.org/2407163003/diff/340001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1821: ParseInfo* info,
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> That obviously makes this slightly more complicated again; but we can cleanly
> separate the code with your flag from the rest. So it might even make it
better
> overall.

I might need to introduce a new parameter here which tells us whether we're
using FetchFreeVariables via AnalyzePartially or via CollectNonLocals.

If we're using it for AnalyzePartially, we can call
LookupRecursiveInPreParsedScope() (or whatever is a descriptive name) instead.

This actually boils down to not having a function which is
JustLookupAndDoNothingElse() which I was complaining about earlier :)

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:157: ZoneList<const AstRawString*>* identifiers =
nullptr) {
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> What about just using a single-linked-list?

Will be done in a follow-up CL, k? PreParserExpression already has this as a
ZoneList and I didn't want to change that in this CL.

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:760: is_optional(is_optional),
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Do we care about is_optional and is_rest? I especially doubt we care about
> is_rest?

Removed

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:776: bool is_optional;
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> bool ... : 1;

Removed

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1491: (*pattern.identifiers_)[0],
!IsEmptyExpression(initializer),
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> pattern.identifiers_->at(0)?

Simplified this code so this branch is done.

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1513: bool is_duplicate = false;
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> Definitely seems like we don't care about is_duplicate; so we shouldn't do the
> O(n^2) work :)

Simplified this; I'm always using DeclareVariableName now, since it shouldn't
matter; for preparsed scopes, parameters and variables are the same.

https://codereview.chromium.org/2407163003/diff/340001/src/parsing/preparser....
src/parsing/preparser.h:1536: // parsed eagerly).
On 2016/12/01 10:50:56, Toon Verwaest wrote:
> What about arrow functions fully parsed inside of
> preparse-with-variable-resolution functions?
> 
> (function() { function f() { g = (...) => ... } })()

Yeah, the comment was wrong. Added tests to demonstrate that we still
over-context-allocate in these cases and fixed the comment.

https://codereview.chromium.org/2407163003/diff/360001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2407163003/diff/360001/src/parsing/preparser....
src/parsing/preparser.h:614: return
PreParserExpression::Assignment(left.identifiers_);
On 2016/12/01 13:33:27, Toon Verwaest wrote:
> Mmh, actually this isn't correct is it?
> 
> var {a:b}={a} just creates an assigned var b on the left side, and a reference
> to a on the right side. This would wrongly assume that the a reference on the
> right resolves to the "local a"; but there is no local "a". It need to be
> context allocated in the outer scope (if this is a nested function with no
other
> "a" declaration).

Tracking unresolved variables doesn't go via this code path.

Added tests to demonstrate that this works:

      {"function inner() { var {my_var : a} = {my_var}; }", true},
      {"function inner() { let {my_var : a} = {my_var}; }", true},
      {"function inner() { const {my_var : a} = {my_var}; }", true},

and

(function TestLazyInnerFunctionDestructuring() {
  var i = (function eager_outer() {
    var outer_var = 41; // Should be context-allocated
    function lazy_inner() {
      // This introduces b and refers to outer_var.
      var {outer_var : b} = {outer_var};
      return b;
    }
    return lazy_inner;
  })();
  assertEquals(41, i());
})();

[marja, 2016-12-05 16:05:53]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 16:06:03]

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

[Toon Verwaest, 2016-12-05 16:29:04]

lgtm

[commit-bot: I haz the power, 2016-12-05 16:31:14]

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

[commit-bot: I haz the power, 2016-12-05 16:31:14]

Dry run: This issue passed the CQ dry run.

[marja, 2016-12-06 13:21:02]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2016-12-06 13:21:20]

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-12-06 13:23:30]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1481030462134330,
"parent_rev": "60bf0eab43a47171799ccc33a46744170f093309", "commit_rev":
"07a0acf6971b266a98c8e2af9217a7896c4e3707"}

[commit-bot: I haz the power, 2016-12-06 13:23:35]

Description was changed from

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501
==========

to

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501
==========

[commit-bot: I haz the power, 2016-12-06 13:23:36]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-12-06 13:24:14]

Description was changed from

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501
==========

to

==========
PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501

Committed: https://crrev.com/1b5ccb055a6e93f7b45d653cf0bbbb9b047ee31d
Cr-Commit-Position: refs/heads/master@{#41521}
==========

[commit-bot: I haz the power, 2016-12-06 13:24:15]

Patchset 23 (id:??) landed as
https://crrev.com/1b5ccb055a6e93f7b45d653cf0bbbb9b047ee31d
Cr-Commit-Position: refs/heads/master@{#41521}