Allow shorthand properties in object literals

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bbenvie, Assigned: Swatinem)

Tracking

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

Trunk
mozilla33
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
ES6 allows shorthand properties in object initializers that set a property of the same name as a given identifier.

> let a = 10, b = 20;
> let x = { a, b }; // { a: 10, b: 20 }

This is symmetric with object destructuring shorthand (bug 404734).


http://wiki.ecmascript.org/doku.php?id=harmony:object_initialiser_shorthand

(either I'm missing something or that page incorrectly states that SpiderMonkey supports this already)
(Reporter)

Updated

6 years ago
Summary: Allow shorthand properties in object literals outside of JS1.8 → Allow shorthand properties in object literals
(Assignee)

Comment 1

5 years ago
Posted patch shorthand.patch (obsolete) — Splinter Review
Works fine locally, running on try now: https://tbpl.mozilla.org/?tree=Try&rev=5cbb4283c432
Assignee: general → arpad.borsos
Status: NEW → ASSIGNED
Attachment #8436377 - Flags: review?(jdemooij)
(Assignee)

Comment 2

5 years ago
Theres a linker error on try/b2g, and some other failures that seem to be unrelated. Maybe I rebased it on a broken inbound. Otherwise the tests are green.
Comment on attachment 8436377 [details] [diff] [review]
shorthand.patch

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

Thanks for the patch! I'll forward this to Jason though as he's more familiar with the spec and the frontend.
Attachment #8436377 - Flags: review?(jdemooij) → review?(jorendorff)
Great. I'll review it tomorrow morning.

(I'm mostly through reviewing your other patch too.)
Comment on attachment 8436377 [details] [diff] [review]
shorthand.patch

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

Phew! Great patch. Clearing r? for now, but eager for the next revision.

Please add tests for object literals containing more than one shorthand property, like
    function point(x, y) { return {x, y}; }
and objects containing a mixture of shorthand and non-shorthand properties.

Also test that ({"ident"}) and ({0}) are SyntaxErrors.

Also: please update Reflect.parse for these changes. Reflect.parse is a SpiderMonkey extenstion. It works very much like [Esprima](http://esprima.org/), and I would love to delete Reflect.parse and let people use Esprima, except that Reflect.parse is something like 7x faster. So here's what needs to be done:

- add a boolean argument to FullParseHandler::addPropertyDefinition indicating whether the property is shorthand or not

- store that on the parse node somehow (I suggest using two different parse node kinds, PNK_COLON and a new one called PNK_SHORTHAND).

- change BytecodeEmitter.cpp to treat PNK_SHORTHAND exactly like PNK_COLON

- change jsreflect.cpp to treat PNK_SHORTHAND differently from PNK_COLON, by adding a `"shorthand": true` property to the "Property" node. PNK_COLON nodes should have `"shorthand": false`. This matches what Esprima does (in the harmony branch):

    js> load("esprima.js")
    js> esprima.parse("({x})").body[0].expression
    ({
        type: "ObjectExpression",
        properties: [
            {
                type: "Property",
                key: {type: "Identifier", name: "x"},
                value: {type: "Identifier", name: "x"},
                ...
                shorthand: true
            }
        ]
    })

- add a test case or two in /js/src/tests/js1_8_5/extensions/reflect-parse.js.

Let me know if you have any questions about this!

::: js/src/frontend/BytecodeEmitter.cpp
@@ -5890,5 @@
>  {
> -    if (pn->pn_xflags & PNX_DESTRUCT) {
> -        bce->reportError(pn, JSMSG_BAD_OBJECT_INIT);
> -        return false;
> -    }

Oh, wow. PNX_DESTRUCT seems to be total nonsense. Do you have a patch to remove it altogether? Interested in writing one? If not, I'll do it, after this lands.

::: js/src/frontend/FullParseHandler.h
@@ +258,2 @@
>          JS_ASSERT(literal->isArity(PN_LIST));
>          literal->pn_xflags |= PNX_DESTRUCT | PNX_NONCONST;  // XXX why PNX_DESTRUCT?

Definitely try removing this use of PNX_DESTRUCT, since you seem to be removing the only two places that checked for it.

Update this comment in ParseNode.h:

> * PNK_COLON    binary      key-value pair in object initializer or
> *                          destructuring lhs
> *                          pn_left: property id, pn_right: value
> *                          var {x} = object destructuring shorthand shares
> *                          PN_NAME node for x on left and right of PNK_COLON
> *                          node in PNK_OBJECT's list, has PNX_DESTRUCT flag

The last three lines, I think, can be replaced with something like

  * PNK_SHORTHAND binary     Same fields as PNK_COLON. This is used for object
  *                          literal properties using shorthand ({x}).

And this one:
> #define PNX_DESTRUCT    0x10            /* destructuring special cases:
>                                            1. shorthand syntax used, at present
>                                               object destructuring ({x,y}) only;
>                                            2. code evaluating destructuring
>                                               arguments occurs before function
>                                               body */

(You are removing special case #1.)

::: js/src/frontend/Parser.cpp
@@ +7207,3 @@
>                      return null();
>              }
>              else {

While you're in this code, please get rid of the newline between "}" and "else {" here. They should be on the same line. Same thing for the "}" and "else if (..." on or near lines 7192-7193.

And, if you don't mind, same thing on line 4143-4 and line 4156-7.

::: js/src/jit-test/tests/basic/object-shorthand.js
@@ +72,5 @@
> +reservedStrict.forEach(ident =>
> +  assertEq(new Function('var ' + ident + ' = 10; return {' + ident + '}.' + ident + ';')(), 10));
> +
> +reservedStrict.concat(['let', 'yield']).forEach(ident =>
> +  assertThrowsInstanceOf(() => new Function('"use strict" return {' + ident + '}'), SyntaxError));

The SyntaxError thrown here is:

js> var ident = 'q'; new Function('"use strict" return {' + ident + '}')
typein:1:13 SyntaxError: missing ; before statement:
typein:1:13 "use strict" return {q}
typein:1:13 .............^

To make this test what you intended, add the missing semicolon!

(Automatic semicolon insertion only happens at the end of a line or before a '}' token.)
Attachment #8436377 - Flags: review?(jorendorff)
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e5748b724871

I’m actually surprised that the previous try run didn’t fail the tests asserting that the shorthand is a syntax error. Am I missing some suites in trychooser?

> - store that on the parse node somehow (I suggest using two different parse
> node kinds, PNK_COLON and a new one called PNK_SHORTHAND).

Had to update quite some places for that, hope I didn’t miss any.

> >          JS_ASSERT(literal->isArity(PN_LIST));
> >          literal->pn_xflags |= PNX_DESTRUCT | PNX_NONCONST;  // XXX why PNX_DESTRUCT?
> 
> Definitely try removing this use of PNX_DESTRUCT, since you seem to be
> removing the only two places that checked for it.

I left the code in for now. Let’s get rid of it in a followup. I will work up a patch after I figure out what the "legitimate" usage of that flag is.

> The SyntaxError thrown here is:
> 
> js> var ident = 'q'; new Function('"use strict" return {' + ident + '}')
> typein:1:13 SyntaxError: missing ; before statement:
> typein:1:13 "use strict" return {q}
> typein:1:13 .............^
> 
> To make this test what you intended, add the missing semicolon!

Good catch! I wouldn’t have noticed that, haha.
Attachment #8436377 - Attachment is obsolete: true
Attachment #8442700 - Flags: review?(jorendorff)
(Assignee)

Comment 7

5 years ago
> - change jsreflect.cpp to treat PNK_SHORTHAND differently from PNK_COLON, by
> adding a `"shorthand": true` property to the "Property" node. PNK_COLON
> nodes should have `"shorthand": false`. This matches what Esprima does (in
> the harmony branch):

What about ObjectPattern? It supported "shorthands" for quite some time, but didn’t report them in the Reflect.parse() AST either. (And they still don’t do it, since they use a different codepath)

Should I add a shorthand flag for those as well?
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(In reply to Arpad Borsos (Swatinem) from comment #7)
> What about ObjectPattern? It supported "shorthands" for quite some time, but
> didn’t report them in the Reflect.parse() AST either. (And they still don’t
> do it, since they use a different codepath)
> 
> Should I add a shorthand flag for those as well?

Yes please, if you don't mind!
(In reply to Arpad Borsos (Swatinem) from comment #6)
> I’m actually surprised that the previous try run didn’t fail the tests
> asserting that the shorthand is a syntax error. Am I missing some suites in
> trychooser?

Which tests? Let's make sure we understand what's going on here before landing.
(Assignee)

Comment 10

5 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #9)
> (In reply to Arpad Borsos (Swatinem) from comment #6)
> > I’m actually surprised that the previous try run didn’t fail the tests
> > asserting that the shorthand is a syntax error. Am I missing some suites in
> > trychooser?
> 
> Which tests? Let's make sure we understand what's going on here before
> landing.

The two in `tests/js1_8_5/extensions` They had asserts that a missing RHS of a property is supposed to throw a SyntaxError. Noticed that running locally. Aren’t they part of the JSReftest Suite? Or which one are they in?
(In reply to Arpad Borsos (Swatinem) from comment #10)
> The two in `tests/js1_8_5/extensions` They had asserts that a missing RHS of
> a property is supposed to throw a SyntaxError. Noticed that running locally.
> Aren’t they part of the JSReftest Suite? Or which one are they in?

Those two tests are shell-only. You can tell by the first line, which says:

// |reftest| skip-if(!xulRuntime.shell)

We should run shell-only tests on tbpl, but we don't.
Comment on attachment 8442700 [details] [diff] [review]
shorthand.patch

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

r=me with these comments addressed. Very nice work.

::: js/src/frontend/FullParseHandler.h
@@ +251,5 @@
> +        JS_ASSERT(literal->isArity(PN_LIST));
> +        ParseNode *propdef = newBinary(isShorthand ? PNK_SHORTHAND : PNK_COLON, name, expr,
> +                                       JSOP_INITPROP);
> +        if (isShorthand)
> +            literal->pn_xflags |= PNX_DESTRUCT | PNX_NONCONST;  // XXX why PNX_DESTRUCT?

Definitely delete PNX_DESTRUCT here -- in a separate changeset if you insist, r=me. This change is required.

::: js/src/frontend/Parser.cpp
@@ +727,5 @@
>        case PNK_RETURN:
>          return ENDS_IN_RETURN;
>  
>        case PNK_COLON:
> +      case PNK_SHORTHAND:

Not here! You have found a bug! This PNK_COLON should be PNK_LABEL. Please fix it (in a separate changeset if you like).

::: js/src/jit-test/tests/basic/object-shorthand.js
@@ +61,5 @@
> +
> +// non-identifiers should also throw
> +var nonidents = [
> +  '"str"',
> +  0

Micro-nit: It makes a little more sense for the 0 to be in (only one set of) quotes. To me, anyway.

@@ +75,5 @@
> +  'private',
> +  'protected',
> +  'public',
> +  'static',
> +  // XXX: according to 12.1.1, these should only be errors in strict code:

Please make sure the follow-up bug for this is filed, blocking es6.

::: js/src/jit-test/tests/basic/testBug775807.js
@@ +1,1 @@
> +// |jit-test| dump-bytecode

Why remove the `;error:SyntaxError` here? It must be a bug... the file should definitely still be a SyntaxError. It would require }}))}) at the end to balance all the open parens and braces!

::: js/src/jit/AsmJS.cpp
@@ +274,5 @@
>  
>  static inline bool
>  IsNormalObjectField(ExclusiveContext *cx, ParseNode *pn)
>  {
> +    JS_ASSERT(pn->isKind(PNK_COLON) || pn->isKind(PNK_SHORTHAND));

Oh, interesting. A new feature for the asm.js language! And one that I think anyone hand-coding asm will appreciate. :)
Attachment #8442700 - Flags: review?(jorendorff) → review+
(Assignee)

Updated

5 years ago
Depends on: 1032150
(Assignee)

Comment 13

5 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> ::: js/src/jit-test/tests/basic/testBug775807.js
> @@ +1,1 @@
> > +// |jit-test| dump-bytecode
> 
> Why remove the `;error:SyntaxError` here? It must be a bug... the file
> should definitely still be a SyntaxError. It would require }}))}) at the end
> to balance all the open parens and braces!

