Getter/setter syntax should work with computed property names

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
a = {get [expr]() { return 3; }, set[expr](v) { return 2; }} should work, and not throw a syntax error.
(Assignee)

Comment 1

5 years ago
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
Attachment #8470315 - Flags: review?(jorendorff)
Comment on attachment 8470315 [details] [diff] [review]
Patch to make getter/setter work with computed property names v1

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

Nice!

It might be fun to search existing tests for the use of std_iterator and switch them over to use the new syntax, where that's reasonable. Follow-up bug if so.

::: js/src/frontend/Parser.cpp
@@ +7221,5 @@
>  }
>  
>  template <typename ParseHandler>
> +bool
> +Parser<ParseHandler>::handleCompdPropName(Node literal, Node &propname)

Please rename this to computedPropertyName.

Also please change this method to return a Node, like all the other parsers (see objectLiteral(), for instance).

If it fails, it should
    return null();

@@ +7320,5 @@
>                  if (!propname)
>                      return null();
> +            } else if (tt == TOK_LB) {
> +                // Computed property name.
> +                if (!handleCompdPropName(literal, propname))

With the renaming, the comment becomes redundant; remove it.

::: js/src/frontend/Parser.h
@@ +618,5 @@
>      Node pushLexicalScope(Handle<StaticBlockObject*> blockObj, StmtInfoPC *stmt);
>      Node pushLetScope(Handle<StaticBlockObject*> blockObj, StmtInfoPC *stmt);
>      bool noteNameUse(HandlePropertyName name, Node pn);
>      Node objectLiteral();
> +    bool handleCompdPropName(Node literal, Node &propname);

computedPropertyName

::: js/src/tests/ecma_6/Class/compPropNames.js
@@ +204,5 @@
> +syntaxError("function f() { set [x](a): 1 }");
> +f1 = "abc";
> +syntaxError('a = {get [f1@](){}, set [f1](a){}}'); // unexpected symbol at end of AssignmentExpression
> +syntaxError('a = {get@ [f1](){}, set [f1](a){}}'); // unexpected symbol after get
> +syntaxError('a = {get [f1](){}, set@ [f1](a){}}'); // unexpected symbol after set

beautiful negative tests :)

@@ +214,5 @@
> +    a.hey = 5;
> +} catch (e) {
> +    if (e != 2)
> +        throw new Error('Expected 2 to be thrown on set');
> +}

assertThrowsValue(() => { a.hey = 5; }, 2);

The test code you wrote here wouldn't fail if a.hey = 5 silently passed without throwing.

@@ +226,5 @@
> +    a[expr] = 7;
> +} catch (e) {
> +    if (e != 4)
> +        throw new Error('Expected 4 to be thrown on set');
> +}

Please sanity-check that:

- expressions with side effects are called in the right order, for example

log = "";
obj = {
    "a": log += 'a',
    get [log += 'b']() {}
    [log += 'c']: log += 'd',
    set [log += 'e']() {}
};
assertEq(log, "abcde");

- assignment expressions are allowed in the brackets
- {} in the brackets is treated as an object literal
- ({[/x/.source]: 0}) works (the /x/ is recognized as a regexp)
Attachment #8470315 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

5 years ago
Addressed review comments.
Attachment #8470315 - Attachment is obsolete: true
Attachment #8472687 - Flags: review+
Guptha, is anything more needed for this to land, or can it be checked in?
Flags: needinfo?(gupta.rajagopal)
(Assignee)

Comment 5

5 years ago
Merged.

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

Till, I'm on vacation and may not be able to follow up on the try run very promptly.
Attachment #8472687 - Attachment is obsolete: true
Attachment #8477899 - Flags: review+
(In reply to guptha from comment #5)
> Till, I'm on vacation and may not be able to follow up on the try run very
> promptly.

Oh, sorry - I certainly didn't mean to interrupt your vacation. :(
(Assignee)

Comment 7

5 years ago
(In reply to Till Schneidereit [:till] from comment #6)
> Oh, sorry - I certainly didn't mean to interrupt your vacation. :(

No problem!
Flags: needinfo?(gupta.rajagopal)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb96975c7681
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
You need to log in before you can comment on or make changes to this bug.