[es6] Handle conflicts for sloppy let

[es6] Handle conflicts for sloppy let

We have to call CheckConflictingVarDeclarations in case we have enabled
--harmony-sloppy

BUG=v8:4287
LOG=N
R=littledan@chromium.org, rossberg@chromium.org

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

[arv (Not doing code reviews), 2015-07-08 21:29:21]

PTAL

[Dan Ehrenberg, 2015-07-08 23:42:13]

lgtm

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h#newcode...
src/preparser.h:3769: if (is_strict(language_mode()) || allow_harmony_sloppy())
{
A tangent, but it looks like CheckConflictingVarDeclarations is implemented only
in the parser, not the preparser, so the errors aren't quite as early as we'd
optimally like. Any particular reason for this? It looks like the preparser
maintains the whole scope thing anyway.

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-conflicts-sloppy.js (right):

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-conflicts-sloppy.js:62: // TestAll('Conflict',
'eval("' + s + '");');
I don't see why these tests wouldn't make sense. Wouldn't there be a conflict in
the new eval-driven block scope, even in sloppy mode?

[arv (Not doing code reviews), 2015-07-09 14:30:53]

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-conflicts-sloppy.js (right):

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-conflicts-sloppy.js:62: // TestAll('Conflict',
'eval("' + s + '");');
On 2015/07/08 23:42:13, Dan Ehrenberg wrote:
> I don't see why these tests wouldn't make sense. Wouldn't there be a conflict
in
> the new eval-driven block scope, even in sloppy mode?

See the bug.

  eval('let x; var x;')

is more similar to

  eval('{ let x; var x; }')

we are not creating the lexical block correctly for sloppy evals so these tests
are not passing or the tests are not correct for sloppy mode.

[rossberg, 2015-07-10 12:13:17]

lgtm

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h#newcode...
src/preparser.h:3769: if (is_strict(language_mode()) || allow_harmony_sloppy())
{
On 2015/07/08 23:42:13, Dan Ehrenberg wrote:
> A tangent, but it looks like CheckConflictingVarDeclarations is implemented
only
> in the parser, not the preparser, so the errors aren't quite as early as we'd
> optimally like. Any particular reason for this? It looks like the preparser
> maintains the whole scope thing anyway.

No, the preparser doesn't maintain scopes. This is a long-standing issue. We
would love to do that, but experiments showed that it is fairly costly and
causes significant regressions in e.g. CodeLoad. It's still an open problem how
to diagnose binding errors early in all cases, and we have punted so far.

(It's a particular pain point for strong mode, btw.)

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-conflicts-sloppy.js (right):

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-conflicts-sloppy.js:62: // TestAll('Conflict',
'eval("' + s + '");');
On 2015/07/09 14:30:53, arv wrote:
> On 2015/07/08 23:42:13, Dan Ehrenberg wrote:
> > I don't see why these tests wouldn't make sense. Wouldn't there be a
conflict
> in
> > the new eval-driven block scope, even in sloppy mode?
> 
> See the bug.
> 
>   eval('let x; var x;')
> 
> is more similar to
> 
>   eval('{ let x; var x; }')
> 
> we are not creating the lexical block correctly for sloppy evals so these
tests
> are not passing or the tests are not correct for sloppy mode.

I suppose

let x; eval('var x');

is an error in sloppy mode, but not in strict mode. That's a fairly unfortunate
semantic deviation, actually. But why does this difference only pop up in the
global scope tests?

These tests should probably be changed to use Realm.eval in TestGlobal, so that
we test the actual global scope, and there should be a separate TestEval
(similarly for strict mode). Can you add a TODO for that?

[arv (Not doing code reviews), 2015-07-10 13:00:48]

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1226103002/diff/40001/src/preparser.h#newcode...
src/preparser.h:3769: if (is_strict(language_mode()) || allow_harmony_sloppy())
{
On 2015/07/10 12:13:17, rossberg wrote:
> On 2015/07/08 23:42:13, Dan Ehrenberg wrote:
> > A tangent, but it looks like CheckConflictingVarDeclarations is implemented
> only
> > in the parser, not the preparser, so the errors aren't quite as early as
we'd
> > optimally like. Any particular reason for this? It looks like the preparser
> > maintains the whole scope thing anyway.
> 
> No, the preparser doesn't maintain scopes.

This is not entirely true. We do set up the scopes but we do not create the
bindings in them in the preparser.

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-conflicts-sloppy.js (right):

https://codereview.chromium.org/1226103002/diff/40001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-conflicts-sloppy.js:62: // TestAll('Conflict',
'eval("' + s + '");');
On 2015/07/10 12:13:17, rossberg wrote:
> On 2015/07/09 14:30:53, arv wrote:
> > On 2015/07/08 23:42:13, Dan Ehrenberg wrote:
> > > I don't see why these tests wouldn't make sense. Wouldn't there be a
> conflict
> > in
> > > the new eval-driven block scope, even in sloppy mode?
> > 
> > See the bug.
> > 
> >   eval('let x; var x;')
> > 
> > is more similar to
> > 
> >   eval('{ let x; var x; }')
> > 
> > we are not creating the lexical block correctly for sloppy evals so these
> tests
> > are not passing or the tests are not correct for sloppy mode.
> 
> I suppose
> 
> let x; eval('var x');
> 
> is an error in sloppy mode, but not in strict mode. That's a fairly
unfortunate
> semantic deviation, actually. But why does this difference only pop up in the
> global scope tests?

The other tests wrap the string in a function or a block so the implicit lexical
scope is not relevant.

> These tests should probably be changed to use Realm.eval in TestGlobal, so
that
> we test the actual global scope, and there should be a separate TestEval
> (similarly for strict mode). Can you add a TODO for that?

I'll add a todo (and/or add a comment to the bug)

[arv (Not doing code reviews), 2015-07-10 16:02:57]

more comments and git rebase

[arv (Not doing code reviews), 2015-07-10 16:09:27]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-07-10 16:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1226103002/#ps60001
(title: "more comments and git rebase")

[commit-bot: I haz the power, 2015-07-10 16:09:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226103002/60001

[commit-bot: I haz the power, 2015-07-10 16:39:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-10 16:40:04]

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