According to my editor, the parens are perfectly balanced.

The syntaxerror came from here:
```
        return {
            t: {
                c
            }
        }
```
Which used to be an error, but now its a perfectly valid shorthand.
(Assignee)

Updated

5 years ago
Attachment #8447964 - Attachment is obsolete: true
Attachment #8447973 - Flags: review?(jorendorff) → review+
(In reply to Arpad Borsos (Swatinem) from comment #13)
> According to my editor, the parens are perfectly balanced.

Oh, I was confused by Splinter -- I thought it was showing the whole file, but it wasn't.
Setting ni?luke to make absolutely sure he knows we're changing the asm.js language here. Luke, let me know if any follow-up is necessary.
Flags: needinfo?(luke)
Actually, given that any addition like this makes asm.js bigger (the spec and any future custom parsers), I'd rather leave it out since it's not necessary.  Also, assuming this isn't already implemented in other browsers, Emscripten wouldn't be able to emit this.  Are there any complications, or can you just remove the "|| pn->isKind(PNK_SHORTHAND)" that was added in this patch?
Flags: needinfo?(luke) → needinfo?(jorendorff)
(Assignee)

Comment 20

5 years ago
Hm, its no problem removing it I guess.
But how about this and other ES6 features. Are they not allowed in asm.js?

As for this specific feature: Its just syntactic sugar. It does not change any semantics at all.
https://hg.mozilla.org/mozilla-central/rev/65a000342390
https://hg.mozilla.org/mozilla-central/rev/73db6acaf49f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(In reply to Luke Wagner [:luke] from comment #19)
> Are there any complications, or
> can you just remove the "|| pn->isKind(PNK_SHORTHAND)" that was added in
> this patch?

No complications.

Swatinem, if you haven't already, please file the follow-up bug to remove this feature from asm.js. Instead of removing the PNK_SHORTHAND check from the assertion, I think you'll have to add a PNK_SHORTHAND check to the return value in IsNormalObjectField.

As for rationale: In general, asm.js's syntax is deliberately extremely simple, and asm.js code runs in all recent browsers.
Flags: needinfo?(jorendorff)
Flags: needinfo?(arpad.borsos)
(Assignee)

Updated

5 years ago
Depends on: 1034972
(Assignee)

Comment 23

5 years ago
Filed bug 1034972 about that.
Flags: needinfo?(arpad.borsos)
Thanks guys!
This is mentioned on the "Firefox 33 for developers" page:
https://developer.mozilla.org/en-US/Firefox/Releases/33#JavaScript

And also on the "lexical grammar" page, which talks about the different literal notations in JS:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Object_literals

Any review or feedback appreciated.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 26

5 years ago
(In reply to Florian Scholz [:fscholz] (elchi3) from comment #25)
> Any review or feedback appreciated.

Looks good. I don’t know to which FF version the lexical grammar should be tied, but there are a lot more syntax features coming there.
You need to log in before you can comment on or make changes to this bug.