guptha Assignee
	Description • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 2 • 10 years ago

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)

	guptha Assignee
	Comment 3 • 10 years ago

Addressed review comments.

	Till Schneidereit [:till]
	Comment 4 • 10 years ago

Guptha, is anything more needed for this to land, or can it be checked in?

	guptha Assignee
	Comment 5 • 10 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.

	Till Schneidereit [:till]
	Comment 6 • 10 years ago

(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. :(

	guptha Assignee
	Comment 7 • 10 years ago

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

No problem!

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb96975c7681

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/cb96975c7681

	Florian Scholz (Open Web Docs)
	Comment 10 • 10 years ago

Updated docs:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set
https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript

Reviews appreciated!