Don't treat catch scopes as possibly-shadowing for sloppy eval

Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n

Committed: https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
Cr-Commit-Position: refs/heads/master@{#36046}

[adamk, 2016-05-04 00:30:04]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org, rmcilroy@chromium.org

[rmcilroy, 2016-05-04 08:50:43]

On 2016/05/04 00:30:04, adamk wrote:

Thanks Adam, LGTM.

[adamk, 2016-05-04 20:41:37]

Description was changed from

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This is a minimal fix to the attached bug, which disregards catch scopes
when marking variables as possibly shadowed. I suspect this if statement
could be widened to also disregard block scopes, but those don't share
the same bug because BlockContexts preserve the "calls eval" bit during
lazy compilation.

BUG=chromium:608279
LOG=n
==========

to

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n
==========

[adamk, 2016-05-04 20:43:50]

I've updated the patch to widen the change: we now ignore shadowing not just for
catch scopes but for non-declaration scopes in general.

I tried looking for some tighter DCHECK to add, but I don't know what it would
be. PTAL.

[Dan Ehrenberg, 2016-05-04 20:54:08]

lgtm

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

https://codereview.chromium.org/1950803002/diff/1/src/ast/scopes.cc#newcode1101
src/ast/scopes.cc:1101: } else if (calls_sloppy_eval() && !is_script_scope() &&
!is_catch_scope() &&
Can anything besides a function_scope have variable bindings be shadowed by a
sloppy eval call?

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

https://codereview.chromium.org/1950803002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1103: // A variable binding may have been found in an outer
scope, but the current
Could you briefly mention in this comment that non-declaration scopes are
irrelevant, and scripts would've resolved to a context lookup anyway? It was not
obvious to me when initially reading the code.

[adamk, 2016-05-04 21:06:19]

Added to the comment.

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

https://codereview.chromium.org/1950803002/diff/1/src/ast/scopes.cc#newcode1101
src/ast/scopes.cc:1101: } else if (calls_sloppy_eval() && !is_script_scope() &&
!is_catch_scope() &&
On 2016/05/04 20:54:08, Dan Ehrenberg wrote:
> Can anything besides a function_scope have variable bindings be shadowed by a
> sloppy eval call?

We also must consider block scopes which are marked as "declaration scopes"
(called "varblock" elsewhere), as well as eval scopes.

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

https://codereview.chromium.org/1950803002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1103: // A variable binding may have been found in an outer
scope, but the current
On 2016/05/04 20:54:08, Dan Ehrenberg wrote:
> Could you briefly mention in this comment that non-declaration scopes are
> irrelevant, and scripts would've resolved to a context lookup anyway? It was
not
> obvious to me when initially reading the code.

Done.

[adamk, 2016-05-04 21:06:24]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-05-04 21:06:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1950803002/#ps40001
(title: "Amend comment")

[commit-bot: I haz the power, 2016-05-04 21:06:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950803002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950803002/40001

[commit-bot: I haz the power, 2016-05-04 21:34:50]

Description was changed from

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n
==========

to

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n
==========

[commit-bot: I haz the power, 2016-05-04 21:34:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-04 21:36:19]

Description was changed from

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n
==========

to

==========
Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable resolutions
as possibly-shadowed by a sloppy eval. In the attached bug, this comes
into play since catch scopes have different behavior with respect to
the "calls eval" in eager vs lazy compilation (in the latter, they
are never marked as "calls eval" because CatchContexts don't have
an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few other
cases where shadowing isn't possible, such as non-declaration block scopes.

BUG=chromium:608279
LOG=n

Committed: https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
Cr-Commit-Position: refs/heads/master@{#36046}
==========

[commit-bot: I haz the power, 2016-05-04 21:36:20]

Patchset 3 (id:??) landed as
https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
Cr-Commit-Position: refs/heads/master@{#36046}