Implement sloppy-mode block-defined functions (Annex B 3.3)

Implement sloppy-mode block-defined functions (Annex B 3.3)

ES2015 specifies very particular semantics for functions defined in blocks.
In strict mode, it is simply a lexical binding scoped to that block. In sloppy
mode, in addition to that lexical binding, there is a var-style binding in
the outer scope, which is overwritten with the local binding when the function
declaration is evaluated, *as long as* introducing ths var binding would not
create a var/let conflict in the outer scope.

This patch implements the semantics by introducing a DelegateStatement, which
is initially filled in with the EmptyStatement and overwritten with the
assignment when the scope is closed out and it can be checked that there is
no conflict.

This patch is tested with a new mjsunit test, and I tried staging it and running
test262, finding that the tests that we have disabled due to lack of Annex B
support now pass.

R=adamk,rossberg
LOG=Y
BUG=v8:4285
Committed: https://crrev.com/e5ff10d7673a723b1b62b1ddfca194265aaff84c
Cr-Commit-Position: refs/heads/master@{#30842}

[Dan Ehrenberg, 2015-09-10 19:21:27]

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

[commit-bot: I haz the power, 2015-09-10 19:21:37]

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

[commit-bot: I haz the power, 2015-09-10 19:32:27]

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

[commit-bot: I haz the power, 2015-09-10 19:32:27]

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

[Dan Ehrenberg, 2015-09-10 23:24:07]

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

[commit-bot: I haz the power, 2015-09-10 23:24:15]

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

[Dan Ehrenberg, 2015-09-11 14:09:36]

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

[commit-bot: I haz the power, 2015-09-11 14:09:42]

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

[commit-bot: I haz the power, 2015-09-11 14:35:45]

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

[commit-bot: I haz the power, 2015-09-11 14:35:46]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-09-11 15:42:57]

