Inferred names for anonymous JavaScript functions could be more accurate

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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(){}
  };
(Assignee)

Comment 1

6 years ago
Created attachment 658537 [details] [diff] [review]
Don't drop "prototype" from inferred display names.
Assignee: general → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 658538 [details] [diff] [review]
When inferring display names, handle numeric literals better.
(Assignee)

Comment 3

6 years ago
Created attachment 658541 [details] [diff] [review]
When inferring display names, handle property names that are not valid identifier names.
(Assignee)

Updated

6 years ago
Attachment #658537 - Flags: review?(alex)
(Assignee)

Updated

6 years ago
Attachment #658538 - Flags: review?(alex)
(Assignee)

Updated

6 years ago
Attachment #658541 - Flags: review?(alex)

Updated

6 years ago
Attachment #658537 - Flags: review?(alex) → review+

Comment 4

6 years ago
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.
Attachment #658538 - Flags: review?(alex) → review+

Comment 5

6 years ago
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
Attachment #658541 - Flags: review?(alex) → review+

Comment 6

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

Comment 7

6 years ago
(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!)
(Assignee)

Comment 8

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

Updated

6 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla18
(Assignee)

Comment 11

6 years ago
For what it's worth, CoffeeScript abbreviates Foo.prototype.bar to Foo::bar.
I'd just stick with Foo.prototype.bar.
FYI, b064789c1758 accidentally referred to bug 778586 instead of this one.
You need to log in before you can comment on or make changes to this bug.