Bug 153796 – [ES6] bound functions .name property should be "bound " + the target function's name

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

see http://tc39.github.io/ecma262/#sec-function.prototype.bind

[Saam Barati]

Created attachment 270510 [details]
patch

[Mark Lam]

Comment on attachment 270510 [details]
patch

r=me

[Build Bot]

Comment on attachment 270510 [details]
patch

Attachment 270510 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/773879

New failing tests:
http/tests/media/media-source/SourceBuffer-abort-removed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
inspector/runtime/getProperties.html
imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html

[Build Bot]

Created attachment 270512 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270510 [details]
patch

Attachment 270510 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/773942

New failing tests:
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html
http/tests/media/media-source/SourceBuffer-abort-removed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/dom/interfaces.html
inspector/runtime/getProperties.html

[Build Bot]

Created attachment 270514 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270510 [details]
patch

Attachment 270510 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/773944

New failing tests:
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html
http/tests/media/media-source/SourceBuffer-abort-removed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/dom/interfaces.html
inspector/runtime/getProperties.html

[Build Bot]

Created attachment 270516 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 270522 [details]
patch

updated toString() of bound functions.

[WebKit Commit Bot]

Attachment 270522 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 270522 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> +    String result = name(exec);
> +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> +    static const size_t boundPrefixLength = 6;
> +    return result.substring(boundPrefixLength);

Can't we just get the name out of m_targetFunction?

[Mark Lam]

Comment on attachment 270522 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review

r=me

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:90
> +static const char* boundPrefix = "bound ";

nit: this can be static const char* const boundPrefix = "bound ";

There's no need for the pointer to not be const.

[Saam Barati]

(In reply to comment #11)
> Comment on attachment 270522 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> 
> > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> > +    String result = name(exec);
> > +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> > +    static const size_t boundPrefixLength = 6;
> > +    return result.substring(boundPrefixLength);
> 
> Can't we just get the name out of m_targetFunction?

It's because m_targetFunction might not be a JSFunction.
It could be an InternalFunction too. I think it's cleaner
to just do it this way.

[Saam Barati]

Created attachment 270525 [details]
patch for landing

updated test results.

[WebKit Commit Bot]

Attachment 270525 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/196033

[Darin Adler]

Comment on attachment 270525 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=270525&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:90
> +static const char* const boundPrefix = "bound ";

We normally write this:

    static const char boundPrefix[] = "bound ";

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:100
> +    function->finishCreation(vm, executable, length, makeString(ASCIILiteral(boundPrefix), name));

I think that stating ASCIILiteral here explicitly might actually make things slower by causing a string to be allocated. It’s an optimization for creating a string, not needed when concatenating to make a string. Please just write "boundPrefix + name" or "makeString(boundPrefix, name)", both of which should be efficient.

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140
> +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);

This should be:

    ASSERT(result.startsWith(boundPrefix));

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:141
> +    static const size_t boundPrefixLength = 6;

The substring function takes an unsigned, not a size_t.

Might be nice to just use strlen(boundPrefix) instead of boundPrefixLength. I believe modern compilers do constant folding in this kind of case.

[Darin Adler]

