788586 - Inferred names for anonymous JavaScript functions could be more accurate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

SpiderMonkey now infers names for anonymous functions, but in some cases those names are not as good as they could be.

- Although JJB's paper suggests it, naming Constructor.prototype.method "Constructor.method" is just not right - especially since people do make 'class methods' by putting functions directly on the constructor. "prototype" should not be dropped.

- We don't quite get names right for numeric indices in object initializers:
  a = { 1: function () {} };

- We generate bogus names when string literal property names in object initializers contain spaces or other things:
  a = {
    "embedded spaces": function(){},
    "dots.look.like.property.references": function(){},
    "\"\'quotes\'\"": function(){},
    "!@#$%": function(){}
  };

Comment 1

[Jim Blandy :jimb]

Attachment: Don't drop "prototype" from inferred display names.

Comment 2

[Jim Blandy :jimb]

Attachment: When inferring display names, handle numeric literals better.

Comment 3

[Jim Blandy :jimb]

Attachment: When inferring display names, handle property names that are not valid identifier names.

Comment 4

[u443197]

Comment on attachment 658538 [details] [diff] [review]
When inferring display names, handle numeric literals better.

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

::: js/src/frontend/NameFunctions.cpp
@@ +39,5 @@
> +        return buf->appendInflated(number, digits);
> +    }
> +
> +    /* Append a reference to a property named by a numeric literal to buf. */
> +    bool appendNumericPropertyReference(double n) {

I'd also be ok with this not being a function because it's only called in one place. It took me a second to figure out what's going on based on the name.

Comment 5

[u443197]

Comment on attachment 658541 [details] [diff] [review]
When inferring display names, handle property names that are not valid identifier names.

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

::: js/src/frontend/NameFunctions.cpp
@@ +47,5 @@
> +            return buf->append(".") && buf->append(name);
> +
> +        /* Quote the string as needed. */
> +        RootedValue string(cx, StringValue(name));
> +        JSString *source = js_ValueToSource(cx, string);

Could you just call js_QuoteString directly?

::: js/src/jit-test/tests/basic/functionnames.js
@@ +145,5 @@
> +
> +a.b = {};
> +a.b.c = {};
> +a.b["c"]["d e"] = { f: { 1: { "g": { "h i": function() {} } } } };
> +assertName(a.b.c["d e"].f[1].g["h i"], 'a.b.c["d e"].f[1].g["h i"]');

I like this! Definitely a good idea for these weird identifiers

Comment 6

[u443197]

(In reply to Alex Crichton [:acrichto] from comment #4)

> I'd also be ok with this not being a function because it's only called in
> one place. It took me a second to figure out what's going on based on the
> name.

Disregard this, with the appendPropertyReference function in the next patch, I'm perfectly OK with this.

Comment 7

[Jim Blandy :jimb]

(In reply to Alex Crichton [:acrichto] from comment #5)
> > +        /* Quote the string as needed. */
> > +        RootedValue string(cx, StringValue(name));
> > +        JSString *source = js_ValueToSource(cx, string);
> 
> Could you just call js_QuoteString directly?

I was going to do that, but then I thought it would be too simple and direct.

(fixed, thanks!)

Comment 8

[Jim Blandy :jimb]

(In reply to Alex Crichton [:acrichto] from comment #6)
> Disregard this, with the appendPropertyReference function in the next patch,
> I'm perfectly OK with this.

I admit to a bit of retroactive consistency in my patch queue...

Comment 9

[Jim Blandy :jimb]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6516f988fdee

Comment 10

[Jim Blandy :jimb]

Looks okay.
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ca4e7be55f
https://hg.mozilla.org/integration/mozilla-inbound/rev/789680f57083
https://hg.mozilla.org/integration/mozilla-inbound/rev/b064789c1758

Comment 11

[Jim Blandy :jimb]

For what it's worth, CoffeeScript abbreviates Foo.prototype.bar to Foo::bar.

Comment 12

[Nicholas Nethercote [inactive]]

I'd just stick with Foo.prototype.bar.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

FYI, b064789c1758 accidentally referred to bug 778586 instead of this one.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49ca4e7be55f
https://hg.mozilla.org/mozilla-central/rev/789680f57083
https://hg.mozilla.org/mozilla-central/rev/b064789c1758