Synchronize scopes between parser/preparser

Synchronize scopes between parser/preparser

This patch introduces new scopes in the preparser, just like they
are introduced by the parser, in the following places:

-   blocks
-   try statement
-   switch statement
-   scoped statements, in several places
-   for statement
-   eager function bodies

R=rossberg@chromium.org
BUG=
LOG=N

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

[nickie, 2016-04-21 12:53:06]

This came up because of a CL related to typed mode.
https://codereview.chromium.org/1871923003/

[nickie, 2016-04-21 12:57:52]

There are a few more calls to NewScope in parser.cc that have no counterparts in
preparser.cc.  As the file is not included in the patch set and I cannot add
comments, I'm listing them here.  Line numbers are from b4889f7.

---- DefaultConstructor:
Called from ParseLazy and from ParseClassLiteral.  This should be OK.
parser.cc:192:  Scope* function_scope = NewScope(scope, FUNCTION_SCOPE, kind);

---- BuildParameterInitializationBlock:
Called from ParseEagerFunctionBody.  This should be OK.
parser.cc:4382:      param_scope = NewScope(scope_, BLOCK_SCOPE);

---- RewriteYieldStar:
This should be OK.
parser.cc:5973:    Scope* catch_scope = NewScope(scope, CATCH_SCOPE);

---- FinalizeIteratorUse:
Called from FinalizeForOfStatement.  This should be OK.
parser.cc:6451:    Scope* catch_scope = parser_->NewScope(scope, CATCH_SCOPE);

---- BuildIteratorCloseForCompletion:
Called from FinalizeIteratorUse.  This should be OK.
parser.cc:6562:    Scope* catch_scope = NewScope(scope, CATCH_SCOPE);


Also, here are the calls to NewScope that I found to have counterparts in
preparser.cc:

---- PreParseProgram/DoParseProgram:
preparser.h:973:    Scope* scope = NewScope(scope_, SCRIPT_SCOPE);
parser.cc:890:    Scope* scope = NewScope(scope_, SCRIPT_SCOPE);
parser.cc:907:      scope = NewScope(scope, EVAL_SCOPE);
parser.cc:909:      scope = NewScope(scope, MODULE_SCOPE);

---- PreParseLazyFunction/ParseLazy:
preparser.cc:107:  Scope* top_scope = NewScope(scope_, SCRIPT_SCOPE);
preparser.cc:112:  Scope* function_scope = NewScope(scope_, FUNCTION_SCOPE,
kind);
parser.cc:1047:    Scope* scope = NewScope(scope_, SCRIPT_SCOPE);
parser.cc:1070:          NewScope(scope_, FUNCTION_SCOPE,
FunctionKind::kArrowFunction);

---- ParseWithStatement:
preparser.cc:704:  Scope* with_scope = NewScope(scope_, WITH_SCOPE);
parser.cc:2670:  Scope* with_scope = NewScope(scope_, WITH_SCOPE);

---- ParseFunctionLiteral:
preparser.cc:969:  Scope* function_scope = NewScope(scope_, FUNCTION_SCOPE,
kind);
parser.cc:3988:  Scope* scope = NewScope(scope_, FUNCTION_SCOPE, kind);

---- ParseClassLiteral:
preparser.cc:1056:  Scope* scope = NewScope(scope_, BLOCK_SCOPE);
parser.cc:4627:  Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);

[rossberg, 2016-04-21 13:04:00]

LGTM

[nickie, 2016-04-21 13:15:26]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-04-21 13:15:32]

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

[commit-bot: I haz the power, 2016-04-21 13:41:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-22 19:14:52]

Description was changed from

==========
Synchronize scopes between parser/preparser

This patch introduces new scopes in the preparser, just like they
are introduced by the parser, in the following places:

-   blocks
-   try statement
-   switch statement
-   scoped statements, in several places
-   for statement
-   eager function bodies

R=rossberg@chromium.org
BUG=
LOG=N
==========

to

==========
Synchronize scopes between parser/preparser

