1463529 - Don't insert anything between "function" and function name in toString under any circumstances.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Eric Faust [:efaust]]

Attachment: Patch

TC39 requested that we stop putting "get" between "function" and "[Symbol.species]" in a slidedeck at the May 2018 meeting. This only happens for self-hosted builtins. Stop doing that.

Comment 1

[André Bargull [:anba]]

Did they really propose in the meeting to remove 9.2.13 SetFunctionName, step 5 <https://tc39.github.io/ecma262/#sec-setfunctionname> from the spec?

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8979697 [details] [diff] [review]
Patch

Evilpie pointed out that maybe we want this to be "get [Symbol.species]", not "function [Symbol.species]". I think I agree. Canceling for the moment.

Comment 3

[Eric Faust [:efaust]]

No, I broke stuff :/

What we currently print is "function get [Symbol.species]" and I made it "function [Symbol.species]" instead of "get [Symbol.species]"

Comment 4

[André Bargull [:anba]]

Attachment: bug1463529.patch

Stealing.

Change the Function.prototype.toString implementation, so it doesn't output any prefix like "get " or "bound " before the function name. The toString output for bound functions now always excludes the name to avoid parsing it to find a suitable substring which can be used as the name. (*) (**)

The "Function.prototype.toString revision" proposal also doesn't allow to output "[sourceless code]", instead we always need to output "[native code]" when no source text is available.
I've kept the existing code for Function.prototype.toSource because it's non-standard, so we can return whatever we like, and because the existing output is slightly more detailed, it may make sense to keep it.


(*) The name part must match the |PropertyName| grammar production per the "Function.prototype.toString revision" proposal.
(**) Well, actually we could always output the name for bound functions while still being compliant to the "Function.prototype.toString revision" proposal: We just need to place it into a string literal, because string literals are valid |PropertyName|s. For example |(function fn(){}.bind().toString()| would then return:
  function "bound fn"() {
      [native code]
  }

But that feels like an intended loophole in the current proposal. :-)

Comment 5

[André Bargull [:anba]]

(In reply to André Bargull [:anba] from comment #4)
> But that feels like an intended loophole in the current proposal. :-)

/intended/unintended/s

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 9004602 [details] [diff] [review]
bug1463529.patch

Review of attachment 9004602 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review flag, as I am confused (see below).

I'm sorry for the slow review here, anba. I'll be quicker about it for any revised patch...

::: js/src/tests/non262/Function/function-toString-builtin-name.js
@@ +4,5 @@
> +// This behaviour is not required by the ECMAScript standard.
> +
> +// Greatly (!) simplified patterns for the PropertyName production.
> +var propertyName = [
> +    // PropertyName :: LiteralPropertyName :: IdentifierName 

trailing whitespace here (and in the other tests)

::: js/src/tests/non262/Function/function-toString-discard-source-name.js
@@ +120,5 @@
> +   set x(_) {},
> +};
> +`);
> +
> +assertFunctionName(obj.m, undefined);

This seems to contradict the current proposal. Currently (without this patch) SpiderMonkey has `obj.m.toString()` return "m() {}". The proposal requires exactly that result, right? But the test seems to expect something like `function () {}`.

::: js/src/vm/JSFunction.cpp
@@ +1040,5 @@
> +                    : hasGetterOrSetterPrefix(name->twoByteChars(nogc)));
> +        };
> +
> +        if (!out.append("function"))
> +            return nullptr;

And here's the new code implementing this change... I'm just confused about what the proposal says, maybe. It seems to me that the proposal always requires returning exactly the source text matched by the method, function, class, etc. -- which often won't contain "function" at all.

Comment 7

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> ::: js/src/tests/non262/Function/function-toString-discard-source-name.js
> @@ +120,5 @@
> > +   set x(_) {},
> > +};
> > +`);
> > +
> > +assertFunctionName(obj.m, undefined);
> 
> This seems to contradict the current proposal. Currently (without this
> patch) SpiderMonkey has `obj.m.toString()` return "m() {}". The proposal
> requires exactly that result, right? But the test seems to expect something
> like `function () {}`.

The proposal requires to return the exact slice of source code - when it is available. When it's not available, which this test is supposed to cover, unless there's an error in the set-up, the proposal says to return a |NativeFunction| representation. This is specified in step 2 of <https://tc39.github.io/Function-prototype-toString-revision/#proposal-sec-function.prototype.tostring>.

Comment 8

[André Bargull [:anba]]

Attachment: bug1463529.patch

Rebased patch and removed trailing whitespace in test files per review comments.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 9010646 [details] [diff] [review]
bug1463529.patch

Review of attachment 9010646 [details] [diff] [review]:
-----------------------------------------------------------------

That makes sense. Thanks.

::: js/src/wasm/AsmJS.cpp
@@ +7630,5 @@
> +            if (!out.append("() {\n    [native code]\n}")) {
> +                return nullptr;
> +            }
> +        } else {
> +            if (!out.append("() {\n    [sourceless code]\n}")) {

Let's use `[native code]` for both.

I'm awfully close to preferring that f.toSource() and f.toString() return the same string across the board. Any differences should be clearly more valuable than consistency... anyway, I'm not asking for a big overhaul of this patch, but we can at least retire "[sourceless code]".

@@ +7686,5 @@
> +            if (!out.append("() {\n    [native code]\n}")) {
> +                return nullptr;
> +            }
> +        } else {
> +            if (!out.append("() {\n    [sourceless code]\n}")) {

And here.

Comment 10

[André Bargull [:anba]]

Attachment: bug1463529.patch

Updated patch to replace all three "[sourceless code]" strings with "[native code]" per review comments, carrying r+.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=201214529&revision=391dff15c6422f93d48a12f6b8447b4a8741e0dc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de7c162147f9
Don't add modifiers to built-in or bound function toString representation. r=jorendorff

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/de7c162147f9