Implement Function.prototype.toMethod.

Implement Function.prototype.toMethod.

R=arv@chromium.org, verwaest@chromium.org
BUG=v8:3330
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=23274

[Dmitry Lomov (no reviews), 2014-08-15 21:58:21]

PTAL.

The spec for toMethod requires the result to be extensible, that will be a
separate CL.

[arv (Not doing code reviews), 2014-08-15 22:28:48]

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

https://codereview.chromium.org/475423003/diff/1/src/runtime.cc#newcode2070
src/runtime.cc:2070: return isolate->heap()->home_object_symbol();
Why do you need this?

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js
File src/harmony-classes.js (right):

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:15: [%ToString(this), typeof this]);
Maybe TO_STRING_INLINE?

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:19: if (!IS_SPEC_OBJECT(homeObject)) {
This is wrong order. This test should come before the is function test.

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:20: throw MakeTypeError('toMethod_non_object');
Maybe include homeObject in the error message?

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:29: "toMethod", FunctionToMethod
wrong indentation

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:31: (function () {
Please name these. Makes it easier to know what you are testing

(function TestSomething() {
  ...
})();

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:64: 
two new lines between top level functions

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:80: assertEquals(o,
fMeth[%HomeObjectSymbol()]);
Maybe define the private symbol in harmony-classes.js. That way there is no need
for the runtime function.

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:104: }());
Can you add a test that we check the type of the newHome in toMethod before we
test the type of this.

[Dmitry Lomov (no reviews), 2014-08-15 22:56:57]

Thanks for review, addressed comments

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js
File src/harmony-classes.js (right):

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:15: [%ToString(this), typeof this]);
On 2014/08/15 22:28:47, arv wrote:
> Maybe TO_STRING_INLINE?
This is an error case, fine without inlining.

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:19: if (!IS_SPEC_OBJECT(homeObject)) {
On 2014/08/15 22:28:47, arv wrote:
> This is wrong order. This test should come before the is function test.
Good catch. This must be a spec bug. Will file.

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:20: throw MakeTypeError('toMethod_non_object');
On 2014/08/15 22:28:47, arv wrote:
> Maybe include homeObject in the error message?

Done.

https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:29: "toMethod", FunctionToMethod
On 2014/08/15 22:28:47, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:31: (function () {
On 2014/08/15 22:28:48, arv wrote:
> Please name these. Makes it easier to know what you are testing
> 
> (function TestSomething() {
>   ...
> })();

Done.

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:64: 
On 2014/08/15 22:28:48, arv wrote:
> two new lines between top level functions

Done.

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:80: assertEquals(o,
fMeth[%HomeObjectSymbol()]);
On 2014/08/15 22:28:48, arv wrote:
> Maybe define the private symbol in harmony-classes.js. That way there is no
need
> for the runtime function.

I am not sure what the suggestion is.
a) we need HomeObjectSymbol outside of harmony-classes.js for codegen
b) without runtime function, how would we test this here?

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:104: }());
On 2014/08/15 22:28:48, arv wrote:
> Can you add a test that we check the type of the newHome in toMethod before we
> test the type of this.
I believe this is a spec bug.
Anyway, frankly not sure what is the good way to test the order of checks -
ideas?

[arv (Not doing code reviews), 2014-08-15 23:02:41]

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:80: assertEquals(o,
fMeth[%HomeObjectSymbol()]);
On 2014/08/15 22:56:57, Dmitry Lomov (chromium) wrote:
> On 2014/08/15 22:28:48, arv wrote:
> > Maybe define the private symbol in harmony-classes.js. That way there is no
> need
> > for the runtime function.
> 
> I am not sure what the suggestion is.
> a) we need HomeObjectSymbol outside of harmony-classes.js for codegen
> b) without runtime function, how would we test this here?

The way we have done this before is that we define the symbol in the js file and
then we use INSTALL_NATIVE in bootstrapper

See symbolIterator for example.

[Dmitry Lomov (no reviews), 2014-08-15 23:07:45]

