[ast/parsing] Pessimistically assume all top-level variables will be assigned.

[ast/parsing] Pessimistically assume all top-level variables will be assigned.

A previous CL (https://codereview.chromium.org/2634123002) did that for
let-declared variables.  This CL also does it for var- and function-declared
variables.

BUG=v8:5636
Review-Url: https://codereview.chromium.org/2656753003
Cr-Commit-Position: refs/heads/master@{#42813}
Committed: https://chromium.googlesource.com/v8/v8/+/32842802b5f64544b0d3222a780f3fff7be13232

[neis, 2017-01-25 12:29:12]

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

[commit-bot: I haz the power, 2017-01-25 12:29:19]

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

[neis, 2017-01-25 12:31:16]

Description was changed from

==========
[ast/parsing] Pessimistically assume all top-level variables will be assigned.

A previous CL (https://codereview.chromium.org/2634123002) did that for
let-declared variables.  This CL also does it for var- and function-declared
variables.

BUG=v8:5636
==========

to

==========
[ast/parsing] Pessimistically assume all top-level variables will be assigned.

A previous CL (https://codereview.chromium.org/2634123002) did that for
let-declared variables.  This CL also does it for var- and function-declared
variables.

BUG=v8:5636
==========

[neis, 2017-01-25 12:31:16]

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

[neis, 2017-01-25 12:31:49]

ptal

[marja, 2017-01-25 12:44:10]

https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:827: // assigned.  This is because they might be accessed
by a lazily parsed
You could fix this comment in this CL :) (There's already discussion about it in
the previous one.)

https://codereview.chromium.org/2656753003/diff/1/src/parsing/pattern-rewrite...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2656753003/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:166:
MarkTopLevelVariableAsAssigned(var_init_scope, proxy);
This CL is a bit weird in that we need to remember to call
MarkTopLevelVariableAsAssigned in a bunch of places. Can't we somehow do that in
Scope whenever we add a declaration in the script scope? Or does that not work
because of destructuring declarations? (I'm continuously confused about this...)

[commit-bot: I haz the power, 2017-01-25 13:01:21]

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

[commit-bot: I haz the power, 2017-01-25 13:01:22]

Dry run: This issue passed the CQ dry run.

[neis, 2017-01-25 13:48:22]

On 2017/01/25 12:44:10, marja wrote:
> https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h
> File src/parsing/parser.h (right):
> 
>
https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h#newcod...
> src/parsing/parser.h:827: // assigned.  This is because they might be accessed
> by a lazily parsed
> You could fix this comment in this CL :) (There's already discussion about it
in
> the previous one.)

In response to Adam's comment in the old CL, I had added the last sentence. But
now I should probably add something about global properties, too.

>
https://codereview.chromium.org/2656753003/diff/1/src/parsing/pattern-rewrite...
> File src/parsing/pattern-rewriter.cc (right):
> 
>
https://codereview.chromium.org/2656753003/diff/1/src/parsing/pattern-rewrite...
> src/parsing/pattern-rewriter.cc:166:
> MarkTopLevelVariableAsAssigned(var_init_scope, proxy);
> This CL is a bit weird in that we need to remember to call
> MarkTopLevelVariableAsAssigned in a bunch of places. Can't we somehow do that
in
> Scope whenever we add a declaration in the script scope? Or does that not work
> because of destructuring declarations? (I'm continuously confused about
this...)

Possibly. I'm not very familiar with all that code either. Is there a canonical
place? It seems like there are many different functions on Scope that are used
to declare variables.

[marja, 2017-01-25 17:15:08]

Hmm, e.g., in the case of complex params we create several variables - which of
those needs to be maybe_assigned? The temporary which is created in the scope
where the params usually would be, or the non-temporary which is in the inner
scope? (That's unrelated to this CL though.)

Not sure if that's analogous to the destructured declarations; will check
tomorrow.

Re: many declaration functions; don't all global variables go through
Scope::DeclareVariable / DeclareLocal?

[marja, 2017-01-25 20:38:57]

Hmm, lgtm, as I don't have concrete ideas how to make this better for now, and
it's turbofan stuff...

[neis, 2017-01-26 08:28:15]

Patchset #2 (id:20001) has been deleted

[neis, 2017-01-27 09:30:00]

Adam, any thoughts?

[adamk, 2017-01-27 16:07:36]

https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:833: if (scope->is_script_scope() ||
scope->is_module_scope()) {
I thought we currently disable preparsing of functions in modules, and changing
that was blocked on marja's work to preparse inner functions.

Or am I confused and we instead force context allocation for all module-scoped
variables?

[neis, 2017-01-30 09:30:34]

On 2017/01/27 16:07:36, adamk wrote:
> https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h
> File src/parsing/parser.h (right):
> 
>
https://codereview.chromium.org/2656753003/diff/1/src/parsing/parser.h#newcod...
> src/parsing/parser.h:833: if (scope->is_script_scope() ||
> scope->is_module_scope()) {
> I thought we currently disable preparsing of functions in modules, and
changing
> that was blocked on marja's work to preparse inner functions.

For a module containing

function foo() {}
function bar() {foo=1}
bar()

--print-scopes says bar (and foo) is lazily parsed and, without my patch, bar
(and foo) is never assigned.

> Or am I confused and we instead force context allocation for all module-scoped
> variables?

Yes, but simply because a module is a generator.

[adamk, 2017-01-30 21:06:57]

Apparently the answer is, I was confused.

This lgtm, though I agree we should continue to think of some way to not have
this logic scattered around the parser.

[neis, 2017-01-31 13:04:33]

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

[commit-bot: I haz the power, 2017-01-31 13:04:38]

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-31 13:32:08]

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

[commit-bot: I haz the power, 2017-01-31 13:32:09]

Dry run: This issue passed the CQ dry run.

[neis, 2017-01-31 13:33:02]

The CQ bit was checked by neis@chromium.org

[neis, 2017-01-31 13:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2656753003/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-01-31 13:33:13]

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-31 13:35:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485869582463250,
"parent_rev": "bfc8dc12e96f4cb016f52121d6815896136a00c9", "commit_rev":
"32842802b5f64544b0d3222a780f3fff7be13232"}

[commit-bot: I haz the power, 2017-01-31 13:36:00]

Description was changed from

==========
[ast/parsing] Pessimistically assume all top-level variables will be assigned.

A previous CL (https://codereview.chromium.org/2634123002) did that for
let-declared variables.  This CL also does it for var- and function-declared
variables.

BUG=v8:5636
==========

to

==========
[ast/parsing] Pessimistically assume all top-level variables will be assigned.

A previous CL (https://codereview.chromium.org/2634123002) did that for
let-declared variables.  This CL also does it for var- and function-declared
variables.

BUG=v8:5636

Review-Url: https://codereview.chromium.org/2656753003
Cr-Commit-Position: refs/heads/master@{#42813}
Committed:
https://chromium.googlesource.com/v8/v8/+/32842802b5f64544b0d3222a780f3fff7be...
==========

[commit-bot: I haz the power, 2017-01-31 13:36:01]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/32842802b5f64544b0d3222a780f3fff7be...