924688 - Implement ES6 computed property names

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brandon Benvie [:benvie]]

ES6 introduces computed property names in ObjectLiterals (and ClassDefinitions). The ToPropertyKey is called on the AssignmentExpression inside the computed property name, which means any non-Symbol will be coerced to a string. Examples:

> var i = 0;
> var obj = {
>   ["foo" + ++i]: i,
>   ["foo" + ++i]: i,
>   ["foo" + ++i]: 1
> };

Would result in obj being:

> ({ foo1: 1, foo2: 2, foo3: 3 })


The grammar for PropertyName is updated to be

> PropertyName :
>   LiteralPropertyName
>   ComputedPropertyName
>
> LiteralPropertyName :
>   IdentifierName
>   StringLiteral
>   NumericLiteral
>
> ComputedPropertyName :
>   [ AssignmentExpression ]

See ES6 draft spec (September 2013 edition) section 12.1.5.

Comment 1

[Brandon Benvie [:benvie]]

This also applies to destructuring. Example:

> let key = "z";
> let { [key]: foo } = { z: "bar" };
> foo; // "bar"

When using a computed name in destructuring it has to be given an alias, so as to not introduce eval-like dynamic bindings.

Comment 2

[guptha]

Attachment: Patch to implement computed property names v0

1. This probably needs more tests.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8462937 [details] [diff] [review]
Patch to implement computed property names v0

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