On 2014/08/15 23:02:41, arv wrote:
>
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
> File test/mjsunit/harmony/toMethod.js (right):
> 
>
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toM...
> test/mjsunit/harmony/toMethod.js:80: assertEquals(o,
> fMeth[%HomeObjectSymbol()]);
> On 2014/08/15 22:56:57, Dmitry Lomov (chromium) wrote:
> > On 2014/08/15 22:28:48, arv wrote:
> > > Maybe define the private symbol in harmony-classes.js. That way there is
no
> > need
> > > for the runtime function.
> > 
> > I am not sure what the suggestion is.
> > a) we need HomeObjectSymbol outside of harmony-classes.js for codegen
> > b) without runtime function, how would we test this here?
> 
> The way we have done this before is that we define the symbol in the js file
and
> then we use INSTALL_NATIVE in bootstrapper
> 
> See symbolIterator for example.

Oh no, symbolIterator only works because it is user-accessible, and
HomeObjectSymbol is not. 
Without the runtime function, there is no way (and there should not be) for the
test to access it.

[Toon Verwaest, 2014-08-18 14:15:35]

https://codereview.chromium.org/475423003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/475423003/diff/60001/src/heap/heap.cc#newcode...
src/heap/heap.cc:3758: if (source->IsJSFunction()) {
Are you sure this works? What code object does this get? What if it's optimized?
If we also become optimized, will we get notified in case the function
deoptimizes?

Can you just make a specialized JSFunction::Copy(), that preferably uses
NewFunctionFromSharedFunctionInfo, but additionally copies over properties and
prototype?

https://codereview.chromium.org/475423003/diff/60001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/60001/src/runtime.cc#newcode2063
src/runtime.cc:2063: Object::SetProperty(clone, home_object_symbol, home_object,
STRICT));
Perhaps use JSObject::SetOwnPropertyIgnoreAttributes?

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:97: function f(x, y, z) {
Add flag "called = false" on the outside, and set it to true on the inside, to
ensure the method is even called.

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:108: }());
What about tests for rebinding a method, given that those are just JSFunctions
too? (This is API is a bit weird imho...)

[Dmitry Lomov (no reviews), 2014-08-19 13:47:42]

Toon, thanks for taking a look, working on a better patch.

https://codereview.chromium.org/475423003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/475423003/diff/60001/src/heap/heap.cc#newcode...
src/heap/heap.cc:3758: if (source->IsJSFunction()) {
On 2014/08/18 14:15:35, Toon Verwaest wrote:
> Are you sure this works? What code object does this get? What if it's
optimized?
> If we also become optimized, will we get notified in case the function
> deoptimizes?
> 
> Can you just make a specialized JSFunction::Copy(), that preferably uses
> NewFunctionFromSharedFunctionInfo, but additionally copies over properties and
> prototype?

You are quite right, it is easy to write a test that crashes. JSFunction::Copy
is a way to go I guess. Another way would be to properly wire up the
JSFunction's clone into the functions' list etc. but that would break whenever
that logic changes because people will forget to update the cloning logic
(people will forget to update JSFunction::Copy logic as well that that might
just be a less frequent change)

[Dmitry Lomov (no reviews), 2014-08-19 20:02:48]

An implementation of JSFunction::Copy + more tests.

https://codereview.chromium.org/475423003/diff/60001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/60001/src/runtime.cc#newcode2063
src/runtime.cc:2063: Object::SetProperty(clone, home_object_symbol, home_object,
STRICT));
On 2014/08/18 14:15:35, Toon Verwaest wrote:
> Perhaps use JSObject::SetOwnPropertyIgnoreAttributes?

Done.

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:97: function f(x, y, z) {
On 2014/08/18 14:15:35, Toon Verwaest wrote:
> Add flag "called = false" on the outside, and set it to true on the inside, to
> ensure the method is even called.

Checking return value instead.

https://codereview.chromium.org/475423003/diff/60001/test/mjsunit/harmony/toM...
test/mjsunit/harmony/toMethod.js:108: }());
On 2014/08/18 14:15:35, Toon Verwaest wrote:
> What about tests for rebinding a method, given that those are just JSFunctions
> too? (This is API is a bit weird imho...)

Done.

[Toon Verwaest, 2014-08-20 07:43:11]

Added comments for the code, still need to look through the tests.
What about function proxies btw?

https://codereview.chromium.org/475423003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9415
src/objects.cc:9415: Handle<FixedArrayBase>
original_elements(function->elements());
Use the ElementsAccessor Copy method instead.

