Bug 153796 - [ES6] bound functions .name property should be "bound " + the target function's name
Summary: [ES6] bound functions .name property should be "bound " + the target function...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-02 12:24 PST by Saam Barati
Modified: 2016-02-07 15:16 PST (History)
18 users (show)

See Also:


Attachments
patch (2.73 KB, patch)
2016-02-02 13:08 PST, Saam Barati
mark.lam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (931.14 KB, application/zip)
2016-02-02 13:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.12 MB, application/zip)
2016-02-02 13:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-02-02 13:58 PST, Build Bot
no flags Details
patch (5.51 KB, patch)
2016-02-02 14:45 PST, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (9.81 KB, patch)
2016-02-02 15:06 PST, Saam Barati
no flags Details | Formatted Diff | Diff
follow-up patch (5.64 KB, patch)
2016-02-03 00:55 PST, Saam Barati
no flags Details | Formatted Diff | Diff
follow-up patch (5.69 KB, patch)
2016-02-03 01:06 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Saam Barati 2016-02-02 13:08:15 PST
Created attachment 270510 [details]
patch
Comment 2 Mark Lam 2016-02-02 13:12:16 PST
Comment on attachment 270510 [details]
patch

r=me
Comment 3 Build Bot 2016-02-02 13:46:40 PST
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
Comment 4 Build Bot 2016-02-02 13:46:43 PST
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
Comment 5 Build Bot 2016-02-02 13:55:28 PST
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
Comment 6 Build Bot 2016-02-02 13:55:32 PST
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
Comment 7 Build Bot 2016-02-02 13:58:50 PST
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
Comment 8 Build Bot 2016-02-02 13:58:53 PST
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
Comment 9 Saam Barati 2016-02-02 14:45:58 PST
Created attachment 270522 [details]
patch

updated toString() of bound functions.
Comment 10 WebKit Commit Bot 2016-02-02 14:48:08 PST
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.
Comment 11 Ryosuke Niwa 2016-02-02 14:48:58 PST
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?
Comment 12 Mark Lam 2016-02-02 14:49:00 PST
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.
Comment 13 Saam Barati 2016-02-02 14:59:18 PST
(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.
Comment 14 Saam Barati 2016-02-02 15:06:28 PST
Created attachment 270525 [details]
patch for landing

updated test results.
Comment 15 WebKit Commit Bot 2016-02-02 15:07:59 PST
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.
Comment 16 Saam Barati 2016-02-02 15:28:13 PST
landed in:
http://trac.webkit.org/changeset/196033
Comment 17 Darin Adler 2016-02-02 16:32:33 PST
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.
Comment 18 Darin Adler 2016-02-02 16:49:01 PST
(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!
Comment 19 Joseph Pecoraro 2016-02-02 18:24:33 PST
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 "
Comment 20 Saam Barati 2016-02-02 18:58:30 PST
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
Comment 21 Saam Barati 2016-02-02 19:04:05 PST
(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.
Comment 22 Saam Barati 2016-02-02 19:04:59 PST
(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.
Comment 23 Saam Barati 2016-02-02 19:13:29 PST
(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.
Comment 24 Saam Barati 2016-02-02 19:14:12 PST
(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"
Comment 25 Saam Barati 2016-02-02 23:59:42 PST
Reopening because I'm going to post follow-ups under this bug.
Comment 26 Saam Barati 2016-02-03 00:55:43 PST
Created attachment 270563 [details]
follow-up patch
Comment 27 Saam Barati 2016-02-03 01:06:06 PST
Created attachment 270566 [details]
follow-up patch

change test output
Comment 28 Mark Lam 2016-02-03 07:48:16 PST
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?
Comment 29 Saam Barati 2016-02-03 11:09:30 PST
(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.
Comment 30 WebKit Commit Bot 2016-02-07 15:16:24 PST
Comment on attachment 270566 [details]
follow-up patch

Clearing flags on attachment: 270566

Committed r196243: <http://trac.webkit.org/changeset/196243>
Comment 31 WebKit Commit Bot 2016-02-07 15:16:32 PST
All reviewed patches have been landed.  Closing bug.