Closed Bug 883377 Opened 7 years ago Closed 3 years ago

Implement ES6 function "name" property semantics

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bbenvie, Assigned: arai)

References

(Blocks 4 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 7 obsolete files)

5.80 KB, patch
jandem
: review+
Details | Diff | Splinter Review
12.32 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.29 KB, patch
smaug
: review+
Details | Diff | Splinter Review
68.65 KB, patch
anba
: review+
Details | Diff | Splinter Review
15.04 KB, patch
arai
: review+
Details | Diff | Splinter Review
8.01 KB, patch
miker
: review+
Details | Diff | Splinter Review
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.
Also:

> obj.property = function(){}; // "property"
Assignee: general → nobody
Test cases for method definitions bug 924672

> let obj = {
>   a(){},                 // "a"
>   1(){},                 // "1"
>   [Symbol.iterator](){}, // "[Symbol.iterator]"
>   *b(){},                // "b"
> }
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?
Flags: needinfo?(jorendorff)
Depends on: 1132630
Depends on: 1054599
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.
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, "");
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.
Flags: needinfo?(jorendorff) → needinfo?(mike)
Assignee: nobody → winter2718
Attached patch funcnames.diff (obsolete) — Splinter Review
This takes care of the low hanging fruit: simple assignments and methods names.
Attachment #8726329 - Flags: review?(jorendorff)
Attached patch funcnames.diff (obsolete) — Splinter Review
This one is a bit nicer.
Attachment #8726329 - Attachment is obsolete: true
Attachment #8726329 - Flags: review?(jorendorff)
Attachment #8726339 - Flags: review?(jorendorff)
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.
Attachment #8726339 - Flags: review?(jorendorff)
(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.
So I *think* parsing can work here come to think of it. Will post a patch with that approach. Thanks Jan !
Depends on: 1255925
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).
Attached patch funcnames-1of2.diff (obsolete) — Splinter Review
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.
Attachment #8726339 - Attachment is obsolete: true
Attachment #8741999 - Flags: review?(jorendorff)
Comment on attachment 8741999 [details] [diff] [review]
funcnames-1of2.diff

Actually, I need to add better comments.
Attachment #8741999 - Flags: review?(jorendorff)
Attached patch funcnames-parsing.diff (obsolete) — Splinter Review
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.
Attachment #8741999 - Attachment is obsolete: true
Attachment #8742216 - Flags: review?(jorendorff)
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.
Attachment #8742216 - Flags: review?(jorendorff)
Depends on: 1266255
Attachment #8742216 - Attachment is obsolete: true
Blocks: test262
Blocks: 1317405
Depends on: 1320042
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.
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
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
See Also: → 1320388
See Also: → 1320403
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
taking over.

will post patches after fixing depending bugs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2632a7acc8e03dd16d1b45df548c217e62cd80b0&selectedJob=31916820
Assignee: winter2718 → arai.unmht
Status: NEW → ASSIGNED
Depends on: 1320408, 1320388, 1320403
Duplicate of this bug: 1054599
actually bug 1320408 and bug 1320403 don't block, but might be nice to have.
No longer depends on: 1320403, 1320408
See Also: → 1320408
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.
Attachment #8814790 - Flags: review?(jdemooij)
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.
Attachment #8814791 - Flags: review?(jdemooij)
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.
Attachment #8814792 - Flags: review?(jdemooij)
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.
Attachment #8814793 - Flags: review?(jdemooij)
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
Also fixed existing tests for guessed atom in browser and dom.
Flags: needinfo?(mike)
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|?
Attachment #8814790 - Flags: review?(jdemooij)
Attachment #8814790 - Flags: review?(andrebargull)
Attachment #8814790 - Flags: review+
Attachment #8814791 - Flags: review?(jdemooij) → review+
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.
Attachment #8814792 - Flags: review?(jdemooij) → review+
Thank you for reviewing.
addressed review comments.
Attachment #8814790 - Attachment is obsolete: true
Attachment #8814790 - Flags: review?(andrebargull)
Attachment #8815122 - Flags: review?(andrebargull)
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.
Attachment #8814794 - Flags: review?(bgrinstead)
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.
Attachment #8814795 - Flags: review?(bugs)
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/
Attachment #8814793 - Flags: review?(jdemooij) → review+
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+.
Attachment #8814795 - Flags: review?(bugs) → review+
(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.
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.
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.
Attachment #8815122 - Flags: review?(andrebargull) → review+
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 ,
Attachment #8814794 - Flags: review?(bgrinstead) → review+
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
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
Attachment #8814794 - Attachment is obsolete: true
Attachment #8815608 - Flags: review?(bgrinstead)
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
Attachment #8815608 - Flags: review?(mratcliffe)
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.
Attachment #8815608 - Flags: review?(mratcliffe)
Attachment #8815608 - Flags: review?(bgrinstead)
Attachment #8815608 - Flags: review+
Blocks: 1321928
Blocks: 1321932
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
Can you file a new bug and also provide some more information?
Depends on: 1325651
See Also: → 1325651
See Also: 1325651
No longer depends on: 1325651
You need to log in before you can comment on or make changes to this bug.