1266255 - Implement function name semantics for [non-computed] method names and simple assignments.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Morgan Phillips [:mrrrgn]]

This should cover basic cases like:

let foo = () => 1; foo.name === "foo";
let bar = {"some1+2": () => 1}; bar["some1+2"].name === "some1+2";

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: funcnames-parsing.diff

I opted to leave escaping inside of my parsing function because I don't need a full implementation -- unicode characters are already handled by the lexer, and so never become escaped. This non-standard behavior makes the code unsuitable for general purpose use.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8743613 [details] [diff] [review]
funcnames-parsing.diff

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

Great tests. Clearing review because I think there are some bugs to fix...

* Please also test that it works when initializing a var or const, and when assigning to a variable that isn't defined anywhere.

* Also check that nested functions that are supposed to be nameless actually are.

* (function(){}).hasOwnProperty("name") should be false. But ({"": function(){}})[""].hasOwnProperty("name") should be true. Simply *not* defining a property in fun_resolve is easy, but distinguishing these two cases seems not so easy.

* Please file a follow-up bug for assigning names to classes. It will be annoying because we have to make sure it does *not* happen for classes that have a `static name()` method.

Morgan ... I hate to bring this up so late, but the alternative to all this is to store an extra JSAtom pointer in every JSScript. We can easily simulate the overhead by calling makeAtomIndex(any_atom, &dontcare) in BytecodeEmitter::init(). Is it noticeable on Octane?

::: js/src/jsfun.cpp
@@ +1485,5 @@
> +    size_t start = 0, end = textLen;
> +
> +    for (size_t i = 0; i < textLen; i++) {
> +
> +        // The string is parsed in reverse order.

Style nit: no blank line before this comment, since it's at the top of a block.

::: js/src/tests/ecma_6/Function/name.js
@@ +53,5 @@
> +assertEq(obj.noOverride.name, "named");
> +assertEq(obj.obj2.obj3.funky.name, "funky");
> +assertEq(obj["1 + 2"].name, "1 + 2");
> +assertEq(obj['" \\ \n \t \v \r \b \f \0'].name, "\" \\ \n \t \v \r \b \f \0");
> +assertEq(obj["\u9999"].name, "\u9999");

Add a test for "\u9999 " (two characters -- a unicode character and a space).

Comment 3

[Morgan Phillips [:mrrrgn]]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Comment on attachment 8743613 [details] [diff] [review]
> funcnames-parsing.diff
> 
> Review of attachment 8743613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great tests. Clearing review because I think there are some bugs to fix...
> 
> * Please also test that it works when initializing a var or const, and when
> assigning to a variable that isn't defined anywhere.

Yeah destructuring doesn't work here :(
js> let {a} = {a: () => 1}; a.name
""

> * Also check that nested functions that are supposed to be nameless actually
> are.
> 
> * (function(){}).hasOwnProperty("name") should be false. But ({"":
> function(){}})[""].hasOwnProperty("name") should be true. Simply *not*
> defining a property in fun_resolve is easy, but distinguishing these two
> cases seems not so easy.

This actually turns out to be rather easy. The first case has a null "fun->atom_" [display name] and the second
doesn't.

> * Please file a follow-up bug for assigning names to classes. It will be
> annoying because we have to make sure it does *not* happen for classes that
> have a `static name()` method.
> 
> Morgan ... I hate to bring this up so late, but the alternative to all this
> is to store an extra JSAtom pointer in every JSScript. We can easily
> simulate the overhead by calling makeAtomIndex(any_atom, &dontcare) in
> BytecodeEmitter::init(). Is it noticeable on Octane?

The tricky thing here is that functionName also touches native functions, I was under the impression that
they had no script object. I could simply fall back to atom_ if there is no script object. I'll think about
whether this is worthwhile, though you'd know better than I.

> ::: js/src/jsfun.cpp
> @@ +1485,5 @@
> > +    size_t start = 0, end = textLen;
> > +
> > +    for (size_t i = 0; i < textLen; i++) {
> > +
> > +        // The string is parsed in reverse order.
> 
> Style nit: no blank line before this comment, since it's at the top of a
> block.
> 
> ::: js/src/tests/ecma_6/Function/name.js
> @@ +53,5 @@
> > +assertEq(obj.noOverride.name, "named");
> > +assertEq(obj.obj2.obj3.funky.name, "funky");
> > +assertEq(obj["1 + 2"].name, "1 + 2");
> > +assertEq(obj['" \\ \n \t \v \r \b \f \0'].name, "\" \\ \n \t \v \r \b \f \0");
> > +assertEq(obj["\u9999"].name, "\u9999");
> 
> Add a test for "\u9999 " (two characters -- a unicode character and a space).

Comment 4

[Morgan Phillips [:mrrrgn]]

I'm going forward with the parsing approach. Because ``NameFunctions``, which generates display names, is being called in BytecodeCompiler.cpp when no script or lazy script is available to attach data to. Taking another approach means both duplicating work and increasing memory usage. It also seems confusing to store function names in two different locations depending on context.

That is: function foo() {}; will have its name stored in the function object's `atom_` while var foo = function () {} will have its name stored in a script object. I say boo to that. Having a little parser is ugly, but at least the ugliness will live in one well known location.

Comment 5

[Morgan Phillips [:mrrrgn]]

* having a little display name parser is ugly.

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: funcnames-parsing.diff

I believe this covers the bugs regarding unicode sequences you'd pointed out.

With respect to destructuring and Object.hasOwnProperty("name") I believe separate patches should be submitted since those will require additional changes to jsfun.cpp and, more importantly, NameFunctions.cpp

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8746369 [details] [diff] [review]
funcnames-parsing.diff

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

OK! Sorry for the slowish review. Was a little buried.

::: js/src/jsfun.cpp
@@ +512,5 @@
>                      return true;
>              }
>  
> +            JSAtom* functionName = fun->functionName(cx);
> +            v.setString(functionName == nullptr ? cx->runtime()->emptyString : functionName);

We have to distinguish between failure (out of memory) and success-but-the-function-is-nameless cases.

Maybe it's best to use null for failure and have JSFunction::functionName() return the empty string on success. The other alternative is to give functionName() an out parameter that's only set on success, like:

    bool functionName(JSContext* cx, MutableHandleAtom name);

@@ +1486,5 @@
> +    size_t end = start + length;
> +    for (size_t i = start; i < end; i++) {
> +
> +        if (text[i] == '\\' && i + 1 <= end) {
> +            bool matched = false;

"matched"?

@@ +1516,5 @@
> +                        JS7_ISHEX(text[i + 1]) &&
> +                        JS7_ISHEX(text[i + 2]) &&
> +                        JS7_ISHEX(text[i + 3]) &&
> +                        JS7_ISHEX(text[i + 4])))
> +                            return false;

Style nit: Please add curly braces like this:

    if (!(blah &&
          blah &&
          blah &&
          blah))
    {
        return false;
    }

(the style rule is, if the condition, "then"-statement and "else"-statement (if any) fit on a single line each, skip the braces)

@@ +1584,5 @@
> +                    return UnescapeSubstr(text, start, end - start, buf);
> +                }
> +            }
> +
> +            return false;

