Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

Reference: http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property

In ES6, the "name" property of functions is smarter and also writable. Whenever an unnamed function (anonymous functions, arrow functions, etc.) is created it is given a name based on what it is directly assigned to. Accessors are given names that include "get " or "set " in the name.

> var varDecl = () => {};           // "varDecl"
> assignment = function(){};        // "assignment"
>
> let obj = {
>   prop: () => {},                 // "prop"
>   noOverride: function named(){}, // "named"
>   get getter(){},                 // "get getter"
>   set setter(v){},                // "set setter"
>   "literal": function(){},        // "literal"
>   5: () => {}                     // "5"
> };

There is also guidance for other new sources of functions in ES6 that aren't implemented in SpiderMonkey yet, such as classes and methods, that should be followed but aren't relevant until those things are implemented.

	Brandon Benvie [:benvie] Reporter
	Comment 1 • 11 years ago

Also:

> obj.property = function(){}; // "property"

	ziyunfei
	Comment 2 • 10 years ago

Test cases for method definitions bug 924672

> let obj = {
>   a(){},                 // "a"
>   1(){},                 // "1"
>   [Symbol.iterator](){}, // "[Symbol.iterator]"
>   *b(){},                // "b"
> }

	Mike Pennisi [:jugglinmike]
	Comment 3 • 10 years ago

I'm interested in implementing this, but this would be my first contribution to SpiderMonkey. I'm researching atoms, properties, bytecodes, etc. but I'm not sure I'll be able to figure this out on my own.

Jason: based on your comments in bug 924672, it sounds like you have a pretty good idea of how this should be done. Do you have the time to mentor me a bit? Or do you know someone who might?

	Jason Orendorff [:jorendorff]
	Comment 4 • 9 years ago

This made it into ES6 exactly as described by Brandon in comment 0.

9.1.2 SetFunctionName() specifies how this is supposed to behave at run time.
http://www.ecma-international.org/ecma-262/6.0/index.html#sec-setfunctionname

It's called during plain assignment (12.14.4), variable initialization (13.3.1.4 and 13.3.2.4), and when creating an object (12.2.6.9), and a few other cases SM doesn't implement yet. (Plus plain old function declarations, but we already implement that case correctly!)

Another corner case to check is:

    var n = 1;
    var obj = {["x" + n]: function () {}};  // 12.2.6.9 again
    assertEq(obj.x1.name, "x1");

Comment 1 is not correct in ES6: `obj.property = function(){}` does not trigger function naming, because first we're to check whether IsIdentifierRef(`obj.property`) is true (12.14.4 step 1.e.), and it turns out to be false (12.3.1.4). es6draft backs me up on this.

	Jason Orendorff [:jorendorff]
	Comment 5 • 9 years ago

Anonymous ES6 classes also get names:

    var x = class {};
    assertEq(x.name, "x");

So are generator functions and generator methods (plus other stuff listed in comment 0 and comment 2).

Parentheses around the right-hand-side do not block naming:

    var abc = ((((function () {}))));
    assertEq(abc.name, "abc");

Chained assignment works as you expect, if you understand how it's parsed:

    var a, b, c;
    a = b = c = function () {};
    assertEq(a.name, "c");

In all other syntactic contexts, even if the value on the right-hand side turns out to be an anonymous function, it still doesn't get named.

    q = (0, function(){});
    assertEq(q.name, "");

	Jason Orendorff [:jorendorff]
	Comment 6 • 9 years ago

I think we should take this in two phases.

First, fix what can be fixed by making changes only in js/src/frontend/NameFunctions.cpp. Currently that function only computes "display names" which are used by the Firefox devtools. It should also compute the .name property where possible.

Two, file bugs for the rest, presumably including classes and computed property names.

Mike, are you interested in giving it a shot? Not gonna lie, this is a pretty tough first bug, but I'll be happy to help.

The main weird thing you'll need to learn, in order to understand the code in NameFunctions.cpp, is how the parse tree looks. See ParseNode.h.

	Morgan Phillips [:mrrrgn]
	Comment 7 • 8 years ago

This takes care of the low hanging fruit: simple assignments and methods names.

	Morgan Phillips [:mrrrgn]
	Comment 8 • 8 years ago

This one is a bit nicer.

	Jason Orendorff [:jorendorff]
	Comment 9 • 8 years ago

Comment on attachment 8726339 [details] [diff] [review]
funcnames.diff

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

looks good, but removing r? as long as we're asking around about JSFunction::nameAtom_. We shouldn't grow JSFunction unless we absolutely have to.

	Jan de Mooij [:jandem]
	Comment 10 • 8 years ago

