1073809 - Allow "get" and "set" as names for generator methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

"get" and "set" are valid names for generator methods.

Expected: No SynaxError
Actual: SyntaxError thrown


js> ({*get(){}})
typein:1:3 SyntaxError: invalid property id:
typein:1:3 ({*get(){}})
typein:1:3 ...^


js> ({*set(){}}) 
typein:2:3 SyntaxError: invalid property id:
typein:2:3 ({*set(){}})
typein:2:3 ...^

Comment 1

[Eric Faust [:efaust]]

Attachment: Fix

Fix is trivial. Just wait a little longer before complaining.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8547863 [details] [diff] [review]
Fix

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

::: js/src/frontend/Parser.cpp
@@ +7969,5 @@
>              }
>  
>              MOZ_ASSERT(op == JSOP_INITPROP_GETTER || op == JSOP_INITPROP_SETTER);
> +            // Wait until we are sure we think it's an accessor to complain
> +            // about being a generator

So this works, but it "overparses" beyond the tokens it should parse, before ungetting and continuing onward.  What if instead we replaced the if-else if-else modified above like so:

            if (!isGenerator && (atom == context->names().get || atom == context->names().set)) {
                op = (atom == context->names().get)
                     ? JSOP_INITPROP_GETTER
                     : JSOP_INITPROP_SETTER;
            } else {
                propname = handler.newIdentifier(atom, pos());
                if (!propname)
                    return null();
                break;
            }

Then we don't need this hunk down here, we don't have to wonder how all the code between here and there handles this (and whether it does so correctly), and things are generally much nicer.  +4,-12 versus +6,-8.

Comment 3

[Jeff Walden [:Waldo]]

Oh, and you don't have a test in the patch.  Need to see those to ensure we cover all the bases here, in code and test-wise too.

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix 2

Now with tests and more Waldo-approved solution

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8547899 [details] [diff] [review]
Fix 2

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

::: js/src/frontend/Parser.cpp
@@ +7915,5 @@
> +                (atom == context->names().get ||
> +                 atom == context->names().set))
> +            {
> +                op = atom == context->names().get ? JSOP_INITPROP_GETTER
> +                                                  : JSOP_INITPROP_SETTER;

I think lining up ? and : underneath the "a" in atom is more common, tho it's not quite codified in the style rules.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +423,5 @@
> +                                        value: genFunExpr(ident("get"), [], blockStmt([]))}]));
> +assertExpr("({*set() { }})", objExpr([{ type: "Property", key: ident("set"), method: true,
> +                                        value: genFunExpr(ident("set"), [], blockStmt([]))}]));
> +assertError("({*get foo() { }})", SyntaxError);
> +assertError("({*set foo() { }})", SyntaxError);

Add more tests for other failure modes, each duplicated for get and set:

({ *get 1() {} });
({ *get ""() {} });
({ *get foo() {} });
({ *get []() {} });
({ *get [1]() {} });

Also modify jit-test/tests/basic/syntax-error-illegal-character.js in the "object: method" section to have test("({ * get @"); test("({ * get ( @"); test("({ * get () @"); test("({ * get () { @"); test("({ * get () {} @"); test("({ * get () {}, @"); in it (and the same for set, as well).

Comment 6

[Eric Faust [:efaust]]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b4f930f8ae9

Comment 8

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#SpiderMonkey-specific_notes