Fix harmony sloppy block scoping dynamic redeclaration check

Fix harmony sloppy block scoping dynamic redeclaration check

The previous code did not properly check for harmony const when
doing the dynamic redeclaration check. This was masked in the
test because each eval had an initializer, and the initializer was what
triggered the exception.

This patch tightens the test by removing initializers and fixes the bug in
DeclareLookupSlot.

Also change the test to use assertThrows where possible.

BUG=v8:4550
LOG=n

Committed: https://crrev.com/fd3ff03da22304344bd95dc65ac76ed354f87da2
Cr-Commit-Position: refs/heads/master@{#31995}

[adamk, 2015-11-12 22:35:38]

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

[adamk, 2015-11-12 22:35:38]

Just noticed this when I broke these tests in an in-progress patch, this
refactoring makes the tests a little easier for me to read.

[adamk, 2015-11-12 22:58:20]

Hmm, I think there's actually a bug in the related code, fix coming up and I'll
likely repurpose this patch to fix that bug.

[adamk, 2015-11-12 23:05:37]

Description was changed from

==========
Use assertThrows where possible in harmony sloppy block scoping tests
==========

to

==========
Fix harmony sloppy block scoping dynamic redeclaration check

The previous code did not properly check for harmony const when
doing the dynamic redeclaration check. This was masked in the
test because each eval had an initializer, and the initializer was what
triggered the exception.

This patch tightens the test by removing initializers and fixes the bug in
DeclareLookupSlot.

Also change the test to use assertThrows where possible.
==========

[adamk, 2015-11-12 23:07:47]

Description was changed from

==========
Fix harmony sloppy block scoping dynamic redeclaration check

The previous code did not properly check for harmony const when
doing the dynamic redeclaration check. This was masked in the
test because each eval had an initializer, and the initializer was what
triggered the exception.

This patch tightens the test by removing initializers and fixes the bug in
DeclareLookupSlot.

Also change the test to use assertThrows where possible.
==========

to

==========
Fix harmony sloppy block scoping dynamic redeclaration check

The previous code did not properly check for harmony const when
doing the dynamic redeclaration check. This was masked in the
test because each eval had an initializer, and the initializer was what
triggered the exception.

This patch tightens the test by removing initializers and fixes the bug in
DeclareLookupSlot.

Also change the test to use assertThrows where possible.

BUG=v8:4550
LOG=n
==========

[adamk, 2015-11-12 23:08:38]

Okay, this patch now fixes a bug besides changing the test :)

[adamk, 2015-11-12 23:22:27]

https://codereview.chromium.org/1437003006/diff/20001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1437003006/diff/20001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:234: binding_flags == IMMUTABLE_CHECK_INITIALIZED
||
This is for CONST_LEGACY, I wonder if it even belongs here...

[Dan Ehrenberg, 2015-11-12 23:41:57]

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-eval-var-over-let.js (right):

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-eval-var-over-let.js:12: eval('var x');
How about leaving in the previous tests and adding these no-intializer ones in
addition, rather than replacing the tests?

[Dan Ehrenberg, 2015-11-12 23:42:23]

lgtm

LGTM modulo re-adding the changed tests.

[adamk, 2015-11-12 23:45:17]

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-eval-var-over-let.js (right):

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-eval-var-over-let.js:12: eval('var x');
On 2015/11/12 23:41:57, Dan Ehrenberg wrote:
> How about leaving in the previous tests and adding these no-intializer ones in
> addition, rather than replacing the tests?

What do you feel the initializer versions add to these tests? They're really
supposed to be about redeclaration, right?

I'm happy to have more tests rather than fewer, I'd just like to understand your
thinking here.

[Dan Ehrenberg, 2015-11-12 23:53:19]

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-eval-var-over-let.js (right):

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-eval-var-over-let.js:12: eval('var x');
On 2015/11/12 at 23:45:17, adamk wrote:
> On 2015/11/12 23:41:57, Dan Ehrenberg wrote:
> > How about leaving in the previous tests and adding these no-intializer ones
in
> > addition, rather than replacing the tests?
> 
> What do you feel the initializer versions add to these tests? They're really
supposed to be about redeclaration, right?
> 
> I'm happy to have more tests rather than fewer, I'd just like to understand
your thinking here.

Well, my thinking is rather mechanical:
1. You found that my tests missed a case
2. My tests previously caused an exception to be thrown, which is the intended
behavior, and wasn't the case (at least for some of them) before I wrote my
patch
3. Your patch deleted my test and replaced it with a test of that other case

