[parser] Skipping inner funcs: produce the same scopes / variables for sloppy block funcs.

[parser] Skipping inner funcs: produce the same scopes / variables for sloppy block funcs.

BUG=v8:5516
R=vogelheim@chromium.org

Review-Url: https://codereview.chromium.org/2670633003
Cr-Commit-Position: refs/heads/master@{#42913}
Committed: https://chromium.googlesource.com/v8/v8/+/dec6112752d700ad21873462b503f4272d3c66f4

[marja, 2017-02-02 11:56:17]

ptal

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

https://codereview.chromium.org/2670633003/diff/1/src/ast/scopes.cc#newcode575
src/ast/scopes.cc:575: created_variable->set_maybe_assigned();
The Variable passing is needed to be able to do this

https://codereview.chromium.org/2670633003/diff/1/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2670633003/diff/1/src/parsing/preparser.cc#ne...
src/parsing/preparser.cc:140: if (is_sloppy(function_scope->language_mode())) {
This change is needed so that sloppy block funcs get declared before arguments
(see DeclareArguments below) like they do in ParserBase.

[vogelheim, 2017-02-02 19:00:42]

lgtm

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

https://codereview.chromium.org/2670633003/diff/1/src/ast/scopes.cc#newcode575
src/ast/scopes.cc:575: created_variable->set_maybe_assigned();
On 2017/02/02 11:56:17, marja wrote:
> The Variable passing is needed to be able to do this

I can't parse that sentence. Care to explain?

https://codereview.chromium.org/2670633003/diff/1/src/ast/scopes.cc#newcode576
src/ast/scopes.cc:576: }
I don't really understand what this whole block does... I have a hunch it might
be a bit easier to read if "if (factory).." becomes the outer condition. Not
sure, though.

https://codereview.chromium.org/2670633003/diff/1/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2670633003/diff/1/src/parsing/preparser.cc#ne...
src/parsing/preparser.cc:140: if (is_sloppy(function_scope->language_mode())) {
On 2017/02/02 11:56:17, marja wrote:
> This change is needed so that sloppy block funcs get declared before arguments
> (see DeclareArguments below) like they do in ParserBase.

Ah. Thanks for the explanation.

[marja, 2017-02-03 07:47:19]

The CQ bit was checked by marja@chromium.org

[marja, 2017-02-03 07:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2670633003/#ps20001
(title: "code review (vogelheim@)")

[commit-bot: I haz the power, 2017-02-03 07:47:27]

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-02-03 08:16:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486108039881740,
"parent_rev": "63dea876da2bc73015c7c462166c9eaf3401a7d7", "commit_rev":
"dec6112752d700ad21873462b503f4272d3c66f4"}

[commit-bot: I haz the power, 2017-02-03 08:16:12]

Description was changed from

==========
[parser] Skipping inner funcs: produce the same scopes / variables for sloppy
block funcs.

BUG=v8:5516
R=vogelheim@chromium.org
==========

to

==========
[parser] Skipping inner funcs: produce the same scopes / variables for sloppy
block funcs.

BUG=v8:5516
R=vogelheim@chromium.org

Review-Url: https://codereview.chromium.org/2670633003
Cr-Commit-Position: refs/heads/master@{#42913}
Committed:
https://chromium.googlesource.com/v8/v8/+/dec6112752d700ad21873462b503f4272d3...
==========

[commit-bot: I haz the power, 2017-02-03 08:16:13]

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

[vogelheim, 2017-02-03 18:33:26]

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

https://codereview.chromium.org/2670633003/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:574: if (created_variable != kDummyPreParserVariable &&
In the previous version, this check always happened when !factory, even if
created_variable was set in a previous iteration and is not-null in this one. Is
this intentional?

[marja, 2017-02-03 21:03:01]

had forgot to post these comments

will look at your question shortly..

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

https://codereview.chromium.org/2670633003/diff/1/src/ast/scopes.cc#newcode575
src/ast/scopes.cc:575: created_variable->set_maybe_assigned();
On 2017/02/02 19:00:42, vogelheim wrote:
> On 2017/02/02 11:56:17, marja wrote:
> > The Variable passing is needed to be able to do this
> 
> I can't parse that sentence. Care to explain?

I need to return Variable* and transmit it up until here so that I can do
set_maybe_assigned.

https://codereview.chromium.org/2670633003/diff/1/src/ast/scopes.cc#newcode576
src/ast/scopes.cc:576: }
On 2017/02/02 19:00:42, vogelheim wrote:
> I don't really understand what this whole block does... I have a hunch it
might
> be a bit easier to read if "if (factory).." becomes the outer condition. Not
> sure, though.

This creates the assignmentstatement which assigns the lazy sloppy block
function into a variable in the outer scope. The variable gets created only
once, but if there are multiple sloppy block functions with the same name, an
assignment is created for each of them.

And an assignment naturally sets maybe_assigned.

When preparsing, AssignmentStatements are not created, but I still need to set
the relevant variable as maybe_assigned.

You're right, this is clearer if I move if (factory) out; I also added some
DCHECKs to make it clear when we're parsing and when we're preparsing.

An additional complication is that we're only creating variables in the
preparser when FLAG_preparser_scope_analysis is on. Oh well. Another possibility
would be to pass the maybe-assignedness-boolean to DeclareVariableName but I
didn't want to make that decision yet... let's see how this develops.

[marja, 2017-02-03 21:03:54]

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

https://codereview.chromium.org/2670633003/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:574: if (created_variable != kDummyPreParserVariable &&
On 2017/02/03 18:33:25, vogelheim wrote:
> In the previous version, this check always happened when !factory, even if
> created_variable was set in a previous iteration and is not-null in this one.
Is
> this intentional?

Yeah, we don't need to set_maybe_assigned on the same variable multiple times.