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

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: anba, Assigned: efaust)

Tracking

({dev-doc-complete})

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
"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 ...^
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Comment 1

4 years ago
Posted patch Fix (obsolete) — Splinter Review
Fix is trivial. Just wait a little longer before complaining.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8547863 - Flags: review?(jwalden+bmo)
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.
Attachment #8547863 - Flags: review?(jwalden+bmo) → review-
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.
(Assignee)

Comment 4

4 years ago
Posted patch Fix 2Splinter Review
Now with tests and more Waldo-approved solution
Attachment #8547863 - Attachment is obsolete: true
Attachment #8547899 - Flags: review?(jwalden+bmo)
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).
Attachment #8547899 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/0b4f930f8ae9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.