[parsing] Fix maybe-assigned for loop variables.

[parsing] Fix maybe-assigned for loop variables.

Due to hoisting, the value of a 'var'-declared variable may actually change even
if the code contains only the "initial" assignment, namely when that assignment
occurs inside a loop.  For example:

  let i = 10;
  do { var x = i } while (i--):

As a simple and very conservative approximation of this, we explicitly mark
as maybe-assigned any non-lexical variable whose "declaration" does not
syntactically occur in the function scope.  (In the example above, it
occurs in a block scope.)

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

[neis, 2017-02-06 15:48:37]

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

[commit-bot: I haz the power, 2017-02-06 15:48:45]

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

[neis, 2017-02-06 16:03:14]

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

[neis, 2017-02-06 16:03:15]

PTAL.

I currently had to disable two tests in test-parsing/PreParserScopeAnalysis. In
these tests, the parser and preparser (accidentally) used to agree that the
variabe is not assigned. 
 Due to my changes, the preparser now thinks it is assigned. 
 That's because the code in the preparser doesn't know that the var statement
comes without an initializer expression (the parser does, however).  Any
suggestions what to do about this?  I'd think we just allow for the preparser to
be less precise here.

[commit-bot: I haz the power, 2017-02-06 16:11:20]

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

[commit-bot: I haz the power, 2017-02-06 16:11:21]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-02-06 20:07:55]

lgtm

[marja, 2017-02-06 21:08:44]

https://codereview.chromium.org/2673403003/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2673403003/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1358: // as maybe-assigned any non-lexical variable
whose "declaration" does not
Confused by this comment + the comment about preparser now thinking stuff is
assigned and parser thinking not assigned.

[marja, 2017-02-07 09:43:18]

lgtm %

MarkLoopVariableAsAssigned is a slightly confusing name since it's not only
about loop variables (non-loop variables go via that code path too). But the
comment above it explains what it does... (nah, can't suggest a better name
either, so I'm fine with that name)

So, I was confused about the overapproximating case which is not the one
mentioned in the description. Dunno if that needs to be explained in a comment
somewhere?

One of us needs to update the test to re-enable the ones you disabled; but I
don't feel strongly about who does that :)

[neis, 2017-02-07 11:15:13]

On 2017/02/07 09:43:18, marja wrote:
> lgtm %
> 
> MarkLoopVariableAsAssigned is a slightly confusing name since it's not only
> about loop variables (non-loop variables go via that code path too). But the
> comment above it explains what it does... (nah, can't suggest a better name
> either, so I'm fine with that name)
> 
> So, I was confused about the overapproximating case which is not the one
> mentioned in the description. Dunno if that needs to be explained in a comment
> somewhere?

Added a comment in preparser.cc.
 
> One of us needs to update the test to re-enable the ones you disabled; but I
> don't feel strongly about who does that :)

Done.

[neis, 2017-02-07 11:17:30]

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

[commit-bot: I haz the power, 2017-02-07 11:17:34]

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-02-07 11:40:12]

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

[commit-bot: I haz the power, 2017-02-07 11:40:13]

Dry run: This issue passed the CQ dry run.

[neis, 2017-02-07 11:43:47]

The CQ bit was checked by neis@chromium.org

[neis, 2017-02-07 11:43:48]

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/2673403003/#ps40001
(title: "Address feedback.")

[commit-bot: I haz the power, 2017-02-07 11:43:52]

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-07 11:45:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486467827808080,
"parent_rev": "9458dc9ef16756bdd554e7d8f74b364d561c01b7", "commit_rev":
"a33fcd663b28b8846e12b97c30d6e7d837767f86"}

[commit-bot: I haz the power, 2017-02-07 11:45:15]

Description was changed from

==========
[parsing] Fix maybe-assigned for loop variables.

Due to hoisting, the value of a 'var'-declared variable may actually change even
if the code contains only the "initial" assignment, namely when that assignment
occurs inside a loop.  For example:

  let i = 10;
  do { var x = i } while (i--):

As a simple and very conservative approximation of this, we explicitly mark
as maybe-assigned any non-lexical variable whose "declaration" does not
syntactically occur in the function scope.  (In the example above, it
occurs in a block scope.)

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

to

==========
[parsing] Fix maybe-assigned for loop variables.

Due to hoisting, the value of a 'var'-declared variable may actually change even
if the code contains only the "initial" assignment, namely when that assignment
occurs inside a loop.  For example:

  let i = 10;
  do { var x = i } while (i--):

As a simple and very conservative approximation of this, we explicitly mark
as maybe-assigned any non-lexical variable whose "declaration" does not
syntactically occur in the function scope.  (In the example above, it
occurs in a block scope.)

BUG=v8:5636

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

[commit-bot: I haz the power, 2017-02-07 11:45:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/a33fcd663b28b8846e12b97c30d6e7d8377...

[Michael Hablich, 2017-02-07 19:32:56]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2679263002/ by hablich@chromium.org.

The reason for reverting is: Speculative revert because of
https://codereview.chromium.org/2679163002/..