Parsing:Create the same scopes for non-simple params in PreParser & Parser.

Parsing: Create the same scopes for non-simple params in PreParser & Parser.

Rationale:

- To do scope analysis based on PreParser, and use the result again when parsing
  later, PreParser and Parser need to produce the same Scopes and variable
  declarations in them.

- This is not the case for non-simple parameters: Parser creates an additional
  inner Scope where the declarations were, whereas PreParser does
  DeclareVariableName directly in the function Scope.

- So this CL fixes that by moving the Scope creation for non-simple parameters
  into ParserBase.

- As a side product (and a partial proof that this change makes sense),
  PreParser::ParseEagerFunctionBody is now gone.


BUG=v8:5516
Review-Url: https://codereview.chromium.org/2638333002
Cr-Commit-Position: refs/heads/master@{#42537}
Committed: https://chromium.googlesource.com/v8/v8/+/3534091756ac394e402d4e3dbca63498b7230ea0

[marja, 2017-01-19 10:17:09]

Description was changed from

==========
WIP - create the same scopes for complex params in PreParser & Parser.

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

to

==========
Parsing: Create the same scopes for non-simple params in PreParser & Parser.

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

[marja, 2017-01-19 10:30:53]

Description was changed from

==========
Parsing: Create the same scopes for non-simple params in PreParser & Parser.

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

to

==========
Parsing: Create the same scopes for non-simple params in PreParser & Parser.

Rationale:

- To do scope analysis based on PreParser, and use the result again when parsing
  later, PreParser and Parser need to produce the same Scopes and variable
  declarations in them.

- This is not the case for non-simple parameters: Parser creates an additional
  inner Scope where the declarations were, whereas PreParser does
  DeclareVariableName directly in the function Scope.

- So this CL fixes that by moving the Scope creation for non-simple parameters
  into ParserBase.

- As a side product (and a partial proof that this change makes sense),
  PreParser::ParseEagerFunctionBody is now gone.


BUG=v8:5516
==========

[marja, 2017-01-19 10:30:53]

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

[marja, 2017-01-19 10:31:05]

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

[marja, 2017-01-19 10:31:08]

ptal

[commit-bot: I haz the power, 2017-01-19 10:31:09]

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, 2017-01-19 11:02:40]

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

[commit-bot: I haz the power, 2017-01-19 11:02:41]

Dry run: This issue passed the CQ dry run.

[marja, 2017-01-19 16:05:25]

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

[marja, 2017-01-19 16:05:26]

switching reviewers (verwaest@ doesn't have time atm), adamk, ptal

[adamk, 2017-01-19 21:57:10]

This patch is tricky to review with stuff moving from one file to another, but
from what I can tell it lgtm

One question: why no test changes in this patch? Will the changes here soon be
exercised somewhere? If not, could this have a (maybe temporary) test in
test-parsing.cc?

[marja, 2017-01-20 08:35:56]

On 2017/01/19 21:57:10, adamk wrote:
> This patch is tricky to review with stuff moving from one file to another, but
> from what I can tell it lgtm
> 
> One question: why no test changes in this patch? Will the changes here soon be
> exercised somewhere? If not, could this have a (maybe temporary) test in
> test-parsing.cc?

Hi, thanks for reviewing.

Yes, the changes will be exercised in the upcoming CLs which will do scope
allocation based on preparsed scopes, store that result, and use it later when
we've parsed the same code. At that point I'll add DCHECKs making sure that the
scopes / variables produced by preparser match the ones produced by parser.

However, it'll take some time and several CLs before that can be done (more
unifying is needed too, e.g., PreParser doesn't distinguish between parameters
and local variables).. so.. maybe I should add a test for explicitly testing
that the Scopes contain the same declarations (ultimately; now they don't, they
just contain similar enough declarations). I'll look into adding tests as a
follow-up CL.

Currently the best test exercising this code is
test-parsing/NoPessimisticContextAllocation; it checks that the scopes /
variables produced by the preparser are "same enough" for deciding context
allocation for outer level variables.

[marja, 2017-01-20 08:36:05]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2017-01-20 08:36:17]

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, 2017-01-20 08:58:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484901365727640,
"parent_rev": "8b8c8df05bef3238004d5a338238e53ba2115f28", "commit_rev":
"3534091756ac394e402d4e3dbca63498b7230ea0"}

[commit-bot: I haz the power, 2017-01-20 08:58:59]

Description was changed from

==========
Parsing: Create the same scopes for non-simple params in PreParser & Parser.

Rationale:

- To do scope analysis based on PreParser, and use the result again when parsing
  later, PreParser and Parser need to produce the same Scopes and variable
  declarations in them.

- This is not the case for non-simple parameters: Parser creates an additional
  inner Scope where the declarations were, whereas PreParser does
  DeclareVariableName directly in the function Scope.

- So this CL fixes that by moving the Scope creation for non-simple parameters
  into ParserBase.

- As a side product (and a partial proof that this change makes sense),
  PreParser::ParseEagerFunctionBody is now gone.


BUG=v8:5516
==========

to

==========
Parsing: Create the same scopes for non-simple params in PreParser & Parser.

Rationale:

- To do scope analysis based on PreParser, and use the result again when parsing
  later, PreParser and Parser need to produce the same Scopes and variable
  declarations in them.

- This is not the case for non-simple parameters: Parser creates an additional
  inner Scope where the declarations were, whereas PreParser does
  DeclareVariableName directly in the function Scope.

- So this CL fixes that by moving the Scope creation for non-simple parameters
  into ParserBase.

- As a side product (and a partial proof that this change makes sense),
  PreParser::ParseEagerFunctionBody is now gone.


BUG=v8:5516

Review-Url: https://codereview.chromium.org/2638333002
Cr-Commit-Position: refs/heads/master@{#42537}
Committed:
https://chromium.googlesource.com/v8/v8/+/3534091756ac394e402d4e3dbca63498b72...
==========

[commit-bot: I haz the power, 2017-01-20 08:59:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/3534091756ac394e402d4e3dbca63498b72...

[marja, 2017-01-20 09:57:34]

Ahm, I looked into adding the test that parsing and preparsing produce the
same(-ish) scopes & variables and enhancing that later.

But that doesn't work since we explicitly destroy the preparsed scopes in
AnalyzePartially, so I don't have a chance to get the information before we
remove it - we only retain the free variables. Ofc in the future we need to
retain the info about what variables there were (+ ctxt allocation status &
maybe_assigned for them). Add that point, I can add a test which matches that
information against what Parser produces.

So for now, the most meaningful test for this refactoring is that the existing
tests, esp. test-parsing/NoPessimisticContextAllocation, pass.

[adamk, 2017-01-20 17:51:20]

Thanks for digging into the testing here, plan sounds good to me.