new classes: special construct stub for derived classs and TDZ for `this`.

new classes: special construct stub for derived classs and TDZ for `this`.

R=arv@chromium.org,rossberg@chromium.org
BUG=v8:3834
LOG=N

Committed: https://crrev.com/6f97a4948f1495a9b9d60b7f495d0e0efeaa1006
Cr-Commit-Position: refs/heads/master@{#26409}

[Dmitry Lomov (no reviews), 2015-01-23 11:53:28]

PTAL. No platform ports yet, but welcome early opinion

[Dmitry Lomov (no reviews), 2015-01-23 15:37:29]

Platform ports done, ready for review.

[arv (Not doing code reviews), 2015-01-23 15:40:46]

https://codereview.chromium.org/867153003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/867153003/diff/1/src/objects.h#newcode7192
src/objects.h:7192: kIsSublclassConstructor,
typo

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

https://codereview.chromium.org/867153003/diff/1/src/parser.cc#newcode274
src/parser.cc:274: (FLAG_experimental_classes && IsSubclassConstructor(kind)) ?
true : false;
bool uninitialized_this = FLAG_experimental_classes &&
IsSubclassConstructor(kind);

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:112: Handle<Code>
stub(isolate->builtins()->JSConstructStubForDerived());
Let me make sure I understand this correctly. We replace the normal construct
stub with one that understands NewTarget.

This makes me think I should use stubs for the default constructors too.

https://codereview.chromium.org/867153003/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/867153003/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:3111: script_scope->Initialize(false);
maybe even use a default param here since this false value gives no hint.

[Dmitry Lomov (no reviews), 2015-01-23 21:08:28]

Comments addressed.

https://codereview.chromium.org/867153003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/867153003/diff/1/src/objects.h#newcode7192
src/objects.h:7192: kIsSublclassConstructor,
On 2015/01/23 15:40:46, arv wrote:
> typo

Done.

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

https://codereview.chromium.org/867153003/diff/1/src/parser.cc#newcode274
src/parser.cc:274: (FLAG_experimental_classes && IsSubclassConstructor(kind)) ?
true : false;
On 2015/01/23 15:40:46, arv wrote:
> bool uninitialized_this = FLAG_experimental_classes &&
> IsSubclassConstructor(kind);

Done (classic!)

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:112: Handle<Code>
stub(isolate->builtins()->JSConstructStubForDerived());
On 2015/01/23 15:40:46, arv wrote:
> Let me make sure I understand this correctly. We replace the normal construct
> stub with one that understands NewTarget.

Correct, although none of stubs yet understand new.target.
They both will eventually (soon).
JSConstructStubForDerived is the one that does not allocate and passes a hole as
receiver.

> 
> This makes me think I should use stubs for the default constructors too.

Correct.
Yes, this patch does not cover default constructors. If you want I can handle
stubs for them here (although I'd prefer a separate patch)

https://codereview.chromium.org/867153003/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/867153003/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:3111: script_scope->Initialize(false);
On 2015/01/23 15:40:46, arv wrote:
> maybe even use a default param here since this false value gives no hint.

Done.

[arv (Not doing code reviews), 2015-01-23 21:37:00]

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/867153003/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:112: Handle<Code>
stub(isolate->builtins()->JSConstructStubForDerived());
On 2015/01/23 21:08:28, Dmitry Lomov (chromium) wrote:
> On 2015/01/23 15:40:46, arv wrote:
> > Let me make sure I understand this correctly. We replace the normal
construct
> > stub with one that understands NewTarget.
> 
> Correct, although none of stubs yet understand new.target.
> They both will eventually (soon).
> JSConstructStubForDerived is the one that does not allocate and passes a hole
as
> receiver.
> 
> > 
> > This makes me think I should use stubs for the default constructors too.
> 
> Correct.
> Yes, this patch does not cover default constructors. If you want I can handle
> stubs for them here (although I'd prefer a separate patch)
> 

Lets handle it here in a different patch. I found some other work until this one
lands.

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

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode2132
src/preparser.h:2132: kind = has_extends ? FunctionKind::kSubclassConstructor
Sad. This needs to be dynamic since an extends value of null needs to be treated
as if no extends clause was present :'(

[Dmitry Lomov (no reviews), 2015-01-24 02:45:14]

I am not sure if you object to something in the patch or not. 
If not, then why is it not looking good? Or maybe I am not clear enough in my
design?

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

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode2132
src/preparser.h:2132: kind = has_extends ? FunctionKind::kSubclassConstructor
On 2015/01/23 21:37:00, arv wrote:
> Sad. This needs to be dynamic since an extends value of null needs to be
treated
> as if no extends clause was present :'(

That is fine. FunctionKind::kSubclassConstructor means in fact "maybe subclass
constructor" (conditioned on whether the actual extends value is null or not).

The key thing to achieve here is that we separate out the case where 'extends'
is syntactically not present - in that case the receiver is definitely never a
hole, we definitely need no checks on 'this', we definitely cannot use 'super'
etc.

[arv (Not doing code reviews), 2015-01-24 16:19:09]

LGTM with nits

It would also be good if someone more experience with the codegen could take a
quick look at that part.

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

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode515
src/preparser.h:515: bool has_extends,
I would prefer has_extends before is_static since that is more similar to the
code order.

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode2132
src/preparser.h:2132: kind = has_extends ? FunctionKind::kSubclassConstructor
On 2015/01/24 02:45:14, Dmitry Lomov (chromium) wrote:
> On 2015/01/23 21:37:00, arv wrote:
> > Sad. This needs to be dynamic since an extends value of null needs to be
> treated
> > as if no extends clause was present :'(
> 
> That is fine. FunctionKind::kSubclassConstructor means in fact "maybe subclass
> constructor" (conditioned on whether the actual extends value is null or not).
> 
> The key thing to achieve here is that we separate out the case where 'extends'
> is syntactically not present - in that case the receiver is definitely never a
> hole, we definitely need no checks on 'this', we definitely cannot use 'super'
> etc. 

Resolved.

has_extends is just an optimization then? If it is false, we know that the we
should not use the derived constructor. If it is true we might still need to use
the derived constructor.

https://codereview.chromium.org/867153003/diff/40001/test/mjsunit/harmony/cla...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/867153003/diff/40001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:30: return this;
Can you add a few more test cases?

1. A constructor without `return this`.
2. A constructor with a second call to `super()` which should throw.

[Dmitry Lomov (no reviews), 2015-01-26 23:33:04]

Patchset #4 (id:60001) has been deleted

[Dmitry Lomov (no reviews), 2015-01-26 23:34:04]

Comments addressed

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

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode515
src/preparser.h:515: bool has_extends,
On 2015/01/24 16:19:09, arv wrote:
> I would prefer has_extends before is_static since that is more similar to the
> code order.

Done.

https://codereview.chromium.org/867153003/diff/40001/src/preparser.h#newcode2132
src/preparser.h:2132: kind = has_extends ? FunctionKind::kSubclassConstructor
On 2015/01/24 16:19:09, arv wrote:
> On 2015/01/24 02:45:14, Dmitry Lomov (chromium) wrote:
> > On 2015/01/23 21:37:00, arv wrote:
> > > Sad. This needs to be dynamic since an extends value of null needs to be
> > treated
> > > as if no extends clause was present :'(
> > 
> > That is fine. FunctionKind::kSubclassConstructor means in fact "maybe
subclass
> > constructor" (conditioned on whether the actual extends value is null or
not).
> > 
> > The key thing to achieve here is that we separate out the case where
'extends'
> > is syntactically not present - in that case the receiver is definitely never
a
> > hole, we definitely need no checks on 'this', we definitely cannot use
'super'
> > etc. 
> 
> Resolved.
> 
> has_extends is just an optimization then? If it is false, we know that the we
> should not use the derived constructor. If it is true we might still need to
use
> the derived constructor.

Yes, correct. Most importantly, normal functions are not derived constructors,
so there is no extra handling for holey `this`.

https://codereview.chromium.org/867153003/diff/40001/test/mjsunit/harmony/cla...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/867153003/diff/40001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:30: return this;
On 2015/01/24 16:19:09, arv wrote:
> Can you add a few more test cases?
> 
> 1. A constructor without `return this`.
This is not implemented yet - coming next

> 2. A constructor with a second call to `super()` which should throw.
Done, actually required code changes.

[arv (Not doing code reviews), 2015-01-27 21:08:28]

LGTM

https://codereview.chromium.org/867153003/diff/80001/test/mjsunit/harmony/cla...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/867153003/diff/80001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:48: var exn = null;
any reason why you are using var here and let above?

[rossberg, 2015-02-02 16:32:38]

https://codereview.chromium.org/867153003/diff/80001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/867153003/diff/80001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1463: } else if (var->is_this()) {
Does this now affect every use of 'this'? Can we limit the check to actual
constructors?

https://codereview.chromium.org/867153003/diff/80001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/867153003/diff/80001/src/preparser.cc#newcode982
src/preparser.cc:982: bool has_extends = false;
Nit:

  bool has_extends = Check(Token::EXTENDS);
  if (has_extends) ParseLeftHandSideExpression(CHECK_OK);

[Dmitry Lomov (no reviews), 2015-02-03 15:47:04]

PTAL

https://codereview.chromium.org/867153003/diff/80001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/867153003/diff/80001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1463: } else if (var->is_this()) {
On 2015/02/02 16:32:37, rossberg wrote:
> Does this now affect every use of 'this'? Can we limit the check to actual
> constructors?

No, this only happens if var->binding_needs_init() and `this` does not need
intitalization outside of subclass constructors. Added an assert.

https://codereview.chromium.org/867153003/diff/80001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/867153003/diff/80001/src/preparser.cc#newcode982
src/preparser.cc:982: bool has_extends = false;
On 2015/02/02 16:32:38, rossberg wrote:
> Nit:
> 
>   bool has_extends = Check(Token::EXTENDS);
>   if (has_extends) ParseLeftHandSideExpression(CHECK_OK);

Done.

https://codereview.chromium.org/867153003/diff/80001/test/mjsunit/harmony/cla...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/867153003/diff/80001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:48: var exn = null;
On 2015/01/27 21:08:27, arv wrote:
> any reason why you are using var here and let above?

Done.

[rossberg, 2015-02-03 15:52:42]

https://codereview.chromium.org/867153003/diff/100001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/867153003/diff/100001/src/preparser.cc#newcod...
src/preparser.cc:1008: if (has_extends) ParseLeftHandSideExpression(CHECK_OK);
Actually, I just realised that you need to put braces around this, or the
CHECK_OK macro will screw you over. 8-}

[Dmitry Lomov (no reviews), 2015-02-03 15:59:14]

PTAL. please click CQ as well if it looks good.

https://codereview.chromium.org/867153003/diff/100001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/867153003/diff/100001/src/preparser.cc#newcod...
src/preparser.cc:1008: if (has_extends) ParseLeftHandSideExpression(CHECK_OK);
On 2015/02/03 15:52:42, rossberg wrote:
> Actually, I just realised that you need to put braces around this, or the
> CHECK_OK macro will screw you over. 8-}

Good catch, done.

[rossberg, 2015-02-03 16:03:27]

lgtm

[rossberg, 2015-02-03 16:03:32]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-02-03 16:03:57]

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

[commit-bot: I haz the power, 2015-02-03 17:42:54]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-02-03 17:43:15]

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