new classes: implement new.target passing to superclass constructor.

new classes: implement new.target passing to superclass constructor.

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

Committed: https://crrev.com/8aed43e82c6d2742fe5988603cb8841324cc942b
Cr-Commit-Position: refs/heads/master@{#26560}

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

[Dmitry Lomov (no reviews), 2015-02-09 16:31:44]

PTAL before platform ports.

The idea is: subclass constructors (the ones that have 'extends') have an extra
argument (which is new.target). This is reflected in the count of formal
arguments as reported by SharedFunctionInfo, so argument adaptor frames etc. are
set correctly.

The only way those functions can be gainfully invoked is via their 'construct'
stub, and their special construct stub sets up arguemnts correctly. If you call
them via [[Call]] the arguments will be bound all wrong, but it does not matter
since [[Call]] throws.

Ideas for more tests or what can go wrong welcome

[arv (Not doing code reviews), 2015-02-09 17:24:07]

On 2015/02/09 16:31:44, Dmitry Lomov (chromium) wrote:
> PTAL before platform ports.
> 
> The idea is: subclass constructors (the ones that have 'extends') have an
extra
> argument (which is new.target). This is reflected in the count of formal
> arguments as reported by SharedFunctionInfo, so argument adaptor frames etc.
are
> set correctly.
> 
> The only way those functions can be gainfully invoked is via their 'construct'
> stub, and their special construct stub sets up arguemnts correctly. If you
call
> them via [[Call]] the arguments will be bound all wrong, but it does not
matter
> since [[Call]] throws.
> 
> Ideas for more tests or what can go wrong welcome

That is a good idea.

Things that might go wrong:

- Usage of arguments
  - Special cases of arguments in apply etc
- Function instance length property
- Rest & optional parameters once implemented
- Bound function [[Construct]]

[arv (Not doing code reviews), 2015-02-09 17:44:39]

I like this approach. But...

How is this going to work? new.target needs to work outside classes. That is the
mechanism we are using to allow determining if a function was [[Call]]ed vs
[[Construct]]ed.

roughly:

function Date() {
  if (new.target === undefined) {
    return '2015-02-08';
  }
  return {[[TimeValue]]: 423443346, __proto__: new.target.prototype};
}

Maybe if we can keep track of `new.target` we can use the same idea for ordinary
functions when they reference `new.target`?

https://codereview.chromium.org/908883002/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/908883002/diff/20001/src/compiler.cc#newcode623
src/compiler.cc:623:
function_info->set_formal_parameter_count(lit->parameter_count() + 1);
I'm worried about doing it at this high abstraction level. NewTarget is not a
formal parameter. Maybe it is better do it at a lower level abstraction in case
other code wants to use the formal parameter count for what it sounds like
(function length, rest params etc).

The SFI already has a field for the FunctionKind so deferring this choice seems
safer.

https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:1304: if (*original_function != *function) {
Is this is to set up the prototype from the NewTarget. Maybe add a comment?

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

https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:161: class ExtendedUint8Array
extends Uint8Array {
HOT!

https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:174: assertTrue(eua.__proto__ ===
ExtendedUint8Array.prototype);
Maybe a test with Object.prototype.toString to make sure the instance is the
right exotic object kind.

[Dmitry Lomov (no reviews), 2015-02-09 18:28:23]

Response to comments:

On 2015/02/09 17:44:39, arv wrote:
> I like this approach. But...
> 
> How is this going to work? new.target needs to work outside classes. That is
the
> mechanism we are using to allow determining if a function was [[Call]]ed vs
> [[Construct]]ed.

Sure. We cannot add new.target parameter to each and every function though -
that is expensive.
The rough sketch for new.target is as follows:
- we will determine, for every function, whether the new.target is required.
- functions that use new.target will:
  - use yet another construct stub that will pass new.target to those functions 
  - on entry to those functions they will determine whether they were
[[Construct]]ed or [[Call]]ed and populate their new.target in the right way.
     No need to do that for class constructors since they are always constructed
(and need a special construct stub anyway)

> 
> roughly:
> 
> function Date() {
>   if (new.target === undefined) {
>     return '2015-02-08';
>   }
>   return {[[TimeValue]]: 423443346, __proto__: new.target.prototype};
> }
> 
> Maybe if we can keep track of `new.target` we can use the same idea for
ordinary
> functions when they reference `new.target`?
> 
> https://codereview.chromium.org/908883002/diff/20001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/908883002/diff/20001/src/compiler.cc#newcode623
> src/compiler.cc:623:
> function_info->set_formal_parameter_count(lit->parameter_count() + 1);
> I'm worried about doing it at this high abstraction level. NewTarget is not a
> formal parameter. Maybe it is better do it at a lower level abstraction in
case
> other code wants to use the formal parameter count for what it sounds like
> (function length, rest params etc).
> 
> The SFI already has a field for the FunctionKind so deferring this choice
seems
> safer.

I thought of it, but this adds complexity to generated code with no good reason.
For example, ArgumentsAdaptor frame will need to switch on FunctionKind, but
that does not save us anything since say the code that sets up ArgumentsAccess
will need to do the same as well. And so on and so forth.
SFI's formal parameter count seems just right for this purpose - the number of
parameters that function's frame has on the stack.
If you want to get a 'true' parameter count, there is always
scope()->num_parameters()
Note that function's length is separate (there is a separate field in SFI just
for that)

> 
>
https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
> File src/runtime/runtime-object.cc (right):
> 
>
https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
> src/runtime/runtime-object.cc:1304: if (*original_function != *function) {
> Is this is to set up the prototype from the NewTarget. Maybe add a comment?
> 
>
https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
> File test/mjsunit/harmony/classes-experimental.js (right):
> 
>
https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
> test/mjsunit/harmony/classes-experimental.js:161: class ExtendedUint8Array
> extends Uint8Array {
> HOT!
> 
>
https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
> test/mjsunit/harmony/classes-experimental.js:174: assertTrue(eua.__proto__ ===
> ExtendedUint8Array.prototype);
> Maybe a test with Object.prototype.toString to make sure the instance is the
> right exotic object kind.

[Dmitry Lomov (no reviews), 2015-02-10 19:22:15]

PTAL, all platforms done and comments addressed

https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/908883002/diff/20001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:1304: if (*original_function != *function) {
On 2015/02/09 17:44:39, arv wrote:
> Is this is to set up the prototype from the NewTarget. Maybe add a comment?

Done.

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

https://codereview.chromium.org/908883002/diff/20001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes-experimental.js:174: assertTrue(eua.__proto__ ===
ExtendedUint8Array.prototype);
On 2015/02/09 17:44:40, arv wrote:
> Maybe a test with Object.prototype.toString to make sure the instance is the
> right exotic object kind.

Done.

[arv (Not doing code reviews), 2015-02-10 20:15:19]

LGTM

Mostly just js style nits but also some ideas to improve the tests.

https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:266: IsSubclassConstructor(info->function()->kind())
Do we do the right thing in the following case?

class C {
  constructor() {
    assertEquals(C, new.target);
  }
}
new C();

The function above is not a subclass constructor.

Maybe this is just a TODO?


Maybe add a function to SHF called, HasNewTarget()?

https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc#newcode326
src/scopes.cc:326: variables_.Declare(this,
ast_value_factory_->new_target_string(), VAR,
Shouldn't this be CONST?

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:32: constructor(x,y) {
ws after commma

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:89: super(tmp(),4);
Because left to right execution?

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:90: } catch(e) { exn = e; }
ws after catch

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:91: assertTrue(exn !== null);
instanceof test would be better

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:127: assertTrue(s.__proto__ ===
Subclass.prototype);
assertSame here too

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:145: let s_2 = new Subclass2();
no underscores in js

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:151: super(x+y);
ws around binops

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:185: return new Object();
in general, prefer {} over new Object()

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:205: let f = Subclass.bind(new
Object());
maybe

let obj = {};
let f = Subclass.bind(obj);

and assert that `this !== obj` in the constructor.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:205: let f = Subclass.bind(new
Object());
Bind a parameter too.

let f = Subclass.bind({}, 1);

[Dmitry Lomov (no reviews), 2015-02-11 00:58:10]

New patchsets have been uploaded after l-g-t-m from arv@chromium.org

[Dmitry Lomov (no reviews), 2015-02-11 00:58:30]

All done, landing

https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:266: IsSubclassConstructor(info->function()->kind())
On 2015/02/10 20:15:18, arv wrote:
> Do we do the right thing in the following case?
> 
> class C {
>   constructor() {
>     assertEquals(C, new.target);
>   }
> }
> new C();
> 
> The function above is not a subclass constructor.
> 
> Maybe this is just a TODO?

Yes, we do not support new.target in general yet, just passing new.target
through subclass constructor.
> 
> 
> Maybe add a function to SHF called, HasNewTarget()?

I will add that when we support new.target proper

https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc#newcode326
src/scopes.cc:326: variables_.Declare(this,
ast_value_factory_->new_target_string(), VAR,
On 2015/02/10 20:15:18, arv wrote:
> Shouldn't this be CONST?

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
File test/mjsunit/harmony/classes-experimental.js (right):

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:89: super(tmp(),4);
On 2015/02/10 20:15:18, arv wrote:
> Because left to right execution?

I realized this is not correct under the new spec. super call needs to happen
and then we validate that 'this' has not been initialized yet. I'll fix in
sparate CL (this CL does not change that behaviour)

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:90: } catch(e) { exn = e; }
On 2015/02/10 20:15:18, arv wrote:
> ws after catch

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:91: assertTrue(exn !== null);
On 2015/02/10 20:15:18, arv wrote:
> instanceof test would be better

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:127: assertTrue(s.__proto__ ===
Subclass.prototype);
On 2015/02/10 20:15:18, arv wrote:
> assertSame here too

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:145: let s_2 = new Subclass2();
On 2015/02/10 20:15:18, arv wrote:
> no underscores in js

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:151: super(x+y);
On 2015/02/10 20:15:18, arv wrote:
> ws around binops

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:185: return new Object();
On 2015/02/10 20:15:18, arv wrote:
> in general, prefer {} over new Object()

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:205: let f = Subclass.bind(new
Object());
On 2015/02/10 20:15:18, arv wrote:
> maybe
> 
> let obj = {};
> let f = Subclass.bind(obj);
> 
> and assert that `this !== obj` in the constructor.

Done.

https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes-experimental.js:205: let f = Subclass.bind(new
Object());
On 2015/02/10 20:15:18, arv wrote:
> Bind a parameter too.
> 
> let f = Subclass.bind({}, 1);

Done.

[Dmitry Lomov (no reviews), 2015-02-11 00:58:36]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-02-11 00:59:12]

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

[commit-bot: I haz the power, 2015-02-11 01:23:11]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-02-11 01:23:23]

Patchset 9 (id:??) landed as
https://crrev.com/8aed43e82c6d2742fe5988603cb8841324cc942b
Cr-Commit-Position: refs/heads/master@{#26560}

[Dmitry Lomov (no reviews), 2015-02-11 01:35:39]

A revert of this CL (patchset #9 id:150001) has been created in
https://codereview.chromium.org/911363002/ by dslomov@chromium.org.

The reason for reverting is: Breaks Linux64 release.

[Dmitry Lomov (no reviews), 2015-02-11 09:24:55]

x64 issue fixed + rebased; landing

[Dmitry Lomov (no reviews), 2015-02-11 09:25:18]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-02-11 09:25:51]

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

[commit-bot: I haz the power, 2015-02-11 09:47:38]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-02-11 09:47:57]

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