(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 270522 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> > > +    String result = name(exec);
> > > +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> > > +    static const size_t boundPrefixLength = 6;
> > > +    return result.substring(boundPrefixLength);
> > 
> > Can't we just get the name out of m_targetFunction?
> 
> It's because m_targetFunction might not be a JSFunction.
> It could be an InternalFunction too. I think it's cleaner
> to just do it this way.

Maybe more expedient. Not cleaner!

[Joseph Pecoraro]

Comment on attachment 270522 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review

> Source/JavaScriptCore/ChangeLog:15
> +        See http://tc39.github.io/ecma262/#sec-function.prototype.bind for details.
> +        What the spec says:
> +        ```
> +        function foo() { }
> +        foo.bind(null).name === "bound foo"
> +
> +        (function bar() { }).bind(null).name === "bound bar"
> +        ```

It would be nice to have more tests for this.

Some examples:

    // Nested
    (function foo() {}).bind(null).bind(null).name === "bound bound foo"

    // Anonymous
    (function(){}).bind(null).name === "bound "

[Saam Barati]

Comment on attachment 270525 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=270525&action=review

I'll come up w/ a follow-up patch to address your comments.

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:100
>> +    function->finishCreation(vm, executable, length, makeString(ASCIILiteral(boundPrefix), name));
> 
> I think that stating ASCIILiteral here explicitly might actually make things slower by causing a string to be allocated. It’s an optimization for creating a string, not needed when concatenating to make a string. Please just write "boundPrefix + name" or "makeString(boundPrefix, name)", both of which should be efficient.

I'll go with makeString(.)

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140
>> +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> 
> This should be:
> 
>     ASSERT(result.startsWith(boundPrefix));

neat. I didn't know we had this function.

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:141
>> +    static const size_t boundPrefixLength = 6;
> 
> The substring function takes an unsigned, not a size_t.
> 
> Might be nice to just use strlen(boundPrefix) instead of boundPrefixLength. I believe modern compilers do constant folding in this kind of case.

I'll go with strlen

[Saam Barati]

(In reply to comment #18)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Comment on attachment 270522 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> > > 
> > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> > > > +    String result = name(exec);
> > > > +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> > > > +    static const size_t boundPrefixLength = 6;
> > > > +    return result.substring(boundPrefixLength);
> > > 
> > > Can't we just get the name out of m_targetFunction?
> > 
> > It's because m_targetFunction might not be a JSFunction.
> > It could be an InternalFunction too. I think it's cleaner
> > to just do it this way.
> 
> Maybe more expedient. Not cleaner!

Expediency wasn't my goal here. I spent more time debating
about whether or not to do it this way than the time it would
have taken me to write the extra ~3 lines of code.
My thinking about this is as follows:
If we case on everything that could be bound as the target of a bound function,
any time we add a new thing that can be bound by a JSBoundFunction, we have
yet even one more function to change. There is something nice about an implementer
not worrying about this function, and this function not worrying about the rest
of the runtime.

[Saam Barati]

(In reply to comment #19)
> Comment on attachment 270522 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> 
> > Source/JavaScriptCore/ChangeLog:15
> > +        See http://tc39.github.io/ecma262/#sec-function.prototype.bind for details.
> > +        What the spec says:
> > +        ```
> > +        function foo() { }
> > +        foo.bind(null).name === "bound foo"
> > +
> > +        (function bar() { }).bind(null).name === "bound bar"
> > +        ```
> 
> It would be nice to have more tests for this.
> 
> Some examples:
> 
>     // Nested
>     (function foo() {}).bind(null).bind(null).name === "bound bound foo"
This is a good test to have, I'll add it in a follow-up patch.

> 
>     // Anonymous
>     (function(){}).bind(null).name === "bound "
We already have a test for this inside the ES6 test suite.

[Saam Barati]

(In reply to comment #21)
> (In reply to comment #18)
> > (In reply to comment #13)
> > > (In reply to comment #11)
> > > > Comment on attachment 270522 [details]
> > > > patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> > > > 
> > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> > > > > +    String result = name(exec);
> > > > > +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> > > > > +    static const size_t boundPrefixLength = 6;
> > > > > +    return result.substring(boundPrefixLength);
> > > > 
> > > > Can't we just get the name out of m_targetFunction?
> > > 
> > > It's because m_targetFunction might not be a JSFunction.
> > > It could be an InternalFunction too. I think it's cleaner
> > > to just do it this way.
> > 
> > Maybe more expedient. Not cleaner!
> 
> Expediency wasn't my goal here. I spent more time debating
> about whether or not to do it this way than the time it would
> have taken me to write the extra ~3 lines of code.
> My thinking about this is as follows:
> If we case on everything that could be bound as the target of a bound
> function,
> any time we add a new thing that can be bound by a JSBoundFunction, we have
> yet even one more function to change. There is something nice about an
> implementer
> not worrying about this function, and this function not worrying about the
> rest
> of the runtime.
Actually, this is really silly.
We can just do m_target->get(exec, exec->vm().propertyNames->name).toWTFString(exec);
This is what JSFunction::name does, and it's to inline here
because it's expected that function-like things to store to that property.

[Saam Barati]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #18)
> > > (In reply to comment #13)
> > > > (In reply to comment #11)
> > > > > Comment on attachment 270522 [details]
> > > > > patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142
> > > > > > +    String result = name(exec);
> > > > > > +    ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0);
> > > > > > +    static const size_t boundPrefixLength = 6;
> > > > > > +    return result.substring(boundPrefixLength);
> > > > > 
> > > > > Can't we just get the name out of m_targetFunction?
> > > > 
> > > > It's because m_targetFunction might not be a JSFunction.
> > > > It could be an InternalFunction too. I think it's cleaner
> > > > to just do it this way.
> > > 
> > > Maybe more expedient. Not cleaner!
> > 
> > Expediency wasn't my goal here. I spent more time debating
> > about whether or not to do it this way than the time it would
> > have taken me to write the extra ~3 lines of code.
> > My thinking about this is as follows:
> > If we case on everything that could be bound as the target of a bound
> > function,
> > any time we add a new thing that can be bound by a JSBoundFunction, we have
> > yet even one more function to change. There is something nice about an
> > implementer
> > not worrying about this function, and this function not worrying about the
> > rest
> > of the runtime.
> Actually, this is really silly.
> We can just do m_target->get(exec,
> exec->vm().propertyNames->name).toWTFString(exec);
> This is what JSFunction::name does, and it's to inline here
> because it's expected that function-like things to store to that property.
"it's to" => "it's appropriate to"

[Saam Barati]

Reopening because I'm going to post follow-ups under this bug.

[Saam Barati]

Created attachment 270563 [details]
follow-up patch

[Saam Barati]

Created attachment 270566 [details]
follow-up patch

change test output

[Mark Lam]

Comment on attachment 270566 [details]
follow-up patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270566&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:137
> +    return m_targetFunction->get(exec, exec->vm().propertyNames->name).toWTFString(exec);

What happens if m_targetFunction->get() throws an exception?  Can you add a test case where the bound function has a getter on name that throws?

[Saam Barati]

(In reply to comment #28)
> Comment on attachment 270566 [details]
> follow-up patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270566&action=review
> 
> > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:137
> > +    return m_targetFunction->get(exec, exec->vm().propertyNames->name).toWTFString(exec);
> 
> What happens if m_targetFunction->get() throws an exception?  Can you add a
> test case where the bound function has a getter on name that throws?

Unfortunately it's not that easy. "name" is a non-writable, non-configurable property.
I'm sure there are really weird/rare situations where we might throw an error
because of OOM or something when getting this property (for any number of reasons),
but I'm not sure how to write a straight-forward test that does that.

[WebKit Commit Bot]

Comment on attachment 270566 [details]
follow-up patch

Clearing flags on attachment: 270566

Committed r196243: <http://trac.webkit.org/changeset/196243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.