Bug 154865 - Implement Function.name support for getters/setters and inferring name of function properties.
Summary: Implement Function.name support for getters/setters and inferring name of fun...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 155169
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-01 11:09 PST by Mark Lam
Modified: 2016-03-08 16:22 PST (History)
7 users (show)

See Also:


Attachments
proposed patch. (44.44 KB, patch)
2016-03-08 13:47 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
x86_64 benchmark result. (68.21 KB, text/plain)
2016-03-08 15:07 PST, Mark Lam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-03-01 11:09:14 PST
THE ECMA spec on assignment (https://tc39.github.io/ecma262/#sec-assignment-operators-runtime-semantics-evaluation) says:

If IsAnonymousFunctionDefinition(AssignmentExpression) and IsIdentifierRef of LeftHandSideExpression are both true, then
    Let hasNameProperty be ? HasOwnProperty(rval, "name").
    If hasNameProperty is false, perform SetFunctionName(rval, GetReferencedName(lref)).

This means the following:

    var foo = function() {};
    var bar = function baz() {};
    print(foo.name); // Should be "foo" implied from the var name.
    print(bar.name); // Should be "baz" because it's explicitly specified and not anonymous.

Implementing this behavior exposes a nuanced behavior in JSC.  Note that the default attributes of Function.name are [[Writable]]:false [[Enumerable]]:false [[Configurable]]:true.  While Function.name is writable by default, it can be made writable.  The issue to consider is what should toString() print as the name of the function if we alter Function.name.

On Chrome and Firefox (after changing the name property to be writable):
    Function.prototype.apply.name = "foo";
    print(Function.prototype.apply); // Prints "function apply() { [native code] }".

In JSC with the above spec fix, we'll get:
    Function.prototype.apply.name = "foo";
    print(Function.prototype.apply); // Prints "function foo() { [native code] }".

Strictly speaking, this issue is already in ToT, but the above spec fix will start manifesting test failures due to this issue.

According to the Jan 28, 2016 TC-39 discussion on Function#toString (https://github.com/rwaldron/tc39-notes/blob/master/es7/2016-01/2016-01-28.md),

    discussion about what method name to use, and what if it isn't an identifier
    MM: Don't use the current value of the name property.

There wasn't any other discussion of what name to use beyond that, but Chrome and Firefox's implementations implies that they have consensus that the toString'ed function name should be the original name of the function, and not the value of Function.name (i.e. JSC needs a fix).
Comment 1 Radar WebKit Bug Importer 2016-03-01 11:11:27 PST
<rdar://problem/24911851>
Comment 2 Mark Lam 2016-03-08 13:47:44 PST
Created attachment 273330 [details]
proposed patch.

Let's let the EWS bot chew on it a bit first while I run some benchmarks on this.
Comment 3 Mark Lam 2016-03-08 15:07:40 PST
Created attachment 273343 [details]
x86_64 benchmark result.

The benchmark results says that this patch is perf neutral.  The "definitely" pieces do not reproduce on re-runs of the benchmark.
Comment 4 Geoffrey Garen 2016-03-08 15:28:22 PST
Comment on attachment 273330 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/InternalFunction.cpp:64
> +    ASSERT_UNUSED(exec, !exec->hadException()); // m_originalName was built from a String, and hence, there is no rope to resolve.

Let's ASSERT(name) here instead.

> Source/JavaScriptCore/runtime/JSFunction.cpp:601
> +        name = jsExecutable()->inferredName().string();

Let's add an ecmaName() to FunctionExecutable, and call it here. For now, ecmaName() can return inferredName(), with a FIXME that says this is wrong.
Comment 5 Mark Lam 2016-03-08 16:02:17 PST
Thanks for the review.  I've applied the requested changes locally before landing.

Landed in r197815: <http://trac.webkit.org/r197815>.
Comment 6 Joseph Pecoraro 2016-03-08 16:04:47 PST
Comment on attachment 273330 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:211
> +    if (shouldThrow && !exec->hadException())
>      if (shouldThrow)

What happened here?
Comment 7 Joseph Pecoraro 2016-03-08 16:08:00 PST
Comment on attachment 273330 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSBoundSlotBaseFunction.cpp:70
> +    String prefix = (type == Type::Getter) ? "get " : "set ";

This could be a `const char*` for use in makeString. Or ASCIILiteral() with WTFString.
Comment 8 Mark Lam 2016-03-08 16:14:12 PST
(In reply to comment #6)
> > Source/JavaScriptCore/runtime/JSCJSValue.cpp:211
> > +    if (shouldThrow && !exec->hadException())
> >      if (shouldThrow)
> 
> What happened here?

This was work in progress that is now obsolete, but I failed to clean it up.  Will remove.

(In reply to comment #7)
> > Source/JavaScriptCore/runtime/JSBoundSlotBaseFunction.cpp:70
> > +    String prefix = (type == Type::Getter) ? "get " : "set ";
> 
> This could be a `const char*` for use in makeString. Or ASCIILiteral() with
> WTFString.

I'll change it to const char*.
Comment 9 Mark Lam 2016-03-08 16:22:12 PST
Follow up fix to address the issues Joe pointed out landed in r197817: <http://trac.webkit.org/r197817>.