Bug 154865 – Implement Function.name support for getters/setters and inferring name of function properties.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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).

[Radar WebKit Bug Importer]

<rdar://problem/24911851>

[Mark Lam]

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.

[Mark Lam]

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.

[Geoffrey Garen]

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.

[Mark Lam]

Thanks for the review.  I've applied the requested changes locally before landing.

Landed in r197815: <http://trac.webkit.org/r197815>.

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Mark Lam]

(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*.

[Mark Lam]

Follow up fix to address the issues Joe pointed out landed in r197817: <http://trac.webkit.org/r197817>.