Seems pretty clear-cut that both have value, but maybe I'm missing something.
Maybe we don't need the full product of both tests, and one
redeclaration+initializer would be enough; is that what you're getting at?

[adamk, 2015-11-13 07:06:01]

https://codereview.chromium.org/1437003006/diff/20001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1437003006/diff/20001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:234: binding_flags == IMMUTABLE_CHECK_INITIALIZED
||
On 2015/11/12 23:22:27, adamk wrote:
> This is for CONST_LEGACY, I wonder if it even belongs here...

Any thoughts on this? The comment above claims this is for checking lexical
conflicts, but IMMUTABLE_CHECK_INITIALIZED is checking a non-lexical binding.

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-eval-var-over-let.js (right):

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-eval-var-over-let.js:12: eval('var x');
On 2015/11/12 23:53:18, Dan Ehrenberg wrote:
> On 2015/11/12 at 23:45:17, adamk wrote:
> > On 2015/11/12 23:41:57, Dan Ehrenberg wrote:
> > > How about leaving in the previous tests and adding these no-intializer
ones
> in
> > > addition, rather than replacing the tests?
> > 
> > What do you feel the initializer versions add to these tests? They're really
> supposed to be about redeclaration, right?
> > 
> > I'm happy to have more tests rather than fewer, I'd just like to understand
> your thinking here.
> 
> Well, my thinking is rather mechanical:
> 1. You found that my tests missed a case
> 2. My tests previously caused an exception to be thrown, which is the intended
> behavior, and wasn't the case (at least for some of them) before I wrote my
> patch
> 3. Your patch deleted my test and replaced it with a test of that other case
> 
> Seems pretty clear-cut that both have value, but maybe I'm missing something.
> Maybe we don't need the full product of both tests, and one
> redeclaration+initializer would be enough; is that what you're getting at?

All I was thinking was that we desugar "var x = 1" into "var x; x = 1", and the
"x = 1" part doesn't have anything to do with what the rest of this test is
testing. The fact that the "x = 1" is the only thing that was (erroneously)
causing one of these tests to pass is sort of beside the point. Though maybe
what you're saying is: why reduce test coverage? That seems fair enough.
Especially if we change out desugaring later.

[adamk, 2015-11-13 20:59:07]

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-eval-var-over-let.js (right):

https://codereview.chromium.org/1437003006/diff/20001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-eval-var-over-let.js:12: eval('var x');
On 2015/11/13 07:06:01, adamk wrote:
> On 2015/11/12 23:53:18, Dan Ehrenberg wrote:
> > On 2015/11/12 at 23:45:17, adamk wrote:
> > > On 2015/11/12 23:41:57, Dan Ehrenberg wrote:
> > > > How about leaving in the previous tests and adding these no-intializer
> ones
> > in
> > > > addition, rather than replacing the tests?
> > > 
> > > What do you feel the initializer versions add to these tests? They're
really
> > supposed to be about redeclaration, right?
> > > 
> > > I'm happy to have more tests rather than fewer, I'd just like to
understand
> > your thinking here.
> > 
> > Well, my thinking is rather mechanical:
> > 1. You found that my tests missed a case
> > 2. My tests previously caused an exception to be thrown, which is the
intended
> > behavior, and wasn't the case (at least for some of them) before I wrote my
> > patch
> > 3. Your patch deleted my test and replaced it with a test of that other case
> > 
> > Seems pretty clear-cut that both have value, but maybe I'm missing
something.
> > Maybe we don't need the full product of both tests, and one
> > redeclaration+initializer would be enough; is that what you're getting at?
> 
> All I was thinking was that we desugar "var x = 1" into "var x; x = 1", and
the
> "x = 1" part doesn't have anything to do with what the rest of this test is
> testing. The fact that the "x = 1" is the only thing that was (erroneously)
> causing one of these tests to pass is sort of beside the point. Though maybe
> what you're saying is: why reduce test coverage? That seems fair enough.
> Especially if we change out desugaring later.

After more discussion offline, I convinced Dan that the duplication here
wouldn't buy us anything.

[adamk, 2015-11-13 21:02:07]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-11-13 21:02:20]

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

[commit-bot: I haz the power, 2015-11-13 21:03:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-14 23:22:00]

Patchset 2 (id:??) landed as
https://crrev.com/fd3ff03da22304344bd95dc65ac76ed354f87da2
Cr-Commit-Position: refs/heads/master@{#31995}