Sloppy mode webcompat: allow conflicting function declarations in blocks

Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y

Committed: https://crrev.com/8aeb6080e11548c018e898a078a162c23a45f7b8
Cr-Commit-Position: refs/heads/master@{#33478}

[adamk, 2016-01-22 23:33:02]

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

[Dan Ehrenberg, 2016-01-22 23:47:06]

lgtm

Lgtm but I'd prefer some more tests to nail down the semantics, namely
- what happens to the hoisted definition  when you break out of the block before
reaching the second one (I guess we'd get the second, which is
counter-intuitive)
- what happens when the two functions are declared in different scope levels,
but still left-conflicting (looks like this would be permitted, but it's be
possible to mitigate the issue more narrowly)
- verify that conflicts still happen between let and function, and still in
strict mode

But I don't see a need to write those tests before submitting this patch.

[adamk, 2016-01-22 23:56:35]

On 2016/01/22 23:47:06, Dan Ehrenberg wrote:
> lgtm
> 
> Lgtm but I'd prefer some more tests to nail down the semantics, namely
> - what happens to the hoisted definition  when you break out of the block
before
> reaching the second one (I guess we'd get the second, which is
> counter-intuitive)

Yes, this is just how the hoisting works (it happens before any code runs). This
isn't specific to the block scoping, it'd happen with any duplicated function
declarations. (I agree that it would be good to see tests for this, but I do
think they can come later).

> - what happens when the two functions are declared in different scope levels,
> but still left-conflicting (looks like this would be permitted, but it's be
> possible to mitigate the issue more narrowly)
> - verify that conflicts still happen between let and function, and still in
> strict mode

First half is already handled in mjsunit/harmony/block-conflicts-sloppy, second
part I've added to the regression test (I think it's important to demonstrate
that we're not affecting strict mode).

> But I don't see a need to write those tests before submitting this patch.

[adamk, 2016-01-22 23:57:12]

Description was changed from

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
==========

to

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y
==========

[adamk, 2016-01-23 00:04:32]

Also added two Annex B-related asserts.

[adamk, 2016-01-23 00:04:51]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-01-23 00:04:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1622723003/#ps40001
(title: "Add Annex B checks to test")

[commit-bot: I haz the power, 2016-01-23 00:04:56]

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

[Dan Ehrenberg, 2016-01-23 00:08:13]

On 2016/01/22 at 23:56:35, adamk wrote:
> On 2016/01/22 23:47:06, Dan Ehrenberg wrote:
> > lgtm
> > 
> > Lgtm but I'd prefer some more tests to nail down the semantics, namely
> > - what happens to the hoisted definition  when you break out of the block
before
> > reaching the second one (I guess we'd get the second, which is
> > counter-intuitive)
> 
> Yes, this is just how the hoisting works (it happens before any code runs).
This isn't specific to the block scoping, it'd happen with any duplicated
function declarations. (I agree that it would be good to see tests for this, but
I do think they can come later).

Well, it's a little less regular than that. For example, here, we'd be left with
the first definition (examples untested):

!function() {
  L: {
    { function f() { return 1; } }
    break L;
    { function f() { return 2; } }
  }
  return f(); // 1
}()

However, if you leave out some brackets, you get different behavior, I'd expect

!function() {
  L: {
    function f() { return 1; }
    break L;
    function f() { return 2; }
  }
  return f(); // 2
}()
> 
> > - what happens when the two functions are declared in different scope
levels,
> > but still left-conflicting (looks like this would be permitted, but it's be
> > possible to mitigate the issue more narrowly)

I think you changed the semantics of this to be permitted, when it wasn't
previously, and doesn't need to be to fix the test cases we know about:

!function() {
  {
    function f() { }
    {
       function f() { }
    }
  }
}()

Is this tested somewhere?

> > - verify that conflicts still happen between let and function, and still in
> > strict mode
> 
> First half is already handled in mjsunit/harmony/block-conflicts-sloppy,
second part I've added to the regression test (I think it's important to
demonstrate that we're not affecting strict mode).
> 
> > But I don't see a need to write those tests before submitting this patch.

[adamk, 2016-01-23 00:09:45]

The CQ bit was unchecked by adamk@chromium.org

[adamk, 2016-01-23 00:12:41]

On 2016/01/23 00:08:13, Dan Ehrenberg wrote:
> On 2016/01/22 at 23:56:35, adamk wrote:
> > On 2016/01/22 23:47:06, Dan Ehrenberg wrote:
> > > lgtm
> > > 
> > > Lgtm but I'd prefer some more tests to nail down the semantics, namely
> > > - what happens to the hoisted definition  when you break out of the block
> before
> > > reaching the second one (I guess we'd get the second, which is
> > > counter-intuitive)
> > 
> > Yes, this is just how the hoisting works (it happens before any code runs).
> This isn't specific to the block scoping, it'd happen with any duplicated
> function declarations. (I agree that it would be good to see tests for this,
but
> I do think they can come later).
> 
> Well, it's a little less regular than that. For example, here, we'd be left
with
> the first definition (examples untested):
> 
> !function() {
>   L: {
>     { function f() { return 1; } }
>     break L;
>     { function f() { return 2; } }
>   }
>   return f(); // 1
> }()
> 
> However, if you leave out some brackets, you get different behavior, I'd
expect
> 
> !function() {
>   L: {
>     function f() { return 1; }
>     break L;
>     function f() { return 2; }
>   }
>   return f(); // 2
> }()

This could well be, haven't tested yet.

> > 
> > > - what happens when the two functions are declared in different scope
> levels,
> > > but still left-conflicting (looks like this would be permitted, but it's
be
> > > possible to mitigate the issue more narrowly)
> 
> I think you changed the semantics of this to be permitted, when it wasn't
> previously, and doesn't need to be to fix the test cases we know about:
> 
> !function() {
>   {
>     function f() { }
>     {
>        function f() { }
>     }
>   }
> }()
> 
> Is this tested somewhere?

That one worked before and still does (and I don't see why it shouldn't). This
patch only changes things for declarations in the same scope.

> > > - verify that conflicts still happen between let and function, and still
in
> > > strict mode
> > 
> > First half is already handled in mjsunit/harmony/block-conflicts-sloppy,
> second part I've added to the regression test (I think it's important to
> demonstrate that we're not affecting strict mode).
> > 
> > > But I don't see a need to write those tests before submitting this patch.

[adamk, 2016-01-23 00:12:46]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-01-23 00:13:00]

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

