1255925 - Give a name to getters/setters and integer-named methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S]

Attachment: WIP

Comment 2

[Tom S]

Attachment: Give a function name to getter/setters/methods.

This allows us to name methods in all cases excluding computed property names. As far as I can tell propAtom already contains something sensible in all cases that I could come up with. I had to change a bunch of functions to take HandleAtom instead of HandlePropertyName, because for {123(){}}, 123 is obviously not a property name ... I am not really happy having to call ConcatStrings inside the parser, but that seems at least better than somehow manually memcpying the atoms chars around.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8737921 [details] [diff] [review]
Give a function name to getter/setters/methods.

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

Looks good to me. The fact that they specced |Object.getOwnPropertyDescriptor({ get foo() { } }, "foo").get.name === "get foo"| is astonishing, but it does seem to be the case.

::: js/src/frontend/Parser.cpp
@@ +2000,5 @@
> +    RootedAtom prefix(context);
> +    if (propType == PropertyType::Setter || propType == PropertyType::SetterNoExpressionClosure)
> +        prefix = context->names().setPrefix;
> +    else
> +        prefix = context->names().getPrefix;

Please add a MOZ_ASSERT in here somewhere about propType.

@@ +2002,5 @@
> +        prefix = context->names().setPrefix;
> +    else
> +        prefix = context->names().getPrefix;
> +
> +    RootedString str(context, ConcatStrings<CanGC>(context, prefix, propAtom));

Would also love to assert somehow that propAtom is non-empty.

::: js/src/tests/js1_8_5/reflect-parse/classes.js
@@ +10,5 @@
> +            methodName = typeof id === 'string' ? ident(id) : null;
> +            break;
> +          case "get":
> +          case "set":
> +            methodName = ident(`${kind} ${typeof id === 'string' ? id : ""}`);

nice.

Comment 4

[Tom S]

Thanks for reviewing.

(In reply to Eric Faust [:efaust] from comment #3)
> Comment on attachment 8737921 [details] [diff] [review]
> @@ +2002,5 @@
> > +        prefix = context->names().setPrefix;
> > +    else
> > +        prefix = context->names().getPrefix;
> > +
> > +    RootedString str(context, ConcatStrings<CanGC>(context, prefix, propAtom));
> 
> Would also love to assert somehow that propAtom is non-empty.
> 
We can't do that: ({get ""() {}}) would be empty.

Comment 5

[Tom S]

Attachment: Change expected name in devtools test

Comment 6

[Tom S]

Because we set an explicit name for get/set now we don't get the inferred name from the analysis anymore.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5cf306560d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff56abc1768b

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a5cf306560d
https://hg.mozilla.org/mozilla-central/rev/ff56abc1768b

Comment 9

[Tom S]

I have to back this out. JSFunction::name returns a js::PropertyName, which is not ok anymore.

Comment 10

[Carsten Book [:Tomcat]]

backed out on request from Tom on m-c per irc

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/d91b00cc6ec6
https://hg.mozilla.org/mozilla-central/rev/10f66b316457

Comment 12

[Tom S]

Attachment: WIP Change JSFunction::name to return a JSAtom

This should fix that fuzzbug. I would appreciate to get some fuzzing for before I land this again.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8742053 [details] [diff] [review]
WIP Change JSFunction::name to return a JSAtom

I ran this quickly for a short time on Mac and it didn't seem to blow anything up.

Comment 14

[Tom S]

Attachment: Change JSFunction::name to return a JSAtom

Comment 15

[Tom S]

Attachment: Rollup for fuzzing

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

It is heavily preferred to list the m-c revision that the roll-up patch applies to, next time. In this case, it seems to apply to m-c rev 0891f0fa044c.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8743801 [details] [diff] [review]
Rollup for fuzzing

Nothing found specifically after few days' worth of fuzzing on a 64-bit debug deterministic js shell.

Comment 18

[Tom S]

Thanks for fuzzing! Eric feel free to review this now.

Comment 19

[Eric Faust [:efaust]]

Comment on attachment 8743504 [details] [diff] [review]
Change JSFunction::name to return a JSAtom

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

WFM.

::: js/src/frontend/Parser.cpp
@@ +941,5 @@
>  Parser<ParseHandler>::reportBadReturn(Node pn, ParseReportKind kind,
>                                        unsigned errnum, unsigned anonerrnum)
>  {
>      JSAutoByteString name;
> +    if (JSAtom* atom = pc->sc->asFunctionBox()->function()->name()) {

This is gonna warn without another set of parens, I think.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/310418e4db4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7e7e4f0b48
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f65fd17dc2

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/310418e4db4b
https://hg.mozilla.org/mozilla-central/rev/ac7e7e4f0b48
https://hg.mozilla.org/mozilla-central/rev/a8f65fd17dc2