(Commenting since you asked about this on IRC.)

The "guessed" name is only used when the function does not have an explicit name, right? We could argue that functions with these new ES6 names should also not have a guessed display name. For instance, right now we have:

  var o = {foo() {}};           // name "foo", no guessed name
  var o = {foo: function() {}}; // no name, we guess

With .name fixed, I don't think it's unreasonable for the second case to behave like the first.

If we want to preserve the guessed name for the second case, maybe we can parse the guessed name to get the .name string? I'm not familiar with the syntax we use for it, but maybe we can do something there.

	Morgan Phillips [:mrrrgn]
	Comment 11 • 8 years ago

So I *think* parsing can work here come to think of it. Will post a patch with that approach. Thanks Jan !

	Morgan Phillips [:mrrrgn]
	Comment 12 • 8 years ago

I've got an approach where we're parsing the display name, then caching it by setting a property on the function object. This is hacky and bad. I'm going to just bargain with devtools folks for removing the display atom distinction altogether (as Jan suggested).

	Morgan Phillips [:mrrrgn]
	Comment 13 • 8 years ago

The first in two patches, here I'm covering cases where a display name doesn't contain a quoted string. In the second patch I'll add helper functions to deal with that circumstance.

	Morgan Phillips [:mrrrgn]
	Comment 14 • 8 years ago

Comment on attachment 8741999 [details] [diff] [review]
funcnames-1of2.diff

Actually, I need to add better comments.

	Morgan Phillips [:mrrrgn]
	Comment 15 • 8 years ago

The comments are not so awesome, I'll look at them again after I sleep. This isn't pretty, but it's better than wasting a lot of memory storing redundant information.

	Morgan Phillips [:mrrrgn]
	Comment 16 • 8 years ago

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

Not sure what I was thinking wrt unescaping '\xNNN' also I think this patch belongs on a dependent bug.

	Tooru Fujisawa [:arai] Assignee
	Comment 17 • 8 years ago

try is running.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=841b003e1674e475a38df336ebd7a219355998a6
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f92e963bebefb9fbb2dce6f71f0a6b72c4a3330

There are several more space for performance improvement tho...

	Tooru Fujisawa [:arai] Assignee
	Comment 18 • 8 years ago

the main issue is that even if we know the name, like "a=function() {}", we cannot set function's atom_ to it at compile time, since setting it will make the function named lambda.
if there's one more flag available for telling that the name is neither named lambda's one nor guessed name, we don't have to set the name dynamically.

	Tooru Fujisawa [:arai] Assignee
	Comment 19 • 8 years ago

benchmark result with the patch in try run above

Kraken
http://bnjbvr.github.io/simplegraph/#title=Kraken%20result%20comparison&ytitle=Values&categories=audio-fft%2Cstanford-crypto-pbkdf2%2Caudio-beat-detection%2Cstanford-crypto-ccm%2Cimaging-darkroom%2Cjson-parse-financial%2Caudio-oscillator%2Cai-astar%2Caudio-dft%2Cstanford-crypto-sha256-iterative%2Cjson-stringify-tinderbox%2Cimaging-gaussian-blur%2Cstanford-crypto-aes%2Cimaging-desaturate&values=before%2048.1%20101.7%2085.2%2088.5%2075.3%2034%2057.5%2066.3%20107.8%2041.3%2040.9%2064.8%2054.7%2053.1%0Aafter%2047.6%20101.6%2084.8%2087.9%2075%2033.1%2057.1%2066.1%20107.4%2041%2040.1%2064.4%2054.8%2052.6

SunSpider
http://bnjbvr.github.io/simplegraph/#title=SunSpider%20result%20comparison&ytitle=Values&categories=math-spectral-norm%2Cbitops-3bit-bits-in-byte%2Cdate-format-xparb%2C3d-morph%2Ccontrolflow-recursive%2Cstring-base64%2C3d-raytrace%2Caccess-nsieve%2Ccrypto-md5%2Cbitops-nsieve-bits%2Caccess-binary-trees%2Cbitops-bitwise-and%2Cstring-validate-input%2Caccess-fannkuch%2Cstring-fasta%2Cregexp-dna%2Cmath-partial-sums%2Caccess-nbody%2Cstring-tagcloud%2Cdate-format-tofte%2Cstring-unpack-code%2Ccrypto-aes%2Ccrypto-sha1%2Cbitops-bits-in-byte%2Cmath-cordic%2C3d-cube&values=before%201.8%201.1%2013.9%204.8%202.2%205.7%2011.4%202.7%203.9%203.6%202.4%202%207.7%205.6%206.6%2014.2%206.4%202.7%2021.4%2010.3%2036%2012.3%203.6%202.4%202.4%2012.5%0Aafter%201.9%201.1%2013.9%204.9%202.2%205.8%2011.1%202.7%204%203.6%202.4%202%207.8%205.6%206.5%2014%206.4%202.8%2021.3%2010.3%2035.8%2012.1%203.7%202.2%202.4%2012.1