This patch introduces new scopes in the preparser, just like they
are introduced by the parser, in the following places:

-   blocks
-   try statement
-   switch statement
-   scoped statements, in several places
-   for statement
-   eager function bodies

R=rossberg@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/fa43f4c99bae8dc57d1cc33dd7b7d914650ecd8c
Cr-Commit-Position: refs/heads/master@{#35708}
==========

[commit-bot: I haz the power, 2016-04-22 19:14:53]

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

[adamk, 2016-04-23 00:09:23]

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

[adamk, 2016-04-23 00:09:24]

Can you explain a bit more of what the purpose of this change is? What effect
does adding these scopes have? The lack of test changes makes this rather
mysterious.

[nickie, 2016-04-25 08:03:20]

On 2016/04/23 00:09:24, adamk wrote:
> Can you explain a bit more of what the purpose of this change is? What effect
> does adding these scopes have? The lack of test changes makes this rather
> mysterious.

This is something that started from typed mode.  There, for some reason, I had
to know if a declaration was at a top-level scope.  After some experimentation,
it seemed that the only way to do this was by introducing an extra parameter to
the relevant ParseXX functions.  You can take a look at the code and comments
here:  https://codereview.chromium.org/1871923003/

Andreas asked why it wasn't possible to just use scope information for this.  It
turned out that the reason was some inconsistency between the scopes introduced
by the parser and those introduced by the preparser (possibly complicated by the
fact that empty scopes are optimized away).  More precisely, the preparser did
not introduce a scope for blocks and, therefore, declarations found inside a
block may be mistaken (by the preparser) for top-level ones.  Andreas suggested
that this was an issue to be fixed in the main branch too, hence this patch.

As far as I see, adding these scopes should not have any effect now, in the main
branch.  In fact, there could be some performance penalty.  Also, I cannot think
of appropriate test cases, as there seemed to be no symptoms --- the preparser
seems not to care if there are scopes there or not.  On the other hand, it may
prevent bugs caused by what I described above.

[rossberg, 2016-04-25 08:17:21]

What Niko said. Plus, I would generally argue that any such discrepancy between
parser and preparser is morally a bug, even if there are no symptoms. At least
it's a hurdle for further pre/pareser unification.

[adamk, 2016-04-25 17:53:03]

On 2016/04/25 08:03:20, nickie wrote:
> On 2016/04/23 00:09:24, adamk wrote:
> > Can you explain a bit more of what the purpose of this change is? What
effect
> > does adding these scopes have? The lack of test changes makes this rather
> > mysterious.
> 
> This is something that started from typed mode.  There, for some reason, I had
> to know if a declaration was at a top-level scope.  After some
experimentation,
> it seemed that the only way to do this was by introducing an extra parameter
to
> the relevant ParseXX functions.  You can take a look at the code and comments
> here:  https://codereview.chromium.org/1871923003/
> 
> Andreas asked why it wasn't possible to just use scope information for this. 
It
> turned out that the reason was some inconsistency between the scopes
introduced
> by the parser and those introduced by the preparser (possibly complicated by
the
> fact that empty scopes are optimized away).  More precisely, the preparser did
> not introduce a scope for blocks and, therefore, declarations found inside a
> block may be mistaken (by the preparser) for top-level ones.  Andreas
suggested
> that this was an issue to be fixed in the main branch too, hence this patch.
> 
> As far as I see, adding these scopes should not have any effect now, in the
main
> branch.  In fact, there could be some performance penalty.  Also, I cannot
think
> of appropriate test cases, as there seemed to be no symptoms --- the preparser
> seems not to care if there are scopes there or not.  On the other hand, it may
> prevent bugs caused by what I described above.

Given the performance penalty, and the lack of behavioral difference, it just
seems like the only observable effects of this change (that is, the behavior of
the engine in the wild) will be negative. With the team trying to reduce startup
time this quarter, I'm not sure it makes sense to take a hit here without any
gain other than increased code symmetry.