Extend Annex B 3.3 sloppy-mode block-scoped hoisting to scripts, eval

Extend Annex B 3.3 sloppy-mode block-scoped hoisting to scripts, eval

The ES2015 spec is missing an extension of sloppy-mode block-scoped function
behavior to the global scope in scripts, as well as to eval. This patch
brings that hoisting to those two areas. The behavior is not perfectly
spec-compliant since properties created on the global scope should be
set as enumerable even if they are non-enumerable previously, but the
attributes will not be modified if the property already exists under
this patch.

BUG=v8:4441
LOG=Y
R=adamk
TEST=reddit comment functionality seems to be fixed

Committed: https://crrev.com/76da493315bcd0fdf6b2bbecc462d8546e6a4cc0
Cr-Commit-Position: refs/heads/master@{#31037}

[Dan Ehrenberg, 2015-09-29 22:50:52]

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

[Dan Ehrenberg, 2015-09-29 22:50:57]

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

[commit-bot: I haz the power, 2015-09-29 22:51:19]

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

[commit-bot: I haz the power, 2015-09-29 22:57:12]

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

[commit-bot: I haz the power, 2015-09-29 22:57:13]

Dry run: Try jobs failed on following builders:
  v8_win64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/9126)

[Dan Ehrenberg, 2015-09-29 23:06:23]

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

[commit-bot: I haz the power, 2015-09-29 23:06:33]

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

[Dan Ehrenberg, 2015-09-29 23:06:57]

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

[commit-bot: I haz the power, 2015-09-29 23:07:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376623002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376623002/60001

[adamk, 2015-09-29 23:23:15]

Code looks good, but needs more tests.

https://codereview.chromium.org/1376623002/diff/60001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-sloppy-function.js (right):

https://codereview.chromium.org/1376623002/diff/60001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:149: // Test that hoisting from
blocks does happen in global scope
More tests, please:

- Negative cases: does not hoist if there's a lexical in global scope (and test
the same in the eval case).

- Cases where there's no overlapping function already in the global; in that
case, add assertions that the property is created on the global (to make sure
the binding went in the right place), and with the expected attributes.

- Also add tests with a frozen global (does not hoist, does not throw).

- Finally, tests where you set up a global property with different attributes
from what you expect, and assertions of the actual current behavior (with a TODO
to fix).

Given how much of this is global, you might have to put some of these in
separate files...

[commit-bot: I haz the power, 2015-09-29 23:30:57]

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

[commit-bot: I haz the power, 2015-09-29 23:30:58]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-09-30 16:25:29]

Added some tests, but a number are commented out because they fail. The worst of
them is that the path of checking whether there was a lexically declared thing
that conflicts seems to not work. How many of these edge cases do you think need
to be fixed before this patch is ready?

[adamk, 2015-09-30 18:24:26]

I think only the final one of these tests needs to be fixed before landing (but
as I noted, the assertions should be left uncommented where they are just
providing a single bit of data about the state of the world).

https://codereview.chromium.org/1376623002/diff/80001/test/mjsunit/harmony/bl...
File test/mjsunit/harmony/block-sloppy-function.js (right):

https://codereview.chromium.org/1376623002/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:214: //
assertTrue(globalUndefinedHoistedDescriptor.configurable);
Please leave this uncommented (with the current behavior) so that changes to the
code will require changes to the test.

https://codereview.chromium.org/1376623002/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:234: //
assertTrue(globalNonEnumerableDescriptor.enumerable);
Same here.

https://codereview.chromium.org/1376623002/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:258: // Currently this actually
throws BUG(v8:4452)
Ah, this throws because it goes through the defineProperty path instead of the
assignment path? I'm OK landing without this fix, since it's the same as the
pre-ES6 behavior.

https://codereview.chromium.org/1376623002/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:273: // OK, this really needs to
be fixed...
Yeah, this needs to be fixed before landing.

[Dan Ehrenberg, 2015-09-30 23:06:55]

https://chromiumcodereview.appspot.com/1376623002/diff/80001/test/mjsunit/har...
File test/mjsunit/harmony/block-sloppy-function.js (right):

https://chromiumcodereview.appspot.com/1376623002/diff/80001/test/mjsunit/har...
test/mjsunit/harmony/block-sloppy-function.js:214: //
assertTrue(globalUndefinedHoistedDescriptor.configurable);
On 2015/09/30 at 18:24:26, adamk wrote:
> Please leave this uncommented (with the current behavior) so that changes to
the code will require changes to the test.

Done

https://chromiumcodereview.appspot.com/1376623002/diff/80001/test/mjsunit/har...
test/mjsunit/harmony/block-sloppy-function.js:234: //
assertTrue(globalNonEnumerableDescriptor.enumerable);
On 2015/09/30 at 18:24:26, adamk wrote:
> Same here.

Done

https://chromiumcodereview.appspot.com/1376623002/diff/80001/test/mjsunit/har...
test/mjsunit/harmony/block-sloppy-function.js:258: // Currently this actually
throws BUG(v8:4452)
On 2015/09/30 at 18:24:26, adamk wrote:
> Ah, this throws because it goes through the defineProperty path instead of the
assignment path? I'm OK landing without this fix, since it's the same as the
pre-ES6 behavior.

I guess it matches, but still, we eventually want to make the illusion that
functions are lexical, so this should eventually work. Still I don't know when
you'd freeze the global object and eval a function declaration in global scope
like this...

https://chromiumcodereview.appspot.com/1376623002/diff/80001/test/mjsunit/har...
test/mjsunit/harmony/block-sloppy-function.js:273: // OK, this really needs to
be fixed...
On 2015/09/30 at 18:24:26, adamk wrote:
> Yeah, this needs to be fixed before landing.

We talked about this off-line and it sounded like maybe it can be fixed in a
follow-on patch. It'll be a bit of work to fix, and this path isn't staged
anymore.

[adamk, 2015-09-30 23:13:25]

One last test tweak, lgtm once that's added.

https://chromiumcodereview.appspot.com/1376623002/diff/100001/test/mjsunit/ha...
File test/mjsunit/harmony/block-sloppy-function.js (right):

https://chromiumcodereview.appspot.com/1376623002/diff/100001/test/mjsunit/ha...
test/mjsunit/harmony/block-sloppy-function.js:224: writable: true,
One more test tweak: can you make this "writable: false", and add a test below
that it gets reset to true?

[Dan Ehrenberg, 2015-09-30 23:27:21]

https://chromiumcodereview.appspot.com/1376623002/diff/100001/test/mjsunit/ha...
File test/mjsunit/harmony/block-sloppy-function.js (right):

https://chromiumcodereview.appspot.com/1376623002/diff/100001/test/mjsunit/ha...
test/mjsunit/harmony/block-sloppy-function.js:224: writable: true,
On 2015/09/30 at 23:13:25, adamk wrote:
> One more test tweak: can you make this "writable: false", and add a test below
that it gets reset to true?

Done (but in a separate test case)

[Dan Ehrenberg, 2015-09-30 23:27:28]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-09-30 23:27:28]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1376623002/#ps120001 (title: "An
additional test")

[commit-bot: I haz the power, 2015-09-30 23:27:46]

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

[commit-bot: I haz the power, 2015-09-30 23:48:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-30 23:48:45]

Patchset 7 (id:??) landed as
https://crrev.com/76da493315bcd0fdf6b2bbecc462d8546e6a4cc0
Cr-Commit-Position: refs/heads/master@{#31037}