[strong] checking of this & super in constructors

[strong] checking of this & super in constructors

R=dslomov@chromium.org, marja@chromium.org
BUG=v8:3956
LOG=N

Enforces for constructors that
- the only use of 'super' is the super constructor call
- the only use of 'this' is a property assignment
- both of these must happen at the top-level of the body
- 'this' may only be assigned after the 'super' call
- 'return' may only be used after the last assignment to 'this'

Not yet working for arrow functions (there might be deeper bugs with those).

Committed: https://crrev.com/580d66bcda66220d2f3062ac58daf925436df74c
Cr-Commit-Position: refs/heads/master@{#27977}

[marja, 2015-03-23 10:20:07]

Could you also add a description to describe exactly what is allowed now; it's
sort of tedious to infer as the negation of the union of the added errors :)

(I'm also slightly skeptical that there are no tests that assert that some stuff
*is* allowed; because without such a test this CL can overshoot, i.e., it could
theoretically produce errors always when it sees super or this, and we wouldn't
notice.)

https://codereview.chromium.org/1024063002/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1024063002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5591: "class C extends Object { constructor() {
(this); } }",
I'm not sure if this test is testing what it looks like at the first glance.. Is
parsing failing for all of these because there's no super call? So
strong_super_call_misplaced? Or is it failing because of
strong_constructor_this, or is it mixed (the this.a = 0 failing because of the
former, at least)?

For the "malformed this expression" case the interesting tests would be like:

super()
if (1) this.a = 0;

etc., right?

I think it would be worthwhile to split this test into 2 cases, where the first
set is failing because there's no super call (even though the this assignment is
valid) -- you can put the super call into context_data, so, test that it's
succeeding w/ context_data_with_super_call, but failing w/
context_data_without_super_call. The second set would then be failing with both
contexts, because the assignment is malformed.

[rossberg, 2015-04-14 14:04:11]

rossberg@chromium.org changed reviewers:
+ conradw@chromium.org

[rossberg, 2015-04-20 17:21:11]

On 2015/03/23 10:20:07, marja wrote:
> Could you also add a description to describe exactly what is allowed now; it's
> sort of tedious to infer as the negation of the union of the added errors :)

Done.

> (I'm also slightly skeptical that there are no tests that assert that some
stuff
> *is* allowed; because without such a test this CL can overshoot, i.e., it
could
> theoretically produce errors always when it sees super or this, and we
wouldn't
> notice.)

Added positive tests. That actually uncovered an inconsistency in the treatment
of skipping lazy functions, where you would only get the super_missing error
when preparse data wasn't cached. :) Now the check is performed in the lazy
parser and not by the caller.

https://codereview.chromium.org/1024063002/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1024063002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5591: "class C extends Object { constructor() {
(this); } }",
On 2015/03/23 10:20:07, marja wrote:
> I'm not sure if this test is testing what it looks like at the first glance..
Is
> parsing failing for all of these because there's no super call? So
> strong_super_call_misplaced? Or is it failing because of
> strong_constructor_this, or is it mixed (the this.a = 0 failing because of the
> former, at least)?
> 
> For the "malformed this expression" case the interesting tests would be like:
> 
> super()
> if (1) this.a = 0;
> 
> etc., right?
> 
> I think it would be worthwhile to split this test into 2 cases, where the
first
> set is failing because there's no super call (even though the this assignment
is
> valid) -- you can put the super call into context_data, so, test that it's
> succeeding w/ context_data_with_super_call, but failing w/
> context_data_without_super_call. The second set would then be failing with
both
> contexts, because the assignment is malformed.

Oops, yes, this test wasn't supposed to test missing super calls. That's already
tested separately below.

[marja, 2015-04-21 08:27:27]

Hmm, did you forget to upload a patch set?

[rossberg, 2015-04-21 08:44:26]

On 2015/04/21 08:27:27, marja wrote:
> Hmm, did you forget to upload a patch set?

Argh, indeed. Done.

[marja, 2015-04-21 09:01:37]

In the description, did you mean s/either/both/ ?

The label stuff is unclear to me.

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

https://codereview.chromium.org/1024063002/diff/40001/src/preparser.h#newcode...
src/preparser.h:2928: // TODO(aperez): Does this actually work with lazily
compiled arrows?
-> wingo, since he took over?

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5600: NULL};
These cases would be relevant to add:

this.a = this.b;

this.a = {b : 1}; this.a.b = 2;

this.a = function() {}; this.a();

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5647: "class C extends Object { constructor() {
super(); super(); } }",
Nit: these 2 lines are the same.

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5652: "class C extends Object { constructor() {
super(this.a = 0); } }",
How about

super().foo; ?

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5657: "class C extends Object { constructor() { l:
66; super(); } }",
Hmm, why is label: super(); forbidden but label: 66; super(); is not?

[rossberg, 2015-04-21 09:47:47]

> In the description, did you mean s/either/both/ ?

Done.

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

https://codereview.chromium.org/1024063002/diff/40001/src/preparser.h#newcode...
src/preparser.h:2928: // TODO(aperez): Does this actually work with lazily
compiled arrows?
On 2015/04/21 09:01:37, marja wrote:
> -> wingo, since he took over?

Done.

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5600: NULL};
On 2015/04/21 09:01:37, marja wrote:
> These cases would be relevant to add:
> 
> this.a = this.b;
> 
> this.a = {b : 1}; this.a.b = 2;
> 
> this.a = function() {}; this.a();

Done.

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5647: "class C extends Object { constructor() {
super(); super(); } }",
On 2015/04/21 09:01:37, marja wrote:
> Nit: these 2 lines are the same.

No, "," vs ";" ;)

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5652: "class C extends Object { constructor() {
super(this.a = 0); } }",
On 2015/04/21 09:01:37, marja wrote:
> How about
> 
> super().foo; ?

Done.

https://codereview.chromium.org/1024063002/diff/40001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5657: "class C extends Object { constructor() { l:
66; super(); } }",
On 2015/04/21 09:01:37, marja wrote:
> Hmm, why is label: super(); forbidden but label: 66; super(); is not?

Because the former nests the super call into a labelled statement, the latter
does not.

[marja, 2015-04-21 10:00:17]

Cleared label confusion (*) offline; lgtm.

So the label really binds only one statement, so it matters whether super is the
immediately following statement or not. And other rules already forbid things
like a switch statement where a label would affect multiple statements.

[rossberg, 2015-04-21 14:16:04]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-04-21 14:16:04]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/1024063002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2015-04-21 14:16:15]

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

[commit-bot: I haz the power, 2015-04-21 14:44:10]

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

[commit-bot: I haz the power, 2015-04-21 14:44:10]

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

[rossberg, 2015-04-21 14:58:26]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-04-21 14:58:26]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/1024063002/#ps90001
(title: "Fixed func name inferrer")

[commit-bot: I haz the power, 2015-04-21 14:58:40]

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

[commit-bot: I haz the power, 2015-04-21 15:01:16]

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

[commit-bot: I haz the power, 2015-04-21 15:01:17]

Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/4130)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[rossberg, 2015-04-21 16:12:03]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-04-21 16:12:04]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/1024063002/#ps110001
(title: "Rebase again")

[commit-bot: I haz the power, 2015-04-21 16:12:17]

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

[commit-bot: I haz the power, 2015-04-21 16:34:49]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-04-22 04:10:32]

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

[Michael Achenbach, 2015-04-22 08:00:19]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/1105453002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks mac gc stress:
http://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/....