Octane
http://bnjbvr.github.io/simplegraph/#title=Octane%20result%20comparison&ytitle=Values&categories=Richards%2CDeltaBlue%2CCrypto%2CRayTrace%2CEarleyBoyer%2CRegExp%2CSplay%2CSplayLatency%2CNavierStokes%2CPdfJS%2CMandreel%2CMandreelLatency%2CGameboy%2CCodeLoad%2CBox2D%2Czlib%2CTypescript%2CScore%20(version%209)&values=before%2034510%2064506%2029483%20100727%2028919%204188%2015242%2017122%2037678%2012411%2031388%2036133%2050260%2016161%2058630%2077287%2025921%2029636%0Aafter%2034181%2064460%2029584%2099085%2014345%204279%2014912%2017630%2038048%2012542%2031422%2035874%2048925%2015961%2057336%2077324%2026763%2028421

Octane EarleyBoyer hits critical regression.
I should fix it

	Tooru Fujisawa [:arai] Assignee
	Comment 20 • 8 years ago

So, currently the patch emits 2 opcodes (JSOP_STRING or JSOP_DUP, and JSOP_SETFUNNAME) for each anonymous function/class that needs SetFunctionName call.
We could optimize that out for the following case:
  * if the string is known, we could set it to JSFunction.atom_
    (that needs flags_ bit to avoid treating the function namedLambda, I'm trying to move existing flags to other places)
  * if class has "name" method/accessor, we don't need to do SetFunctionName

	Tooru Fujisawa [:arai] Assignee
	Comment 21 • 8 years ago

the performance regression in Octane EarleyBoyer disappears by setting function name at compile time.
try is running:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2ca06d270eb4f68db9392af1df131393dd0833

	Tooru Fujisawa [:arai] Assignee
	Comment 22 • 8 years ago

taking over.

will post patches after fixing depending bugs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2632a7acc8e03dd16d1b45df548c217e62cd80b0&selectedJob=31916820

	Tooru Fujisawa [:arai] Assignee
	Comment 24 • 8 years ago

actually bug 1320408 and bug 1320403 don't block, but might be nice to have.

	Tooru Fujisawa [:arai] Assignee
	Comment 25 • 8 years ago

these patches are based on bug 1320408 (parts 1-3), bug 1320388, bug 1320403 (optional), and bug 1320042.

There are 2 parts that changed:

  * SetFunctionName at Compile Time
    * Set JSFunction.atom_ to statically known name
      and set JSFunction flag for it
  * SetFunctionName at Runtime
    * emit JSOP_SETFUNNAME that does SetFunctionName,
      after checking 'name' own property if necessary

Both are handled in BytecodeEmitter.
While emitting property, assignment, or initializer that needs SetFunctionName,
BytecodeEmitter first tries to statically set the function name, for the following case:
  * name is known string (instead of computed property)
  * it's setting to anonymous function (instead of class constructor)
If above conditions match, BytecodeEmitter calls JSFunction::setCompileTimeName.

JSFunction::setCompileTimeName sets JSFunction.atom_ and add HAS_COMPILE_NAME flag (I was about to use HAS_COMPILE_TIME_NAME name, but it's long :P

If above conditions don't match, BytecodeEmitter emits JSOP_STRING for function name (or JSOP_DUPAT to get property key), and JSOP_SETFUNNAME.
JSOP_SETFUNNAME calls SetFunctionNameIfNoOwnName function, that does SetFunctionName after checking 'name' own property for class constructor.  SetFunctionNameIfNoOwnName defines function's 'name' property by NativeDefineProperty.

Function name set by JSFunction::setCompileTimeName is not used for following case:
  * JSFunction::isNamedLambda
  * toString result
  * everything else that needs named function's name
So, added JSFunction::nameOrCompileTimeName(), that is used while resolving 'name' property.

Also, changed IdToFunctionName to receive FunctionPrefixKind enum instead of const char*, so that we don't have to append white space separately.

JIT support for JSOP_SETFUNNAME is added in Part 3.

	Tooru Fujisawa [:arai] Assignee
	Comment 26 • 8 years ago

Since setting compile time name clears function's guesses atom, we can reorder emitting and NameFunctions call.
This patch moves NameFunctions call after emitting, so we don't need to calculate guessed atom for already named function.

	Tooru Fujisawa [:arai] Assignee
	Comment 27 • 8 years ago

Added Baseline and Ion support for JSOP_SETFUNNAME, both just calls js::SetFunctionNameIfNoOwnName.
Currently this opcode is used only for anonymous class, that won't appear so much, so I believe this doesn't harm performance for most case.

As noted in comment #20, we could optimize away JSOP_SETFUNNAME for class if the class doesn't have static "name" method/accessor, but I'd like to leave it to other bug for now.

	Tooru Fujisawa [:arai] Assignee
	Comment 28 • 8 years ago

Some tests are relying on current guessed atom behavior.
Since this patch changes guessed atom for functions with HAS_COMPILE_NAME and any functions inside it, fixed tests to follow the change.

	Tooru Fujisawa [:arai] Assignee
	Comment 29 • 8 years ago

Devtools' devtools/server/event-parsers.js relies on current guessed atom to detect jQuery handler.
Added new pattern for updated guessed atom, and also fixed existing tests for guessed atom.

I'll ask review after Part 1-4

	Tooru Fujisawa [:arai] Assignee
	Comment 30 • 8 years ago

Also fixed existing tests for guessed atom in browser and dom.

	Jan de Mooij [:jandem]
	Comment 31 • 8 years ago

Comment on attachment 8814790 [details] [diff] [review]
Part 1: Implement ES6 function name property semantics.

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

LGTM with comments below addressed, but I'm adding an additional review from anba as he's more familiar with the spec.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +71,5 @@
> +    if (pn->isKind(PNK_FUNCTION) && !pn->pn_funbox->function()->name())
> +        return true;
> +
> +    // 14.5.8 (ClassExpression)
> +    if (pn->isKind(PNK_CLASS) && !pn->pn_kid1)

Please use ClassNode here, something like:

  if (pn->is<ClassNode>() && !pn->as<ClassNode>().names())

@@ +4539,5 @@
> +                                     FunctionPrefixKind prefixKind)
> +{
> +    if (maybeFun->isKind(PNK_FUNCTION)) {
> +        // Function doesn't have 'name' property at this point.
> +        // Set function's name compile time.

Nit: at compile time.

@@ +4543,5 @@
> +        // Set function's name compile time.
> +        RootedFunction fun(cx, maybeFun->pn_funbox->function());
> +
> +        // Single node can be emitted multiple times if it appears in
> +        // array destructuring default.  If function already has name,

Nit: has a name

::: js/src/frontend/BytecodeEmitter.h
@@ +675,5 @@
>      // onto the stack.
>      MOZ_MUST_USE bool emitIteratorNext(ParseNode* pn, bool allowSelfHosted = false);
>  
>      // Check if the value on top of the stack is "undefined". If so, replace
>      // that value on the stack with the value defined by |defaultExpr|.

Update this comment to explain what |pattern| is for.

::: js/src/jsfun.cpp
@@ +2128,5 @@
> +}
> +
> +JSAtom*
> +js::NameToFunctionName(ExclusiveContext* cx, HandleAtom name,
> +                     FunctionPrefixKind prefixKind /* = FunctionPrefixKind::None */)

Nit: fix indentation

@@ +2165,5 @@
> +    } else {
> +#ifdef DEBUG
> +        // Anonymous function shouldn't have own 'name' property at this point.
> +        Shape* unused;
> +        MOZ_ASSERT(!LookupOwnPropertyPure(cx, fun, NameToId(cx->names().name), &unused));

I think this returns false because functions have a resolve hook that resolves |name|, not because the function doesn't have the property. Change it to:

  MOZ_ASSERT(!fun->containsPure(cx->names().name));

::: js/src/jsfun.h
@@ +66,5 @@
>                                         Function() call (not a FunctionDeclaration or nonstandard
>                                         function-statement) */
>          SELF_HOSTED      = 0x0080,  /* function is self-hosted builtin and must not be
>                                         decompilable nor constructible. */
> +        HAS_COMPILE_NAME = 0x0100,  /* function had no explicit name, but a

Nit: I think I'd prefer HAS_COMPILE_TIME_NAME, for symmetry with hasCompileTimeName below. It breaks the "alignment", but I can live with that :)

@@ +317,5 @@
>                                      js::MutableHandleValue v);
>  
>      JSAtom* getUnresolvedName(JSContext* cx);
>  
> +    JSAtom* name() const {

I think it would be nice to rename this to |explicitName()| or something. It makes it more obvious why we return nullptr if hasCompileTimeName() or hasGuessedAtom().

@@ +320,5 @@
>  
> +    JSAtom* name() const {
> +        return (hasCompileTimeName() || hasGuessedAtom()) ? nullptr : atom_.get();
> +    }
> +    JSAtom* nameOrCompileTimeName() const {

And this to |explicitOrCompileTimeName|?

	Jan de Mooij [:jandem]
	Comment 32 • 8 years ago

Comment on attachment 8814792 [details] [diff] [review]
Part 3: Support JSOP_SETFUNNAME in Baseline and Ion.

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

::: js/src/jit/MIR.h
@@ +8488,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(SetFunName)
> +    TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, getFun), (1, getName))

Nit: slight preference for just "fun" and "name" without the "get".

::: js/src/jsfun.cpp
@@ +2150,5 @@
>  bool
>  js::SetFunctionNameIfNoOwnName(JSContext* cx, HandleFunction fun, HandleValue name,
>                                 FunctionPrefixKind prefixKind)
>  {
> +    MOZ_ASSERT(name.isString() || name.isSymbol() || name.isNumber());

Nice, I was wondering whether isInt32 should be isNumber when I first looked at the other patch, but forgot to comment.

	Tooru Fujisawa [:arai] Assignee
	Comment 33 • 8 years ago

Thank you for reviewing.
addressed review comments.

	Tooru Fujisawa [:arai] Assignee
	Comment 35 • 8 years ago

Comment on attachment 8814794 [details] [diff] [review]
Part 5: Update devtools to follow displayName change.

Devtools' devtools/server/event-parsers.js relies on current guessed atom to detect jQuery handler (.event.proxy/ and .proxy/).
Added new pattern (proxy/) for updated guessed atom, and also fixed existing tests for guessed atom.

	Tooru Fujisawa [:arai] Assignee
	Comment 36 • 8 years ago

Comment on attachment 8814795 [details] [diff] [review]
Part 6: Update browser and dom tests to follow displayName change.

fixed existing tests that relies on current guessed atom behavior, in browser and dom.

	Jan de Mooij [:jandem]
	Comment 37 • 8 years ago

Comment on attachment 8814793 [details] [diff] [review]
Part 4: Fix existing tests.

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

::: js/src/jit-test/tests/saved-stacks/function-display-name.js
@@ +3,5 @@
>  function uno() { return dos(); }
>  const dos = () => tres.quattro();
>  const tres = {
>    quattro: () => saveStack()
>  };

Maybe change this to:

  let tres = {};
  tres.quattro = () => saveStack();

To keep testing the guessed-atom behavior?

::: js/src/tests/ecma_6/Object/accessor-name.js
@@ -26,5 @@
>  o = {get case() { }, set case(v) {}}
>  assertEq(name(o, "case", true), "get case");
>  assertEq(name(o, "case", false), "set case");
>  
> -// Congratulations on implementing these!

\o/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 38 • 8 years ago

Comment on attachment 8814795 [details] [diff] [review]
Part 6: Update browser and dom tests to follow displayName change.

What do other browsers do with cases like test_promise_rejections_from_jsimplemented.html ?

I assume this bug isn't making us incompatible with others. If that is true, r+.

	Tooru Fujisawa [:arai] Assignee
	Comment 39 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #38)
> Comment on attachment 8814795 [details] [diff] [review]
> Part 6: Update browser and dom tests to follow displayName change.
> 
> What do other browsers do with cases like
> test_promise_rejections_from_jsimplemented.html ?

With following test:

  function doTest() {
    Promise.all([{
      then: function() { thereIsNoSuchContentFunction3(); }
    }]).catch(
      e=>console.log("stack\n" + e.stack)
    );
  }
  doTest();

Chrome says:
  stack
  a.html:17 ReferenceError: thereIsNoSuchContentFunction3 is not defined
      at Object.then (a.html:15)

and Safari says:
  stack
  then@file:///Users/arai/Desktop/a.html:15:55
  promiseResolveThenableJob@[native code]

So, current Firefox's behavior doesn't match either,
and updated one may match Safari.

	Tooru Fujisawa [:arai] Assignee
	Comment 40 • 8 years ago

Comment on attachment 8814794 [details] [diff] [review]
Part 5: Update devtools to follow displayName change.

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

::: devtools/server/event-parsers.js
@@ +92,5 @@
>      },
>      normalizeHandler: function (handlerDO) {
>        let paths = [
>          [".event.proxy/", ".event.proxy/", "*"],
>          [".proxy/", "*"]

forgot to add trailing "," while rebasing.

	André Bargull [:anba]
	Comment 41 • 8 years ago

Comment on attachment 8815122 [details] [diff] [review]
Part 1: Implement ES6 function name property semantics. r=jandem

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

Looks good, hopefully the constant folding issue doesn't lead to further complications!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4581,5 @@
> +{
> +    if (!emitTree(initializer))
> +        return false;
> +
> +    if (pattern->isKind(PNK_NAME) && IsAnonymousFunctionDefinition(initializer)) {

This needs to check that |pattern| isn't parenthesized.

|PrimaryExpression : CoverParenthesizedExpressionAndArrowParameterList| returns false in https://tc39.github.io/ecma262/#sec-semantics-static-semantics-isidentifierref .

Test case:
---
var a;
(a) = function(){};
assertEq(a.name, "");
---

@@ +5247,5 @@
>              // the top of the stack and we need to pick the right RHS value.
>              if (!EmitAssignmentRhs(bce, rhs, emittedBindOp ? 2 : 1))
>                  return false;
>  
> +            if (op == JSOP_NOP && rhs && IsAnonymousFunctionDefinition(rhs)) {

IsAnonymousFunctionDefinition() is called too late, FoldConstants already updated the parse tree. 

Test case:
---
var a = true && function(){};
assertEq(a.name, "");
---

::: js/src/frontend/BytecodeEmitter.h
@@ +676,5 @@
>      MOZ_MUST_USE bool emitIteratorNext(ParseNode* pn, bool allowSelfHosted = false);
>  
>      // Check if the value on top of the stack is "undefined". If so, replace
>      // that value on the stack with the value defined by |defaultExpr|.
> +    // |pattern| is a lhs node of the default expression.  If it's identifier

s/identifier/an identifier/

@@ +677,5 @@
>  
>      // Check if the value on top of the stack is "undefined". If so, replace
>      // that value on the stack with the value defined by |defaultExpr|.
> +    // |pattern| is a lhs node of the default expression.  If it's identifier
> +    // and |defaultExpr| is anonymous function, |SetFunctionName| is done at

s/anonymous function/an anonymous function/
s/is done at/is called at/

::: js/src/jsfun.cpp
@@ +1308,5 @@
>  
>      if (isClassConstructor()) {
>          // It's impossible to have an empty named class expression. We use
>          // empty as a sentinel when creating default class constructors.
> +        MOZ_ASSERT(explicitOrCompileTimeName() != cx->names().empty);

I was so confused why this assertion isn't triggered for `({"": class {}})` until I realized that SetFunctionName is never called for class constructors. Can we add an assertion that HAS_COMPILE_TIME_NAME is never set for class constructors?

@@ +2149,5 @@
> +bool
> +js::SetFunctionNameIfNoOwnName(JSContext* cx, HandleFunction fun, HandleValue name,
> +                               FunctionPrefixKind prefixKind)
> +{
> +    MOZ_ASSERT(name.isString() || name.isSymbol() || name.isInt32());

`({0.4: function(){}})` asserts at this line (*), can we add a test case for double numbers as property keys? 

(*) I already spotted that this gets fixed in part 3.

@@ +2174,5 @@
> +    if (!funNameAtom)
> +        return false;
> +
> +    RootedValue funNameVal(cx, StringValue(funNameAtom));
> +    if (!NativeDefineProperty(cx, fun, cx->names().name, funNameVal, nullptr, nullptr,

Do you think it makes sense to avoid calling the resolve hook here? Or is that too hacky?

::: js/src/jsfun.h
@@ +65,5 @@
>                                         function-statement) */
>          SELF_HOSTED      = 0x0080,  /* function is self-hosted builtin and must not be
>                                         decompilable nor constructible. */
> +        HAS_COMPILE_TIME_NAME = 0x0100, /* function had no explicit name, but a
> +                                           name was set by SetFunctionName */

I wonder if "HAS_INFERRED_NAME" or "HAS_IMPLICIT_NAME" (as the counterpart to the explicitName() method) is more easy to grasp for future readers. WDYT?

::: js/src/tests/ecma_6/Function/function-name-assignment.js
@@ +105,5 @@
> +    eval(`
> +    let lexical = ${expr};
> +    assertEq(lexical.name, named ? "named" : "lexical");
> +
> +    let constLexical = ${expr};

typo: const

::: js/src/tests/ecma_6/Function/function-name-property.js
@@ +24,5 @@
> +    var obj = eval(`({
> +        prop: ${expr},
> +        "literal": ${expr},
> +        "": ${expr},
> +        5: ${expr},

Also add a test with non-integer numbers.

@@ +29,5 @@
> +        [Symbol.iterator]: ${expr},
> +        [fooSymbol]: ${expr},
> +        [emptySymbol]: ${expr},
> +        [undefSymbol]: ${expr},
> +        [/a/]: ${expr},

We should also test __proto__ to ensure that |({__proto__: function(){}})| doesn't assign "__proto__" as the inferred for an anonymous function.

	Brian Grinstead [:bgrins]
	Comment 42 • 8 years ago

Comment on attachment 8814794 [details] [diff] [review]
Part 5: Update devtools to follow displayName change.

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

Looks fine to me with the trailing ,

	Tooru Fujisawa [:arai] Assignee
	Comment 43 • 8 years ago

Thank you for reviewing :D

(In reply to André Bargull from comment #41)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4581,5 @@
> > +{
> > +    if (!emitTree(initializer))
> > +        return false;
> > +
> > +    if (pattern->isKind(PNK_NAME) && IsAnonymousFunctionDefinition(initializer)) {
> 
> This needs to check that |pattern| isn't parenthesized.
> 
> |PrimaryExpression : CoverParenthesizedExpressionAndArrowParameterList|
> returns false in
> https://tc39.github.io/ecma262/#sec-semantics-static-semantics-
> isidentifierref .
> 
> Test case:
> ---
> var a;
> (a) = function(){};
> assertEq(a.name, "");
> ---

Added |node->isInParens()| check.


> @@ +5247,5 @@
> >              // the top of the stack and we need to pick the right RHS value.
> >              if (!EmitAssignmentRhs(bce, rhs, emittedBindOp ? 2 : 1))
> >                  return false;
> >  
> > +            if (op == JSOP_NOP && rhs && IsAnonymousFunctionDefinition(rhs)) {
> 
> IsAnonymousFunctionDefinition() is called too late, FoldConstants already
> updated the parse tree. 
> 
> Test case:
> ---
> var a = true && function(){};
> assertEq(a.name, "");
> ---

Moved the timing to Parser.  Now it checks while creating assignment/initializer/property, and the result is stored on |ParseNode.pn_rhs_anon_fun| via |ParseNode::setDirectRHSAnonFunction|.
BytecodeEmitter checks |node->isDirectRHSAnonFunction()| instead of IsAnonymousFunctionDefinition.


> @@ +2174,5 @@
> > +    if (!funNameAtom)
> > +        return false;
> > +
> > +    RootedValue funNameVal(cx, StringValue(funNameAtom));
> > +    if (!NativeDefineProperty(cx, fun, cx->names().name, funNameVal, nullptr, nullptr,
> 
> Do you think it makes sense to avoid calling the resolve hook here? Or is
> that too hacky?

Yeah, we could skip resolve hook, but I'd like to leave it to other bug ;)


> ::: js/src/jsfun.h
> @@ +65,5 @@
> >                                         function-statement) */
> >          SELF_HOSTED      = 0x0080,  /* function is self-hosted builtin and must not be
> >                                         decompilable nor constructible. */
> > +        HAS_COMPILE_TIME_NAME = 0x0100, /* function had no explicit name, but a
> > +                                           name was set by SetFunctionName */
> 
> I wonder if "HAS_INFERRED_NAME" or "HAS_IMPLICIT_NAME" (as the counterpart
> to the explicitName() method) is more easy to grasp for future readers. WDYT?

the name should say that the function name is set at compile time, because SetFunctionName on runtime doesn't touch the flag, and also doesn't touch JSFunction.atom_.
'IMPLICIT' won't fit since it's only partially counterpart to explicitName.
maybe we could call it HAS_COMPILE_TIME_INFERRED_NAME or something tho... :P

	Tooru Fujisawa [:arai] Assignee
	Comment 44 • 8 years ago

Sorry, the previous patch for event-parsers.js broke the functionality and unexpectedly passed the tests.  also the code was wrong even if I fixed trailing ",", in jQuery 1.4's case:

> proxy: function(a, b, d) {
>   if (arguments.length === 2)
>     if (typeof b ===
>       "string") {
>       d = a;
>       a = d[b];
>       b = w
>     } else if (b && !c.isFunction(b)) {
>     d = b;
>     b = w
>   }
>   if (!b && a) b = function() {
>     return a.apply(d || this, arguments)
>   };
>   if (a) b.guid = a.guid = a.guid || b.guid || c.guid++;
>   return b
> },

|proxy| function returns function |b|, that gets named "b" by SetFunctionName, and its displayName is also "b".  So checking the function by "proxy/*" name doesn't work.


In attached patch, i've changed |normalizeListener| to do following:
  a) check if the function's displayName starts with "proxy/"
  b) check if the function's environment is "proxy" function's one

(a) corresponds to jQuery 1.3's style, that is:

> proxy: function(E, D) {
>   D = D || function() {
>     return E.apply(this, arguments)
>   };
>   D.guid = E.guid = E.guid || D.guid || this.guid++;
>   return D
> }

now |proxy| function always gets "proxy" name, so the function pointed by |D| gets "proxy/D<".


(b) corresponds to jQuery 1.4 or later's style:

> proxy: function(a, b, d) {
> ...
>   if (!b && a) b = function() {
>     return a.apply(d || this, arguments)
>   };
> ...
>   return b
> },

the function pointed by |b| gets "b" name, so we cannot check if the function |b| is inside |proxy| by displayName.
So, instead of displayName, now the patch checks if |handlerDO.environment.callee.displayName| is "proxy".


I added 2 function to |normalizeListener| to do above.
  * |isFunctionInProxy| does the above 2 kinds of check.
  * |getFirstFunctionVariable| returns first function variable in the environment, that is the same thing as unpatched code does.


Also, apparently unpatched |normalizeListener| supports nested handler, by repeating ".event.proxy/":
>        [".event.proxy/", ".event.proxy/", "*"],

So the code in this patch also supports it by for-loop, with MAX_NESTED_HANDLER_COUNT (currently it's 2).
The loop continues searching unwrapped function if the function is still inside |proxy|.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a3ebe99a19b9836a2921e7102ae295f563bc65&group_state=expanded

	Brian Grinstead [:bgrins]
	Comment 45 • 8 years ago

Comment on attachment 8815608 [details] [diff] [review]
Part 5: Update devtools to follow displayName change.

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

Going to see if Mike can take a look at this, since he knows the event parser code

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 46 • 8 years ago

Comment on attachment 8815608 [details] [diff] [review]
Part 5: Update devtools to follow displayName change.

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

Looks good to me.

	Tooru Fujisawa [:arai] Assignee
	Comment 47 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0faba97ebe86d2e32c6afee3f0486cb5b538d74f
Bug 883377 - Part 1: Implement ES6 function name property semantics. r=jandem,anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/c94153a390fa65444bf578aca5e3a562a170ebce
Bug 883377 - Part 2: Call NameFunctions after emitting. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85f460926f103fec2c319e5524c0c03c853b534
Bug 883377 - Part 3: Support JSOP_SETFUNNAME in Baseline and Ion. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/67447b2f92944a6a1eaaa06026c9f1d670a8946c
Bug 883377 - Part 4: Fix existing tests. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/a182612850f93496d6e1a4d753fd0e61fc1581c8
Bug 883377 - Part 5: Update devtools to follow displayName change. r=miker

https://hg.mozilla.org/integration/mozilla-inbound/rev/deb743b033b70f97af83172d90008da2a5ce7c59
Bug 883377 - Part 6: Update browser and dom tests to follow displayName change. r=smaug

	Phil Ringnalda (:philor)
	Comment 48 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0faba97ebe86
https://hg.mozilla.org/mozilla-central/rev/c94153a390fa
https://hg.mozilla.org/mozilla-central/rev/d85f460926f1
https://hg.mozilla.org/mozilla-central/rev/67447b2f9294
https://hg.mozilla.org/mozilla-central/rev/a182612850f9
https://hg.mozilla.org/mozilla-central/rev/deb743b033b7

	Gary [:streetwolf52]
	Comment 49 • 7 years ago

I am having problems in safe-mode.  I can't get to most sites and about:support isn't filed in with the normal stuff.

Here's the regression range I came up with.

Bad
https://hg.mozilla.org/integration/mozilla-inbound/rev/deb743b033b70f97af83172d90008da2a5ce7c59
This includes Bug 883377

Good
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f494ff3b83be2b5e8529e859d80ca3efacf19f7

	Tooru Fujisawa [:arai] Assignee
	Comment 50 • 7 years ago

Can you file a new bug and also provide some more information?

	Greg Mierzwinski [:sparky]
	Comment 51 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db6142563af

	Florian Scholz (Open Web Docs)
	Comment 52 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name