Great patch! r=me with these comments addressed, including the new tests.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3276,5 @@
>                      doElemOp = false;
>                  }
> +            } else {
> +                // Has to be a computed property name.
> +                JS_ASSERT(key->isKind(PNK_COMPUTED_NAME));

Remove the redundant comment, please.

@@ +6072,5 @@
>                  isIndex = true;
>              }
> +        } else {
> +            // Has to be a computed property name.
> +            JS_ASSERT(pn3->isKind(PNK_COMPUTED_NAME));

And here.

::: js/src/frontend/ParseNode.h
@@ +393,5 @@
>   *                          destructuring lhs
>   *                          pn_left: property id, pn_right: value
>   * PNK_SHORTHAND binary     Same fields as PNK_COLON. This is used for object
>   *                          literal properties using shorthand ({x}).
> + * PNK_COMPUTED_NAME unary  pn_kid: assignment expr

I'd like these to be clearer. How about:

    PNK_COMPUTED_NAME unary  ES6 ComputedPropertyName.
                             pn_kid: the AssignmentExpression inside the square brackets

::: js/src/frontend/Parser.cpp
@@ +7042,5 @@
>  typename ParseHandler::Node
> +Parser<ParseHandler>::newComputedName(Node name)
> +{
> +    return handler.newComputedName(name, pos().begin);
> +}

Please remove this method.

@@ +7279,5 @@
>              break;
>  
> +          case TOK_LB: {
> +              // Computed property name.
> +              propname = newComputedName(assignExpr());

A few bugs here:

1. The result of assignExpr() must be checked for errors.

2. propname must be checked for errors.

3. This assigns the wrong start position to the new PNK_COMPUTED_NAME node, because it calls pos().begin after calling assignExpr().

4. This assigns the wrong end position to the PNK_COMPUTED_NAME node, because it doesn't call pos() after consuming the closing bracket. Please make ParseHandler::newComputedName() take both a Node and a TokenPos argument, and call it only after getting all three pieces of information (kid, begin, and end).

5. If getToken() produces a character that isn't TOK_RB but also isn't TOK_ERROR, then we would fail without producing an error message. Use MUST_MATCH_TOKEN instead. In fact, please file a bug about MUST_MATCH_TOKEN (a) being a macro rather than a method; (b) calling report() unconditionally (it should not report if tokenStream.getToken() returns TOK_ERROR); (c) the comment saying things about "cx" and "ts" which don't exist in the code anymore.

Add tests to detect bugs 1, 3, 4, and 5.  You can detect bugs 3 and 4 using Reflect.parse. Each node in the output has a .loc property containing location information.

@@ +7282,5 @@
> +              // Computed property name.
> +              propname = newComputedName(assignExpr());
> +              if (tokenStream.getToken() != TOK_RB)
> +                  return null();
> +              handler.setListFlag(literal, PNX_NONCONST);

Good catch adding PNX_NONCONST here! I would have missed that. Please add a test that would detect the bug if you hadn't.

::: js/src/frontend/Parser.h
@@ +441,5 @@
>      bool appendToCallSiteObj(Node callSiteObj);
>      bool addExprAndGetNextTemplStrToken(Node nodeList, TokenKind &tt);
>  #endif
>      inline Node newName(PropertyName *name);
> +    inline Node newComputedName(Node expr);

reminder to remove this declaration along with the definition

::: js/src/jsreflect.cpp
@@ +2920,5 @@
>          return builder.objectExpression(elts, &pn->pn_pos, dst);
>        }
>  
> +      case PNK_COMPUTED_NAME:
> +        return expression(pn->pn_kid, dst);

Please delete this...

@@ +2995,5 @@
>  bool
>  ASTSerializer::propertyName(ParseNode *pn, MutableHandleValue dst)
>  {
> +    if (pn->isKind(PNK_COMPUTED_NAME))
> +        return expression(pn, dst);

...and instead pass pn->pn_kid to expression here.

::: js/src/tests/ecma_6/Class/compPropNames.js
@@ +38,5 @@
> +
> +
> +// Destructuring
> +var key = "z";
> +var { [key]: foo } = { z: "bar" };

Great tests! Please move destructuring tests to a separate file.

More things to test:

*   All these should be syntax errors:

    ({[
    ({[expr
    ({[expr]
    ({[expr]})
    ({[expr] 0})
    ({[expr], 0})
    [[expr]: 0]
    ({[expr]: name: 0})
    ({[1, 2]: 3})  // because '1,2' is an Expression but not an AssignmentExpression
    ({[1;]: 1})    // and not an ExpressionStatement
    ({[if (0) 0;]})  // much less a Statement
    function f() { {[x]: 1} }  // that's not even an ObjectLiteral
    function f() { [x]: 1 }    // or that

*   Test that JSON.parse() rejects computed property names. (I'm sure it does, so just a one-line test will do.)

*   Test that the properties defined this way are ordinary enumerable, writable, configurable data properties (using Object.getOwnPropertyDescriptor to check).

*   Test that if the computed property name happens to be the name of a property on Object.prototype that has a setter:
        Object.defineProperty(Object.prototype, "x", {set: function (x) { throw "FAIL"; }});
        var a = {["x"]: 0};
    the setter is *not* called, and a.x is 0.

*   Using the same property name more than once is *not* an error. In ObjectLiterals like this one:
        a = {[x]: 1, [x]: 2};
    the second property can overwrite the first.

*   The same thing happens if the either property was defined using a non-computed property name, and even if it's an accessor property:
        a = {x: 1, ["x"]: 2};
        a = {["x"]: 1, x: 2};
        a = {get x() { return 1; }, ["x"]: 2};  // test that this makes a data property

*   In fact, I believe ES6 changes the rules so that even this is not an error, even in strict mode:
        var a = {x: 1, x: 2};
    I think the same thing happens. If you want to implement this in a separate patch, feel free.

*   Test that it works with symbols.  Stuff like
        a = {
            data: [1, 2, 3],
            [Symbol.iterator]: function () { return this.data[Symbol.iterator](); }
        };
    will probably be common; but here are two other ways to create symbols:
       var unique_sym = Symbol("1"), registered_sym = Symbol.for("2");

*   Test that it works if you run the same expression several times to build objects with different property names:

    a = [];
    for (var i = 0; ...) {
        a[i] = {["foo" + i]: ...};
    }

*   Add jit-tests. Test that it works if an expression inside a loop or function is first used to build many objects with the *same* property name or names:
        function f(tag) { return {[tag]: 0}; }
        for (...)
            a = f("first");
    and then the same loop or function is used again to build an object with a different property name or names:
        for (...)
            a = f("second");

*   Test that it can be used to define several elements (that is, properties with names that are nonnegative integers), and if possible, test that the resulting properties are stored in the object's elements rather than slots (see the comment on class ObjectElements in vm/ObjectImpl.h).

*   Test using computed property names to define several elements, but then also defining a single large index (greater than MIN_SPARSE_INDEX) or a single string property name.

*   Test using this syntax to define lots of properties:
        var code = "({";
        for (i = 0; i < 1000; i++)
            code += "['foo' + " + i + "]: 'ok', "
        code += "['bar']: 'ok'});";
        var obj = eval(code);
        // then add some assertions involving obj

*   Test that in a generator, it's possible to yield in the middle of a ComputedPropertyName.

*   Test that the behavior when combined with getter/setter syntax works as desired:
        a = {get [expr]() { ... }, set[expr](v) { ... }}
    If this syntax doesn't work yet, that's OK - just test that it's a SyntaxError, not a crash! And file a second bug to implement it.

*   Test getter/setter syntax with Reflect.parse too.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +388,5 @@
> +
> +assertExpr('a= {["field1"]: "a", field2 : "b"}',
> +          aExpr("=", ident("a"),
> +                objExpr([{ key: lit("field1"), value: lit("a"), computed: true },
> +                         { key: ident("field2"), value: lit("b"), computed: false }])));

Great! Along the same lines, test that in {[0]: 0, 1: 1}, the first field is `computed: true` and the second is `computed: false.

Comment 4

[guptha]

Thanks for the review!

(In reply to Jason Orendorff [:jorendorff] from comment #3)

> 3. This assigns the wrong start position to the new PNK_COMPUTED_NAME node,
> because it calls pos().begin after calling assignExpr().
> 
> 4. This assigns the wrong end position to the PNK_COMPUTED_NAME node,
> because it doesn't call pos() after consuming the closing bracket. Please
> make ParseHandler::newComputedName() take both a Node and a TokenPos
> argument, and call it only after getting all three pieces of information
> (kid, begin, and end).
> 

How would I actually test 3 and 4? We just pass pn->pn_kid to expression in jsreflect? We don't actually create a node for COMPUTED_NAME in Reflect.parse.

> Good catch adding PNX_NONCONST here! I would have missed that. Please add a
> test that would detect the bug if you hadn't.

How do I test this? If I don't add that, control will flow to getConstantValue. The fact that something like
var b = 2;
a = { [b] : 2, 3 : 3 }
works is proof that the flag's been set, and patterns like that are tested in other places. Is that enough or did you have something particular in mind?


>         a = {get x() { return 1; }, ["x"]: 2};  // test that this makes a
> data property

The above throws an error saying, "SyntaxError: property name x appears more than once in object literal". The same error is thrown even for
a = {get x() { return 1; }, x: 2};
Is that a bug?

> *   Test that it works with symbols.  Stuff like
>         a = {
>             data: [1, 2, 3],
>             [Symbol.iterator]: function () { return
> this.data[Symbol.iterator](); }
>         };

Um, I'm not sure I understand what this code fragment does. Can you please explain?

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to guptha from comment #4)
> How would I actually test 3 and 4? We just pass pn->pn_kid to expression in
> jsreflect? We don't actually create a node for COMPUTED_NAME in
> Reflect.parse.

Oh no! This is my fault.

Can you easily change it to the other approach? Add a ComputedPropertyName node, get rid of the .computed boolean? I'm sorry for the noise.

Reflect.parse has to be good enough to use for static analysis and rewriting; some simple rewrites really benefit from precise location information on things like this.

> > Good catch adding PNX_NONCONST here! I would have missed that. Please add a
> > test that would detect the bug if you hadn't.
> 
> How do I test this?

Existing tests are good enough, thanks.

> >         a = {get x() { return 1; }, ["x"]: 2};  // test that this makes a
> > data property
> 
> The above throws an error saying, "SyntaxError: property name x appears more
> than once in object literal". The same error is thrown even for
> a = {get x() { return 1; }, x: 2};
> Is that a bug?

Yes.

> > *   Test that it works with symbols.  Stuff like
> >         a = {
> >             data: [1, 2, 3],
> >             [Symbol.iterator]: function () { return
> > this.data[Symbol.iterator](); }
> >         };
> 
> Um, I'm not sure I understand what this code fragment does. Can you please
> explain?

A symbol is a kind of value that can be used as a property key. It's not a string, it's a different kind of value. So for example:

    var key1 = "moon";
    var key2 = Symbol("moon");  // create a unique symbol
    var obj = {};
    obj[key1] = 1;     // create a data property with a string key
    print(obj[key1]);  // 1
    obj[key2] = 2;     // create a data property with a symbol key
    print(obj[key2]);  // 2

You should be able to do the same thing with computed property names:

    var obj = {[key1]: 1, [key2]: 2};

It should just work, we just need tests for it.

Symbol.iterator is just a standard built-in symbol; you don't have to use that for the tests.

Comment 6

[guptha]

Attachment: Patch to implement computed property names v1

Comment 7

[guptha]

Jason,

Did you want to skim through the patch? There were quite a few changes.

Also, is this ready for commit? The getter and setter syntax will be added in bug 1048384. Bug 1041128 was created by someone else for the duplicate property names issue.

Comment 8

[Jason Orendorff [:jorendorff]]

This looks good!

Comment 9

[guptha]

Attachment: Patch to implement computed property names v2

Updated commit message.

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7079b7552946

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7079b7552946

Comment 12

[Florian Scholz (Open Web Docs)]

Wrote a new page collecting info on object literal syntax:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

Also mentioned on related pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Computed_object_property_names_and_destructuring
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#Using_a_computed_property_name
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set#Using_a_computed_property_name
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Computed_property_names

Developer release notes
https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript

Any reviews to the wiki pages are very much appreciated.