Disable lazy parsing inside eval (see bug).

Disable lazy parsing inside eval (see bug).

If the eval contains a let, we need to know whether an inner function
refers to the variable to be able to decide its context allocation
status.

The added test needs https://codereview.chromium.org/2435023002/ too
in order to pass.

BUG=v8:5736
Review-Url: https://codereview.chromium.org/2574753002
Cr-Commit-Position: refs/heads/master@{#41723}
Committed: https://chromium.googlesource.com/v8/v8/+/ed080e69665c96f9e5e61cf1d0e1ce377fc5aca6

[marja, 2016-12-13 13:36:18]

Patchset #1 (id:1) has been deleted

[marja, 2016-12-13 13:40:55]

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

[marja, 2016-12-13 13:40:56]

ptal

[adamk, 2016-12-14 08:00:14]

Is this specific to block-scoped variables? Can you add a test case for a var in
a strict eval?

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

https://codereview.chromium.org/2574753002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:126: force_context_allocation_ =
Another option would be to add an

if (is_eval_scope()) return true;

to Scope::MustAllocateInContext(). I think that would be a little more
idiomatic, though there's likely room for some cleanup here...

[Toon Verwaest, 2016-12-14 12:03:17]

Isn't this over-eagerly context allocating in eval scopes? If there are no
nested scopes, we don't need to context allocate?

It seems like we just cannot allow lazy functions without scope resolution
inside eval. That assumption was wrong due to let unfortunately.

[Toon Verwaest, 2016-12-14 12:04:07]

Especially given that we'll probably always do scope resolution the preparser to
avoid reparsing.

[adamk, 2016-12-14 12:26:57]

On 2016/12/14 12:03:17, Toon Verwaest wrote:
> Isn't this over-eagerly context allocating in eval scopes? If there are no
> nested scopes, we don't need to context allocate?
> 
> It seems like we just cannot allow lazy functions without scope resolution
> inside eval. That assumption was wrong due to let unfortunately.

Yes, we just discussed this offline with Marja, Georg, and myself. Sounds like
we're agreed that the right fix is to re-force eager parsing in eval.

[marja, 2016-12-15 12:25:22]

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

[commit-bot: I haz the power, 2016-12-15 12:25:24]

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-12-15 12:38:59]

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

[commit-bot: I haz the power, 2016-12-15 12:39:00]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[marja, 2016-12-15 12:47:05]

Description was changed from

==========
Force context allocation in eval scopes (see bug).

We allow lazy functions inside evals, so we have no way of knowing
which variables they refer to.

The added test needs https://codereview.chromium.org/2435023002/ too
in order to pass.

BUG=v8:5736
==========

to

==========
Disable lazy parsing inside eval (see bug).

If the eval contains a let, we need to know whether an inner function
refers to the variable to be able to decide its context allocation
status.

The added test needs https://codereview.chromium.org/2435023002/ too
in order to pass.

BUG=v8:5736
==========

[marja, 2016-12-15 13:02:02]

updated with the new solution, ptal

[adamk, 2016-12-15 13:11:49]

Change to scopes.cc lgtm, just a few comments on the tests.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/bugs/bug-2...
File test/mjsunit/bugs/bug-2728.js (left):

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/bugs/bug-2...
test/mjsunit/bugs/bug-2728.js:1: // Copyright 2016 the V8 project authors. All
rights reserved.
Did this file get deleted accidentally? I don't see a connection with the rest
of the CL.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-5736.js (right):

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:10: eval("let foo = 1; function
maybe_lazy() { foo = 2; } maybe_lazy(); my_global = foo;");
Nit: you could use a template literal to split this across multiple lines and
make it a little easier to read.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:10: eval("let foo = 1; function
maybe_lazy() { foo = 2; } maybe_lazy(); my_global = foo;");
You could make this test pass without Dan's fix by storing maybe_lazy in another
let-bound variable, i.e.,

let maybe_lazy = function() { foo = 2 };

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:11: assertEquals(my_global, 2);
assertEquals takes (expected, actual) I think?

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:14: "use strict"
Please add a semicolon here.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:15: eval("var foo = 3; function
maybe_lazy() { foo = 4; } maybe_lazy(); my_global = foo;");
Same here regarding template literal.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:16: assertEquals(my_global, 4);
And same assertEquals comment.

[marja, 2016-12-15 13:48:07]

thanks for review

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/bugs/bug-2...
File test/mjsunit/bugs/bug-2728.js (left):

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/bugs/bug-2...
test/mjsunit/bugs/bug-2728.js:1: // Copyright 2016 the V8 project authors. All
rights reserved.
On 2016/12/15 13:11:48, adamk wrote:
> Did this file get deleted accidentally? I don't see a connection with the rest
> of the CL.

Routing offline discussion here:
assertThrows calls eval and that no longer invokes the preparser, so there's no
way to test that we *don't* have certain errors in the preparser.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-5736.js (right):

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:10: eval("let foo = 1; function
maybe_lazy() { foo = 2; } maybe_lazy(); my_global = foo;");
On 2016/12/15 13:11:48, adamk wrote:
> You could make this test pass without Dan's fix by storing maybe_lazy in
another
> let-bound variable, i.e.,
> 
> let maybe_lazy = function() { foo = 2 };

Done.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:10: eval("let foo = 1; function
maybe_lazy() { foo = 2; } maybe_lazy(); my_global = foo;");
On 2016/12/15 13:11:48, adamk wrote:
> Nit: you could use a template literal to split this across multiple lines and
> make it a little easier to read.

Done.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:11: assertEquals(my_global, 2);
On 2016/12/15 13:11:48, adamk wrote:
> assertEquals takes (expected, actual) I think?

Done.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:14: "use strict"
On 2016/12/15 13:11:48, adamk wrote:
> Please add a semicolon here.

Done.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:15: eval("var foo = 3; function
maybe_lazy() { foo = 4; } maybe_lazy(); my_global = foo;");
On 2016/12/15 13:11:48, adamk wrote:
> Same here regarding template literal.

Done.

https://codereview.chromium.org/2574753002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5736.js:16: assertEquals(my_global, 4);
On 2016/12/15 13:11:48, adamk wrote:
> And same assertEquals comment.

Done.

[adamk, 2016-12-15 14:00:39]

lgtm, thanks!

[marja, 2016-12-15 14:01:49]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2016-12-15 14:02:00]

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-12-15 14:26:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481810509482690,
"parent_rev": "f1441384ff3800885e5c6f074656bf8806986ff4", "commit_rev":
"ed080e69665c96f9e5e61cf1d0e1ce377fc5aca6"}

[commit-bot: I haz the power, 2016-12-15 14:27:02]

Description was changed from

==========
Disable lazy parsing inside eval (see bug).

If the eval contains a let, we need to know whether an inner function
refers to the variable to be able to decide its context allocation
status.

The added test needs https://codereview.chromium.org/2435023002/ too
in order to pass.

BUG=v8:5736
==========

to

==========
Disable lazy parsing inside eval (see bug).

If the eval contains a let, we need to know whether an inner function
refers to the variable to be able to decide its context allocation
status.

The added test needs https://codereview.chromium.org/2435023002/ too
in order to pass.

BUG=v8:5736

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

[commit-bot: I haz the power, 2016-12-15 14:27:03]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/ed080e69665c96f9e5e61cf1d0e1ce377fc...