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

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mrrrgn, Assigned: mrrrgn)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
This should cover basic cases like:

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

Updated

2 years ago
Blocks: 883377
(Assignee)

Updated

2 years ago
Assignee: nobody → winter2718
(Assignee)

Comment 1

2 years ago
Created attachment 8743613 [details] [diff] [review]
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.
Attachment #8743613 - Flags: review?(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).
Attachment #8743613 - Flags: review?(jorendorff)
(Assignee)

Comment 3

2 years ago
(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).
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
* having a little display name parser is ugly.
(Assignee)

Comment 6

2 years ago
Created attachment 8746369 [details] [diff] [review]
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
Attachment #8743613 - Attachment is obsolete: true
Attachment #8746369 - Flags: review?(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.
Attachment #8746369 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8749915 [details] [diff] [review]
funcnames-parsing.diff

Landing after try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be4beba6863a
Attachment #8746369 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8750370 [details] [diff] [review]
funcnames-parsing.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b75f09fca2c&selectedJob=20552234
Attachment #8749915 - Attachment is obsolete: true

Comment 10

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

Comment 11

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

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8300ca8b7a
(Assignee)

Comment 13

2 years ago
Created attachment 8750514 [details] [diff] [review]
funcnames-parsing.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb0b8bc2393&selectedJob=20577363
Attachment #8750370 - Attachment is obsolete: true

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e8300ca8b7a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1282332
See Also: → bug 1289749
Blocks: 1289749
See Also: bug 1289749
You need to log in before you can comment on or make changes to this bug.