https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9433
src/objects.cc:9433: if (details.type() == NORMAL || details.type() == FIELD) {
NORMAL is not a valid entry for descriptors. Only FIELD needs to be copied.
We should better separate those ...

That also indicates though that if your function is in dictionary mode, you
don't copy anything from the backing store. Hence you'll lose all your
properties. For fast mode you copy them. Please add a test that normalizes the
function before binding. (The runtime function to turn something into dictionary
mode is called %OptimizeObjectForAddingMultipleProperties).

What about having separate methods to copy named and indexed properties,
extracted from the regular object copying mechanism?

https://codereview.chromium.org/475423003/diff/100001/src/property.h
File src/property.h (right):

https://codereview.chromium.org/475423003/diff/100001/src/property.h#newcode26
src/property.h:26: Descriptor() : details_(Smi::FromInt(0)) {}
Is this change necessary?

https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc#newcode2064
src/runtime.cc:2064: DONT_SHOW).Assert();
DONT_ENUM.

The other flags are just there for filtering. Probably we should extract them to
a separate filter class rather than having them as part of the regular
PropertyAttributes. We don't even have enough bits to encode DONT_SHOW into the
descriptor I believe.

[Dmitry Lomov (no reviews), 2014-08-20 10:13:43]

Thanks Toon!

On 2014/08/20 07:43:11, Toon Verwaest wrote:
> Added comments for the code, still need to look through the tests.
> What about function proxies btw?
I plan to just fail on them for now. Our implementation of proxies is not
functional anyway.
Let me know if you think it is an issue for production code as well.



> 
> https://codereview.chromium.org/475423003/diff/100001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9415
> src/objects.cc:9415: Handle<FixedArrayBase>
> original_elements(function->elements());
Will do.
> Use the ElementsAccessor Copy method instead.
> 
>
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9433
> src/objects.cc:9433: if (details.type() == NORMAL || details.type() == FIELD)
{
> NORMAL is not a valid entry for descriptors. Only FIELD needs to be copied.
> We should better separate those ...
> 
> That also indicates though that if your function is in dictionary mode, you
> don't copy anything from the backing store. Hence you'll lose all your
> properties. For fast mode you copy them. Please add a test that normalizes the
> function before binding. (The runtime function to turn something into
dictionary
> mode is called %OptimizeObjectForAddingMultipleProperties).
> 
> What about having separate methods to copy named and indexed properties,
> extracted from the regular object copying mechanism?

Thanks, and right, we should share the code between this function and
Heap::CopyJSObject.
AFAIU Heap::CopyJSObject has the same bug, i.e. it will shared the properties
dictionary between the source and the copy instead of copying it.
Is there any other object copying mechanism?

> 
> https://codereview.chromium.org/475423003/diff/100001/src/property.h
> File src/property.h (right):
> 
> https://codereview.chromium.org/475423003/diff/100001/src/property.h#newcode26
> src/property.h:26: Descriptor() : details_(Smi::FromInt(0)) {}
> Is this change necessary?

No, a stray change.

> 
> https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc#newcode2064
> src/runtime.cc:2064: DONT_SHOW).Assert();
> DONT_ENUM.
> 
> The other flags are just there for filtering. Probably we should extract them
to
> a separate filter class rather than having them as part of the regular
> PropertyAttributes. We don't even have enough bits to encode DONT_SHOW into
the
> descriptor I believe.

Will fix, thanks.

[Dmitry Lomov (no reviews), 2014-08-20 10:42:19]

On 2014/08/20 10:13:43, Dmitry Lomov (chromium) wrote:
> AFAIU Heap::CopyJSObject has the same bug, i.e. it will shared the properties
> dictionary between the source and the copy instead of copying it.
> Is there any other object copying mechanism?
ah, no, scratch that, CopyJSObject is fine.

[arv (Not doing code reviews), 2014-08-20 15:35:54]