Don't return false without reporting an error -- the options here are

*   MOZ_CRASH
*   report an InternalError, then `return false;`
*   JS_ASSERT(0), but for the benefit of non-debug builds, `break;` instead of `return false;`,
    leaving `start` and `end` set to default values.

This code is fiddly and someone else might come along and break it later by making changes in QuoteString; so MOZ_CRASH is maybe too harsh. Which of the other 2 to take is up to you.

@@ +1601,5 @@
> +    }
> +
> +    for (size_t i = start; i < end; i++)
> +        if (!buf.append(text[i]))
> +            return false;

style nit: Curly braces on the for-loop, please, since the loop body is >1 line

@@ +1608,5 @@
> +
> +JSAtom*
> +JSFunction::functionName(JSContext* cx) const {
> +    if (!atom_ || !hasGuessedAtom())
> +        return atom_;

Note: don't return a value here that can be confused with OOM

@@ +1610,5 @@
> +JSFunction::functionName(JSContext* cx) const {
> +    if (!atom_ || !hasGuessedAtom())
> +        return atom_;
> +
> +    JSFlatString* text = AtomToFlatString(atom_);

This variable isn't necessary - JSAtom is a subclass of JSFlatString, so you can use atom_ directly throughout.

@@ +1634,5 @@
> +            return nullptr;
> +    }
> +
> +    RootedAtom name(cx, buf.finishAtom());
> +    return name;

It's OK to just
    return buf.finishAtom();
and drop the RootedAtom here.

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: funcnames-parsing.diff

Landing after try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be4beba6863a

Comment 9

[Morgan Phillips [:mrrrgn]]

Attachment: funcnames-parsing.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b75f09fca2c&selectedJob=20552234

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f03cd0ff53df

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23a4649788f0

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8300ca8b7a

Comment 13

[Morgan Phillips [:mrrrgn]]

Attachment: funcnames-parsing.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb0b8bc2393&selectedJob=20577363

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e8300ca8b7a