First round of comments, mostly style nits.  The juicy bits are the handling of
scopes in the fixup code, and some suggestions for tests to exercise interaction
with other fancy scoping features.

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1273
src/ast.h:1273: // Delegates to another statement, which may be overwritten.
Nit: class comments should go right above the declaration.

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1280
src/ast.h:1280: Statement* statement() { return statement_; }
Make this a const method

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1282
src/ast.h:1282: Scope* scope() { return scope_; }
ditto: const please

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1284
src/ast.h:1284: protected:
I realize the use of "protected" here matches some other cases, but it makes no
sense to me, since this is a "final" class. Can this just be private?

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1285
src/ast.h:1285: explicit DelegateStatement(Zone* zone, int pos, Statement*
statement,
No need for "explicit" here.

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1287
src/ast.h:1287: : Statement(zone, pos), statement_(statement), scope_(scope) {}
Could you just pass RelocInfo::kNoPosition to the base class here and avoid
accepting a |pos| argument? I can't imagine a use-case for a DelegateStatement
that actually has a position.

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4954
src/parser.cc:4954: void Parser::InsertSloppyBlockFunctionVarBindings(Scope*
scope, bool* ok) {
Can you add a DCHECK that the passed-in scope is a declaration scope?

Also, how does this interact with the case where a function has two var
declaration scopes (as it does for sloppy non-simple argument lists)?

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4958
src/parser.cc:4958: for (ZoneHashMap::Entry* p = map->Start(); p != NULL; p =
map->Next(p)) {
Nit: I think "nullptr" is preferred these days over "NULL"

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4959
src/parser.cc:4959: AstRawString* name =
reinterpret_cast<AstRawString*>(p->key);
Can this just be a static_cast? Also, please re-constify it:

static_cast<const AstRawString*>(p->key)

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4962
src/parser.cc:4962: if (var == NULL || !IsLexicalVariableMode(var->mode())) {
s/NULL/nullptr/

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4967
src/parser.cc:4967: Declare(declaration, DeclarationDescriptor::NORMAL, true,
ok);
I'm a little bit confused: why do you use the passed-in scope above, but the
current scope_ here (since you don't pass the last argument)? Are |scope| and
|scope_| actually the same here?

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4973
src/parser.cc:4973: reinterpret_cast<ZoneVector<DelegateStatement*>*>(p->value);
Same here, please static_cast instead of reinterpret_cast

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto stmt = delegates->begin(); stmt <
delegates->end(); stmt++) {
You can use more C++11 here:

for (auto stmt : delegates)

That'll also make stmt actually a Statement*, instead of an iterator.

Also, "delegate" is probably a better name for the loop var, since you're
already using "statement" below.

https://codereview.chromium.org/1332873003/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1332873003/diff/80001/src/parser.h#newcode1137
src/parser.h:1137: void InsertSloppyBlockFunctionVarBindings(Scope* scope, bool*
ok);
I think this deserves a quick comment, perhaps just a reference to the spec.

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode79
src/scopes.cc:79: // FIXME(marja): fix the type of Lookup.
What's this TODO about? The return value seems fine to me.

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode83
src/scopes.cc:83: if (!p->value) {
Nit: p->value == nullptr is the v8 style here, generally.

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode84
src/scopes.cc:84: p->value = new
(zone_->New(sizeof(ZoneVector<DelegateStatement*>)))
Might be worth typedefing ZoneVector<DelegateStatement*> to something as you
have to refer to it a bunch in this method.

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

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:5: // Flags: --no-legacy-const
--harmony-sloppy --harmony-sloppy-let --harmony-sloppy-function
You can have multiple "Flags:" lines to keep this under 80 chars

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:7: // Test Annex B 3.3 semantics
for functions declared in blocks in sloppy mode.
Can you add tests for shadowing function params, both for simple parameter lists
and complex? I'm not actually sure what'll happen with destructured param names
at the moment, given that they're LET-bound, but I'd like to know.

Also would be interesting to see this shadow "arguments".

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:147: // Test that hoisting from
blocks doesn't happen in global scope
How is this effect achieved?

[Dan Ehrenberg, 2015-09-17 17:44:10]

Haven't figured out tests for this scoping issue, but fixed all the other review
comments

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1273
src/ast.h:1273: // Delegates to another statement, which may be overwritten.
On 2015/09/11 at 15:42:56, adamk wrote:
> Nit: class comments should go right above the declaration.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1280
src/ast.h:1280: Statement* statement() { return statement_; }
On 2015/09/11 at 15:42:56, adamk wrote:
> Make this a const method

Done

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1282
src/ast.h:1282: Scope* scope() { return scope_; }
On 2015/09/11 at 15:42:56, adamk wrote:
> ditto: const please

Done

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1284
src/ast.h:1284: protected:
On 2015/09/11 at 15:42:56, adamk wrote:
> I realize the use of "protected" here matches some other cases, but it makes
no sense to me, since this is a "final" class. Can this just be private?

Done

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1285
src/ast.h:1285: explicit DelegateStatement(Zone* zone, int pos, Statement*
statement,
On 2015/09/11 at 15:42:56, adamk wrote:
> No need for "explicit" here.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1287
src/ast.h:1287: : Statement(zone, pos), statement_(statement), scope_(scope) {}
On 2015/09/11 at 15:42:56, adamk wrote:
> Could you just pass RelocInfo::kNoPosition to the base class here and avoid
accepting a |pos| argument? I can't imagine a use-case for a DelegateStatement
that actually has a position.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4954
src/parser.cc:4954: void Parser::InsertSloppyBlockFunctionVarBindings(Scope*
scope, bool* ok) {
On 2015/09/11 at 15:42:56, adamk wrote:
> Can you add a DCHECK that the passed-in scope is a declaration scope?
> 
> Also, how does this interact with the case where a function has two var
declaration scopes (as it does for sloppy non-simple argument lists)?

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4958
src/parser.cc:4958: for (ZoneHashMap::Entry* p = map->Start(); p != NULL; p =
map->Next(p)) {
On 2015/09/11 at 15:42:56, adamk wrote:
> Nit: I think "nullptr" is preferred these days over "NULL"

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4959
src/parser.cc:4959: AstRawString* name =
reinterpret_cast<AstRawString*>(p->key);
On 2015/09/11 at 15:42:56, adamk wrote:
> Can this just be a static_cast? Also, please re-constify it:
> 
> static_cast<const AstRawString*>(p->key)

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4962
src/parser.cc:4962: if (var == NULL || !IsLexicalVariableMode(var->mode())) {
On 2015/09/11 at 15:42:56, adamk wrote:
> s/NULL/nullptr/

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4967
src/parser.cc:4967: Declare(declaration, DeclarationDescriptor::NORMAL, true,
ok);
On 2015/09/11 at 15:42:56, adamk wrote:
> I'm a little bit confused: why do you use the passed-in scope above, but the
current scope_ here (since you don't pass the last argument)? Are |scope| and
|scope_| actually the same here?

It happens to work because this is called when coming back out to the function
scope, but that's unclear, so I passed scope.

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4973
src/parser.cc:4973: reinterpret_cast<ZoneVector<DelegateStatement*>*>(p->value);
On 2015/09/11 at 15:42:56, adamk wrote:
> Same here, please static_cast instead of reinterpret_cast

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto stmt = delegates->begin(); stmt <
delegates->end(); stmt++) {
On 2015/09/11 at 15:42:56, adamk wrote:
> You can use more C++11 here:
> 
> for (auto stmt : delegates)
> 
> That'll also make stmt actually a Statement*, instead of an iterator.
> 
> Also, "delegate" is probably a better name for the loop var, since you're
already using "statement" below.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1332873003/diff/80001/src/parser.h#newcode1137
src/parser.h:1137: void InsertSloppyBlockFunctionVarBindings(Scope* scope, bool*
ok);
On 2015/09/11 at 15:42:56, adamk wrote:
> I think this deserves a quick comment, perhaps just a reference to the spec.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode79
src/scopes.cc:79: // FIXME(marja): fix the type of Lookup.
On 2015/09/11 at 15:42:56, adamk wrote:
> What's this TODO about? The return value seems fine to me.

I think it's about using the hash. I copied it from the other map. Probably this
is not so useful; deleted the TODO.

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode83
src/scopes.cc:83: if (!p->value) {
On 2015/09/11 at 15:42:56, adamk wrote:
> Nit: p->value == nullptr is the v8 style here, generally.

Done

https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode84
src/scopes.cc:84: p->value = new
(zone_->New(sizeof(ZoneVector<DelegateStatement*>)))
On 2015/09/11 at 15:42:56, adamk wrote:
> Might be worth typedefing ZoneVector<DelegateStatement*> to something as you
have to refer to it a bunch in this method.

Done

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

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:5: // Flags: --no-legacy-const
--harmony-sloppy --harmony-sloppy-let --harmony-sloppy-function
On 2015/09/11 at 15:42:56, adamk wrote:
> You can have multiple "Flags:" lines to keep this under 80 chars

Done

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:147: // Test that hoisting from
blocks doesn't happen in global scope
On 2015/09/11 at 15:42:56, adamk wrote:
> How is this effect achieved?

By 'global' I meant 'not in a function'. This is tested because we see that the
local globalUnhoisted isn't hoisted, and we reach the outer one, because its
return value is 0 not 1.

[Dan Ehrenberg, 2015-09-17 17:44:21]

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

[commit-bot: I haz the power, 2015-09-17 17:44:31]

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

[Dan Ehrenberg, 2015-09-17 18:10:27]

PTAL

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

https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/bl...
test/mjsunit/harmony/block-sloppy-function.js:7: // Test Annex B 3.3 semantics
for functions declared in blocks in sloppy mode.
On 2015/09/11 at 15:42:56, adamk wrote:
> Can you add tests for shadowing function params, both for simple parameter
lists and complex? I'm not actually sure what'll happen with destructured param
names at the moment, given that they're LET-bound, but I'd like to know.
> 
> Also would be interesting to see this shadow "arguments".

Added tests. As you predicted, one of them fails, so I commented out the key
assertion.

[adamk, 2015-09-17 18:44:36]

lgtm % naming

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4967
src/parser.cc:4967: Declare(declaration, DeclarationDescriptor::NORMAL, true,
ok);
On 2015/09/17 17:44:10, Dan Ehrenberg wrote:
> On 2015/09/11 at 15:42:56, adamk wrote:
> > I'm a little bit confused: why do you use the passed-in scope above, but the
> current scope_ here (since you don't pass the last argument)? Are |scope| and
> |scope_| actually the same here?
> 
> It happens to work because this is called when coming back out to the function
> scope, but that's unclear, so I passed scope.

Thanks, this looks better. A later cleanup (not in this CL) should probably make
that argument non-optional and make Declare static, to avoid other member
accesses of scope_creeping in.

https://codereview.chromium.org/1332873003/diff/120001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1332873003/diff/120001/src/ast.h#newcode1276
src/ast.h:1276: class DelegateStatement final : public Statement {
I thought this was going to get renamed? I believe the proposal was
"SloppyBlockFunctionStatement"?

https://codereview.chromium.org/1332873003/diff/120001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1332873003/diff/120001/src/scopes.h#newcode74
src/scopes.h:74: typedef ZoneVector<DelegateStatement*> DelegateVector;
This typedef shouldn't be at toplevel (the name is too general). How about
making it public in SloppyBlockFunctionMap, instead?

[Dan Ehrenberg, 2015-09-17 19:06:03]

https://codereview.chromium.org/1332873003/diff/120001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1332873003/diff/120001/src/ast.h#newcode1276
src/ast.h:1276: class DelegateStatement final : public Statement {
On 2015/09/17 at 18:44:36, adamk wrote:
> I thought this was going to get renamed? I believe the proposal was
"SloppyBlockFunctionStatement"?

Done

https://codereview.chromium.org/1332873003/diff/120001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1332873003/diff/120001/src/scopes.h#newcode74
src/scopes.h:74: typedef ZoneVector<DelegateStatement*> DelegateVector;
On 2015/09/17 at 18:44:36, adamk wrote:
> This typedef shouldn't be at toplevel (the name is too general). How about
making it public in SloppyBlockFunctionMap, instead?

How's the new name, SloppyBlockFunctionMap::Vector?

[Dan Ehrenberg, 2015-09-17 19:06:17]

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

[commit-bot: I haz the power, 2015-09-17 19:06:27]

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

[adamk, 2015-09-17 19:20:06]

lgtm % a C++11 nit

https://codereview.chromium.org/1332873003/diff/160001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1332873003/diff/160001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto delegate : *delegates) {
With the renaming, I think this would be clearer as:

for (SloppyBlockFunctionStatement* delegate : *delegates)

since it's otherwise unclear what the type of delegate is.

[adamk, 2015-09-17 19:26:31]

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

[adamk, 2015-09-17 19:26:32]

+bmeurer for various compiler OWNERs

[commit-bot: I haz the power, 2015-09-17 19:31:17]

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

[commit-bot: I haz the power, 2015-09-17 19:31:18]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-09-17 19:48:36]

https://codereview.chromium.org/1332873003/diff/160001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1332873003/diff/160001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto delegate : *delegates) {
On 2015/09/17 at 19:20:06, adamk wrote:
> With the renaming, I think this would be clearer as:
> 
> for (SloppyBlockFunctionStatement* delegate : *delegates)
> 
> since it's otherwise unclear what the type of delegate is.

Done

[Dan Ehrenberg, 2015-09-18 02:23:24]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-09-18 02:23:24]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1332873003/#ps180001
(title: "Improve type clarity")

[commit-bot: I haz the power, 2015-09-18 02:23:31]

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

[commit-bot: I haz the power, 2015-09-18 02:31:19]

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

[commit-bot: I haz the power, 2015-09-18 02:31:19]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5912)

[Benedikt Meurer, 2015-09-19 16:51:53]

LGTM on compiler and fullcodegen changes; only briefly checked the rest.

[Dan Ehrenberg, 2015-09-21 04:04:51]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-09-21 04:05:01]

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

[commit-bot: I haz the power, 2015-09-21 04:30:56]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-21 04:31:17]

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