Move sloppy block function hoisting logic from Parser to Scope.

Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

BUG=

Committed: https://crrev.com/8ee581af4b6de5ac4beda973461fef2b4e105339
Cr-Commit-Position: refs/heads/master@{#39233}

[marja, 2016-09-02 09:59:15]

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

[commit-bot: I haz the power, 2016-09-02 09:59: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-09-02 10:42:31]

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

[commit-bot: I haz the power, 2016-09-02 10:42:32]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-02 10:54:52]

marja@chromium.org changed reviewers:
+ adamk@chromium.org, dehrenberg@chromium.org

[marja, 2016-09-02 10:54:53]

adamk, dehrenberg, ptal. (verwaest is ooo)

This is a second try at moving sloppy block function handling to Scope. Now I
just move the hoisting logic to Scope (as much as I can) but don't change the
point in time when the analysis is done.

It's kinda meh, since the Parser anyway needs to go through the sloppy block
function map to create the Statements (we didn't want to put that into Scope.
Also, now Scope creates VariableProxies, but it already does that in
NewUnresolved, so I guess that part is fine.

[adamk, 2016-09-02 17:59:14]

Description was changed from

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

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

to

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

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

[adamk, 2016-09-02 17:59:14]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org
- dehrenberg@chromium.org

[adamk, 2016-09-02 18:16:27]

I'd like littledan to review this, he'll be back on Tuesday (we have Monday off
here in the US)

https://codereview.chromium.org/2301183003/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2301183003/diff/1/src/ast/ast.h#newcode1180
src/ast/ast.h:1180: VariableProxy* from() const { return from_; }
Nit: swap order of these? (first getter, then setter)

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

https://codereview.chromium.org/2301183003/diff/1/src/ast/scopes.cc#newcode452
src/ast/scopes.cc:452: void
DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory,
Can you add some DCHECKS at the start here?

DCHECK(is_sloppy(language_mode()));
DCHECK(is_function_scope() || is_eval_scope() || is_script_scope()
||(is_block_scope() && outer_scope()->is_function_scope());
DCHECK(HasSimpleParameters() || is_block_scope());

https://codereview.chromium.org/2301183003/diff/1/src/ast/scopes.cc#newcode471
src/ast/scopes.cc:471: if (!HasSimpleParameters()) {
HasSimpleParameters() does a scope chain walk, seems like it might be better to
hoist this call out of the loop.

[marja, 2016-09-05 09:23:43]

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

[marja, 2016-09-05 09:23:43]

adamk@, thanks for comments!

nikolaos@, could you have a look too?

(I'll wait for littledan@'s review before landing in any case.)

https://codereview.chromium.org/2301183003/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2301183003/diff/1/src/ast/ast.h#newcode1180
src/ast/ast.h:1180: VariableProxy* from() const { return from_; }
On 2016/09/02 18:16:27, adamk wrote:
> Nit: swap order of these? (first getter, then setter)

Done.

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

https://codereview.chromium.org/2301183003/diff/1/src/ast/scopes.cc#newcode452
src/ast/scopes.cc:452: void
DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory,
On 2016/09/02 18:16:27, adamk wrote:
> Can you add some DCHECKS at the start here?
> 
> DCHECK(is_sloppy(language_mode()));
> DCHECK(is_function_scope() || is_eval_scope() || is_script_scope()
> ||(is_block_scope() && outer_scope()->is_function_scope());
> DCHECK(HasSimpleParameters() || is_block_scope());

Done.

https://codereview.chromium.org/2301183003/diff/1/src/ast/scopes.cc#newcode471
src/ast/scopes.cc:471: if (!HasSimpleParameters()) {
On 2016/09/02 18:16:27, adamk wrote:
> HasSimpleParameters() does a scope chain walk, seems like it might be better
to
> hoist this call out of the loop.

Done.

[nickie, 2016-09-05 11:53:21]

LGTM, with a couple of comments.

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

https://codereview.chromium.org/2301183003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4684: scope->HoistSloppyBlockFunctions(factory(), ok);
If I understand it right, this separation of logic results in two traversals of
scope->sloppy_block_function_map(), instead of one.  (One hidden in
HoistSloppyBlockFunctions, and one a few lines below.)  I don't understand the
context enough to assess if that's a problem.

https://codereview.chromium.org/2301183003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4686: if (!*ok) return;
Because of the DCHECK in HoistSloppyBlockFunctions, this DCHECK is not necessary
(there's another DCHECK on *ok before that).  I suggest a CHECK_OK_VOID here.

[marja, 2016-09-06 12:16:10]

Thanks for review!

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

https://codereview.chromium.org/2301183003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4684: scope->HoistSloppyBlockFunctions(factory(), ok);
On 2016/09/05 11:53:21, nickie wrote:
> If I understand it right, this separation of logic results in two traversals
of
> scope->sloppy_block_function_map(), instead of one.  (One hidden in
> HoistSloppyBlockFunctions, and one a few lines below.)  I don't understand the
> context enough to assess if that's a problem.

Yes, this is correct, and this is also why this CL is somewhat "meh". :/ But
otoh, I don't see another way how to make this work (passing AstNodeFactory to
Scope and letting it create the Statements was a no-go). We'd need somehow to
move the responsibility of deciding what to hoist to Scope (so that it's
accessible to PreParser variants too so that they can also start understanding
more about Scopes).

https://codereview.chromium.org/2301183003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4686: if (!*ok) return;
On 2016/09/05 11:53:21, nickie wrote:
> Because of the DCHECK in HoistSloppyBlockFunctions, this DCHECK is not
necessary
> (there's another DCHECK on *ok before that).  I suggest a CHECK_OK_VOID here.

Will do (I'll do all review comments at one go, assuming I get some from
littledan@ too).

[Dan Ehrenberg, 2016-09-06 22:34:44]

lgtm

I like the new form of the refactoring; seems like more appropriate layering,
and this looks correct to me.

[marja, 2016-09-07 08:21:22]

thanks for review!

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

https://codereview.chromium.org/2301183003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4686: if (!*ok) return;
On 2016/09/05 11:53:21, nickie wrote:
> Because of the DCHECK in HoistSloppyBlockFunctions, this DCHECK is not
necessary
> (there's another DCHECK on *ok before that).  I suggest a CHECK_OK_VOID here.

Done.

[marja, 2016-09-07 08:21:28]

The CQ bit was checked by marja@chromium.org

[marja, 2016-09-07 08:21:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
nikolaos@chromium.org, littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2301183003/#ps80001
(title: "code review (nikolaos@)")

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

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-07 08:47:54]

Description was changed from

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

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

to

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

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

[commit-bot: I haz the power, 2016-09-07 08:47:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-07 08:48:39]

Description was changed from

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

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

to

==========
Move sloppy block function hoisting logic from Parser to Scope.

This moves scope-related logic (such as looking up variables) to Scope
where it belongs, and enables PreParser to do more Scope-related
operations in the future.

BUG=

Committed: https://crrev.com/8ee581af4b6de5ac4beda973461fef2b4e105339
Cr-Commit-Position: refs/heads/master@{#39233}
==========

[commit-bot: I haz the power, 2016-09-07 08:48:40]

Patchset 5 (id:??) landed as
https://crrev.com/8ee581af4b6de5ac4beda973461fef2b4e105339
Cr-Commit-Position: refs/heads/master@{#39233}