Bug 155336 – Implement Function.name and Function#toString for ES6 class.

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

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/25098147>

[Mark Lam]

Created attachment 273725 [details]
proposed patch.

Still need to run benchmarks.  This patch has passed the JSC and layout tests.

[Geoffrey Garen]

Comment on attachment 273725 [details]
proposed patch.

r=me

[Saam Barati]

Comment on attachment 273725 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/Executable.cpp:513
> +    m_classSource = unlinkedExecutable->classSource();

Is this correct with code caching?
Is it possible to construct a conflict here where we get
instantiated with an unlinked code block that isn't from a class constructor?

[Mark Lam]

Comment on attachment 273725 [details]
proposed patch.

Let me double check the UnlinkedFunctionExecutable caching issue.

[Saam Barati]

To me it seems like we could construct the same exact
normal "function(){...}" function, call new on it, and then
have a class' constructor be the same source code, and get a
conflict. I don't remember exactly how the hash is determined,
but it seems possible that this could happen.

[Saam Barati]

(In reply to comment #6)
> To me it seems like we could construct the same exact
> normal "function(){...}" function, call new on it, and then
> have a class' constructor be the same source code, and get a
> conflict. I don't remember exactly how the hash is determined,
> but it seems possible that this could happen.

Or even two different classes with the same constructor

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #6)
> > To me it seems like we could construct the same exact
> > normal "function(){...}" function, call new on it, and then
> > have a class' constructor be the same source code, and get a
> > conflict. I don't remember exactly how the hash is determined,
> > but it seems possible that this could happen.
> 
> Or even two different classes with the same constructor

I'm aware of the theoretical issue, but testing shows that there's no issue.  I'll dig into the code a bit and will update later with reasons why there is / isn't an issue.

[Darin Adler]

Comment on attachment 273725 [details]
proposed patch.

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

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:46
>      const Identifier* className = 0;

nullptr

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:50
> +    unsigned startOffset = 0;
> +    unsigned endOffset = 0;
> +    int startLine = 0;
> +    unsigned startColumn = 0;

In other WebKit code we have been using the brace syntax:

    unsigned startOffset { 0 };

Not sure it’s better, but it would be nice to be consistent in style.

[Mark Lam]

Created attachment 273755 [details]
proposed patch with more tests, some additional comments, + darin's feedback.

I investigated the potential code caching issue, and concluded that there is no issue.  This is because we currently only do caching of unlinked executables for global programs or dynamically created functions via the Function constructor.  Class constructors are not cached, and hence, it is safe to stash the class source in its unlinked function executable.

function-toString-vs-name.js already tests for this caching behavior, and I've also added some additional tests to check for global, eval, and Function constructor cases in this revised patch.  If the code caching behavior changes in the future, the tests in function-toString-vs-name.js will catch it.

[Mark Lam]

Created attachment 273757 [details]
diff of the old patch vs the current one: for your reviewing convenience.

[Mark Lam]

Comment on attachment 273755 [details]
proposed patch with more tests, some additional comments, + darin's feedback.

Saam tells me (offline) that he doesn't think this need additional review.  Will land.

[Mark Lam]

Thanks for all the reviews.  Landed in r198042: <http://trac.webkit.org/r198042>.

[Ryosuke Niwa]

*** Bug 144285 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 149743 has been marked as a duplicate of this bug. ***

[Jordan Harband]

*** Bug 149743 has been marked as a duplicate of this bug. ***