https://codereview.chromium.org/475423003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9412
src/objects.cc:9412: clone->set_properties(
For F.p.toMethod we should not copy the properties. However if we use private
own property for the [[HomeObject]] we need to at least handle that.

https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc#newcode2058
src/runtime.cc:2058: Handle<JSFunction> clone = JSFunction::Copy(fun);
This is going to copy the properties which is not right.

https://codereview.chromium.org/475423003/diff/100001/test/mjsunit/harmony/to...
File test/mjsunit/harmony/toMethod.js (right):

https://codereview.chromium.org/475423003/diff/100001/test/mjsunit/harmony/to...
test/mjsunit/harmony/toMethod.js:1: // Copyright 2013 the V8 project authors.
All rights reserved.
Use short copyright header with the year 2014

https://codereview.chromium.org/475423003/diff/100001/test/mjsunit/harmony/to...
test/mjsunit/harmony/toMethod.js:132: (function TestPreserveProperties() {
This test is not right.

[Dmitry Lomov (no reviews), 2014-08-20 16:54:33]

On 2014/08/20 15:35:54, arv wrote:
> https://codereview.chromium.org/475423003/diff/100001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9412
> src/objects.cc:9412: clone->set_properties(
> For F.p.toMethod we should not copy the properties. However if we use private
> own property for the [[HomeObject]] we need to at least handle that.

Oh, you are right indeed! I have been looking as some old spec apparently.
This makes implementation much simpler of course.

[Dmitry Lomov (no reviews), 2014-08-20 18:03:20]

Thanks to arv@ it all got much simpler. PTAL.

[arv (Not doing code reviews), 2014-08-20 18:13:07]

LGTM

[Dmitry Lomov (no reviews), 2014-08-20 18:16:27]

On 2014/08/20 18:13:07, arv wrote:
> LGTM

Thanks for review! Toon, could you take a look as well?

[Dmitry Lomov (no reviews), 2014-08-20 18:52:29]

Added a test for extensibility change + renamed tests

[Toon Verwaest, 2014-08-21 09:02:20]

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc#newcode9302
src/objects.cc:9302: Handle<JSFunction> JSFunction::Copy(Handle<JSFunction>
function) {
This method doesn't exactly copy a function anymore. It just creates a new
closure with the same bindings, bound values, and prototype. Please rename to
indicate this.

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc#newcode9318
src/objects.cc:9318: JSObject::SetPrototype(clone, original_prototype,
false).Assert();
JSObject::SetPrototype isn't exactly a no-op right now; it does a whole lot of
stuff. Either manually check here, or there, whether it's the same object.

https://codereview.chromium.org/475423003/diff/160001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/160001/src/runtime.cc#newcode2064
src/runtime.cc:2064: DONT_ENUM).Assert();
Are you using "git cl format"? This seems weirdly formatted for our
auto-format...

[Dmitry Lomov (no reviews), 2014-08-21 11:09:12]

Comments addressed PTAL

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc#newcode9302
src/objects.cc:9302: Handle<JSFunction> JSFunction::Copy(Handle<JSFunction>
function) {
On 2014/08/21 09:02:20, Toon Verwaest wrote:
> This method doesn't exactly copy a function anymore. It just creates a new
> closure with the same bindings, bound values, and prototype. Please rename to
> indicate this.

Done.

https://codereview.chromium.org/475423003/diff/160001/src/objects.cc#newcode9318
src/objects.cc:9318: JSObject::SetPrototype(clone, original_prototype,
false).Assert();
On 2014/08/21 09:02:20, Toon Verwaest wrote:
> JSObject::SetPrototype isn't exactly a no-op right now; it does a whole lot of
> stuff. Either manually check here, or there, whether it's the same object.

Done.

https://codereview.chromium.org/475423003/diff/160001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/475423003/diff/160001/src/runtime.cc#newcode2064
src/runtime.cc:2064: DONT_ENUM).Assert();
On 2014/08/21 09:02:20, Toon Verwaest wrote:
> Are you using "git cl format"? This seems weirdly formatted for our
> auto-format...

Done.

[Toon Verwaest, 2014-08-21 11:32:28]

lgtm with nit

https://codereview.chromium.org/475423003/diff/180001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/475423003/diff/180001/src/objects.cc#newcode9315
src/objects.cc:9315: // prototype, which means that SetPrototype below is a
no-op.
Now this comment is a bit off

[Dmitry Lomov (no reviews), 2014-08-21 12:39:48]

Committed patchset #10 manually as 23274 (presubmit successful).