[Dan Ehrenberg, 2016-01-23 00:15:00]

On 2016/01/23 at 00:08:13, Dan Ehrenberg wrote:
> On 2016/01/22 at 23:56:35, adamk wrote:
> > On 2016/01/22 23:47:06, Dan Ehrenberg wrote:
> > > lgtm
> > > 
> > > Lgtm but I'd prefer some more tests to nail down the semantics, namely
> > > - what happens to the hoisted definition  when you break out of the block
before
> > > reaching the second one (I guess we'd get the second, which is
> > > counter-intuitive)
> > 
> > Yes, this is just how the hoisting works (it happens before any code runs).
This isn't specific to the block scoping, it'd happen with any duplicated
function declarations. (I agree that it would be good to see tests for this, but
I do think they can come later).
> 
> Well, it's a little less regular than that. For example, here, we'd be left
with the first definition (examples untested):
> 
> !function() {
>   L: {
>     { function f() { return 1; } }
>     break L;
>     { function f() { return 2; } }
>   }
>   return f(); // 1
> }()
> 
> However, if you leave out some brackets, you get different behavior, I'd
expect
> 
> !function() {
>   L: {
>     function f() { return 1; }
>     break L;
>     function f() { return 2; }
>   }
>   return f(); // 2
> }()
> > 
> > > - what happens when the two functions are declared in different scope
levels,
> > > but still left-conflicting (looks like this would be permitted, but it's
be
> > > possible to mitigate the issue more narrowly)
> 
> I think you changed the semantics of this to be permitted, when it wasn't
previously, and doesn't need to be to fix the test cases we know about:
> 
> !function() {
>   {
>     function f() { }
>     {
>        function f() { }
>     }
>   }
> }()
> 
> Is this tested somewhere?
> 
> > > - verify that conflicts still happen between let and function, and still
in
> > > strict mode
> > 
> > First half is already handled in mjsunit/harmony/block-conflicts-sloppy,
second part I've added to the regression test (I think it's important to
demonstrate that we're not affecting strict mode).
> > 
> > > But I don't see a need to write those tests before submitting this patch.

FYI examples work as expected, but you'll get more useful results if you replace
! with +

[Dan Ehrenberg, 2016-01-23 00:16:06]

