Implement ES6 computed property names

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bbenvie, Assigned: gupta.rajagopal)

Tracking

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

Trunk
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2][DocArea=JS])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

6 years ago
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.
Reporter

Comment 1

6 years ago
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.
Keywords: feature
Whiteboard: [js:p2]
Assignee

Comment 2

5 years ago
1. This probably needs more tests.
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
Attachment #8462937 - Flags: review?(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.
Attachment #8462937 - Flags: review?(jorendorff) → review+
Assignee

Comment 4

5 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(jorendorff)
Whiteboard: [js:p2] → [js:p2][DocArea=JS]
Assignee

Comment 6

5 years ago
Attachment #8462937 - Attachment is obsolete: true
Attachment #8467186 - Flags: review+
Assignee

Comment 7

5 years ago
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.
Flags: needinfo?(jorendorff)
This looks good!
Flags: needinfo?(jorendorff)
Assignee

Comment 9

5 years ago
Updated commit message.

https://tbpl.mozilla.org/?tree=Try&rev=2b114954081c
Attachment #8467186 - Attachment is obsolete: true
Attachment #8470087 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7079b7552946
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Depends on: 1073919
Depends on: 1090096
Depends on: 1127012
You need to log in before you can comment on or make changes to this bug.