[parser] use-strict directives in function body affect init block

[parser] use-strict directives in function body affect init block

BUG=
LOG=N
R=rossberg@chromium.org

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

[caitp (gmail), 2015-07-15 20:31:39]

caitpotter88@gmail.com changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[caitp (gmail), 2015-07-15 20:31:39]

PTAL --- first attempt at improving commutativity of strictness-altering
directives within a function body, so that they affect init blocks
(destructuring initializers, optional parameter initializers, and rest
parameters when implemented via desugaring).

This is split out from crrev.com/1235153006 in order to make review a bit easier

Does not yet re-trigger errors which occur during parsing

[caitp (gmail), 2015-07-15 20:35:44]

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
File test/mjsunit/strong/destructuring.js (right):

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
test/mjsunit/strong/destructuring.js:10: var a = f({ x: undefined });
this move kind of makes destructuring initializers not very good, but maybe this
is actually intentional? Not sure.

[rossberg, 2015-07-16 12:53:47]

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4310
src/parser.cc:4310: bool Parser::NeedsParameterInitializationBlock(
Make this into IsSimpleParameterList, to match what's in the spec. In
particular, it needs to check for rest as well.

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4383
src/parser.cc:4383: if (fvar != nullptr) {
Hm, why do you need to move (and duplicate) fvar initialisation and IsConstruct
assert?

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
File test/mjsunit/strong/destructuring.js (right):

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
test/mjsunit/strong/destructuring.js:10: var a = f({ x: undefined });
On 2015/07/15 20:35:44, caitp wrote:
> this move kind of makes destructuring initializers not very good, but maybe
this
> is actually intentional? Not sure.

Not sure I understand. What's not good?

Btw, language mode vs parameter destructuring is a much bigger problem than I
thought yesterday. Will post something later.

[caitp (gmail), 2015-07-16 13:03:57]

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4310
src/parser.cc:4310: bool Parser::NeedsParameterInitializationBlock(
On 2015/07/16 12:53:47, rossberg wrote:
> Make this into IsSimpleParameterList, to match what's in the spec. In
> particular, it needs to check for rest as well.

Acknowledged.

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4383
src/parser.cc:4383: if (fvar != nullptr) {
On 2015/07/16 12:53:47, rossberg wrote:
> Hm, why do you need to move (and duplicate) fvar initialisation and
IsConstruct
> assert?

it seems that these need to happen before the init block. I suppose it could
just use `result->insertAt(0)` instead though

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
File test/mjsunit/strong/destructuring.js (right):

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
test/mjsunit/strong/destructuring.js:10: var a = f({ x: undefined });
On 2015/07/16 12:53:47, rossberg wrote:
> On 2015/07/15 20:35:44, caitp wrote:
> > this move kind of makes destructuring initializers not very good, but maybe
> this
> > is actually intentional? Not sure.
> 
> Not sure I understand. What's not good?
> 


It's okay for the VM, but having to fill out each field is sort of inconvenient
and hurts one of the conveniences of using destructuring in the first place.

eg, `function f({ x = true }) { "use strong"; return x; } f({})` would throw
because `x` does not exist in `{}` --- so it makes destructuring initializers
sort of pointless, because you always need to be explicit.

> Btw, language mode vs parameter destructuring is a much bigger problem than I
> thought yesterday. Will post something later.

Yeah, this doesn't quite cut it although it does keep existing tests passing at
least.

[rossberg, 2015-07-16 13:21:05]

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4383
src/parser.cc:4383: if (fvar != nullptr) {
On 2015/07/16 13:03:57, caitp wrote:
> On 2015/07/16 12:53:47, rossberg wrote:
> > Hm, why do you need to move (and duplicate) fvar initialisation and
> IsConstruct
> > assert?
> 
> it seems that these need to happen before the init block. I suppose it could
> just use `result->insertAt(0)` instead though

Not sure I understand. They were happening before the init block before, AFAICT?
Why did that need to change at all?

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
File test/mjsunit/strong/destructuring.js (right):

https://codereview.chromium.org/1234213004/diff/1/test/mjsunit/strong/destruc...
test/mjsunit/strong/destructuring.js:10: var a = f({ x: undefined });
On 2015/07/16 13:03:57, caitp wrote:
> On 2015/07/16 12:53:47, rossberg wrote:
> > On 2015/07/15 20:35:44, caitp wrote:
> > > this move kind of makes destructuring initializers not very good, but
maybe
> > this
> > > is actually intentional? Not sure.
> > 
> > Not sure I understand. What's not good?
> 
> It's okay for the VM, but having to fill out each field is sort of
inconvenient
> and hurts one of the conveniences of using destructuring in the first place.
> 
> eg, `function f({ x = true }) { "use strong"; return x; } f({})` would throw
> because `x` does not exist in `{}` --- so it makes destructuring initializers
> sort of pointless, because you always need to be explicit.

Ah, I see. Yes, that shouldn't throw in strong mode either, defaults are
supposed to continue to work for absent properties. But that needs some
adaptation of the desugaring, which we haven't done yet.

[caitp (gmail), 2015-07-16 14:44:32]

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1234213004/diff/1/src/parser.cc#newcode4383
src/parser.cc:4383: if (fvar != nullptr) {
On 2015/07/16 13:21:05, rossberg wrote:
> On 2015/07/16 13:03:57, caitp wrote:
> > On 2015/07/16 12:53:47, rossberg wrote:
> > > Hm, why do you need to move (and duplicate) fvar initialisation and
> > IsConstruct
> > > assert?
> > 
> > it seems that these need to happen before the init block. I suppose it could
> > just use `result->insertAt(0)` instead though
> 
> Not sure I understand. They were happening before the init block before,
AFAICT?
> Why did that need to change at all?

I've refactored this, it's much cleaner now

[rossberg, 2015-07-16 15:06:19]

lgtm

https://codereview.chromium.org/1234213004/diff/40001/test/mjsunit/strong/des...
File test/mjsunit/strong/destructuring.js (right):

https://codereview.chromium.org/1234213004/diff/40001/test/mjsunit/strong/des...
test/mjsunit/strong/destructuring.js:13: // TODO(???): Loading non-existent
properties during destructuring should not
You can put my name there

[caitp (gmail), 2015-07-16 15:11:13]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-07-16 15:11:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1234213004/#ps60001
(title: "update name in TODO")

[commit-bot: I haz the power, 2015-07-16 15:11:23]

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

[commit-bot: I haz the power, 2015-07-16 15:21:54]

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

[commit-bot: I haz the power, 2015-07-16 15:21:54]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/7734)
  v8_win64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/6756)

(exceeded global retry quota)

[caitp (gmail), 2015-07-16 15:26:13]

On 2015/07/16 15:21:54, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/7734)
>   v8_win64_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/6756)
> 
> (exceeded global retry quota)

hmm, `make check` passes here, will take another look..

[caitp (gmail), 2015-07-16 15:57:12]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-07-16 15:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1234213004/#ps80001
(title: "Fix arrow-function failures")

[commit-bot: I haz the power, 2015-07-16 15:57:25]

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

[commit-bot: I haz the power, 2015-07-16 16:45:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-16 16:45:16]

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