On 2016/01/23 at 00:12:41, adamk wrote:
> On 2016/01/23 00:08:13, Dan Ehrenberg wrote:
> > On 2016/01/22 at 23:56:35, adamk wrote:
> > > On 2016/01/22 23:47:06, Dan Ehrenberg wrote:
> > > > lgtm
> > > > 
> > > > Lgtm but I'd prefer some more tests to nail down the semantics, namely
> > > > - what happens to the hoisted definition  when you break out of the
block
> > before
> > > > reaching the second one (I guess we'd get the second, which is
> > > > counter-intuitive)
> > > 
> > > Yes, this is just how the hoisting works (it happens before any code
runs).
> > This isn't specific to the block scoping, it'd happen with any duplicated
> > function declarations. (I agree that it would be good to see tests for this,
but
> > I do think they can come later).
> > 
> > Well, it's a little less regular than that. For example, here, we'd be left
with
> > the first definition (examples untested):
> > 
> > !function() {
> >   L: {
> >     { function f() { return 1; } }
> >     break L;
> >     { function f() { return 2; } }
> >   }
> >   return f(); // 1
> > }()
> > 
> > However, if you leave out some brackets, you get different behavior, I'd
expect
> > 
> > !function() {
> >   L: {
> >     function f() { return 1; }
> >     break L;
> >     function f() { return 2; }
> >   }
> >   return f(); // 2
> > }()
> 
> This could well be, haven't tested yet.
> 
> > > 
> > > > - what happens when the two functions are declared in different scope
> > levels,
> > > > but still left-conflicting (looks like this would be permitted, but it's
be
> > > > possible to mitigate the issue more narrowly)
> > 
> > I think you changed the semantics of this to be permitted, when it wasn't
> > previously, and doesn't need to be to fix the test cases we know about:
> > 
> > !function() {
> >   {
> >     function f() { }
> >     {
> >        function f() { }
> >     }
> >   }
> > }()
> > 
> > Is this tested somewhere?
> 
> That one worked before and still does (and I don't see why it shouldn't). This
patch only changes things for declarations in the same scope.

Oh, sorry, that's just a case where hoisting is blocked for the inner one. No
issue there.
> 
> > > > - verify that conflicts still happen between let and function, and still
in
> > > > strict mode
> > > 
> > > First half is already handled in mjsunit/harmony/block-conflicts-sloppy,
> > second part I've added to the regression test (I think it's important to
> > demonstrate that we're not affecting strict mode).
> > > 
> > > > But I don't see a need to write those tests before submitting this
patch.

[commit-bot: I haz the power, 2016-01-23 00:40:21]

Description was changed from

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y
==========

to

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y
==========

[commit-bot: I haz the power, 2016-01-23 00:40:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-23 00:41:02]

Description was changed from

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y
==========

to

==========
Sloppy mode webcompat: allow conflicting function declarations in blocks

The web appears to depend on being able to redeclare functions-in-blocks
in sloppy mode (examples seen so far tend to redeclare identical functions,
most likely accidentally).

This patch opens a minimal hole: two same-named function declarations
in the same scope are allowed, only in sloppy mode.

BUG=v8:4693, chromium:579395
LOG=y

Committed: https://crrev.com/8aeb6080e11548c018e898a078a162c23a45f7b8
Cr-Commit-Position: refs/heads/master@{#33478}
==========

[commit-bot: I haz the power, 2016-01-23 00:41:03]

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

[Michael Achenbach, 2016-01-25 08:09:27]

Could you have a look why the dopt fuzzer isn't happy about this change?
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/7477

Search for "webkit/function-declarations-in-switch-statement" in the output.

[adamk, 2016-01-25 19:10:17]

On 2016/01/25 08:09:27, Michael Achenbach wrote:
> Could you have a look why the dopt fuzzer isn't happy about this change?
>
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/7477
> 
> Search for "webkit/function-declarations-in-switch-statement" in the output.

Looks like littledan took care of this in
https://codereview.chromium.org/1628013003?

I'm confused as to why this started failing on the Deopt Fuzzer, though, since
the test was already failing and marked "FAIL"; it just changed the way in which
it was failing.

[Dan Ehrenberg, 2016-01-25 20:04:03]

On 2016/01/25 at 19:10:17, adamk wrote:
> On 2016/01/25 08:09:27, Michael Achenbach wrote:
> > Could you have a look why the dopt fuzzer isn't happy about this change?
> >
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/7477
> > 
> > Search for "webkit/function-declarations-in-switch-statement" in the output.
> 
> Looks like littledan took care of this in
https://codereview.chromium.org/1628013003?
> 
> I'm confused as to why this started failing on the Deopt Fuzzer, though, since
the test was already failing and marked "FAIL"; it just changed the way in which
it was failing.

Turns out the deopt fuzzer doesn't interpret failing test expectations all that
well. Previously, it was a syntax error, and with your patch, it became an
expectations mismatch. Long-term, this should probably be fixed in the test
infrastructure, but that patch seems to be enough for now.