Brandon Benvie [:benvie] Reporter
	Description • 11 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)

	Arpad Borsos [:Swatinem] Assignee
	Comment 1 • 10 years ago

Works fine locally, running on try now: https://tbpl.mozilla.org/?tree=Try&rev=5cbb4283c432

	Arpad Borsos [:Swatinem] Assignee
	Comment 2 • 10 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.

	Jan de Mooij [:jandem]
	Comment 3 • 10 years ago

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.

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

Great. I'll review it tomorrow morning.

(I'm mostly through reviewing your other patch too.)

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

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.)

	Arpad Borsos [:Swatinem] Assignee
	Comment 6 • 10 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.

	Arpad Borsos [:Swatinem] Assignee
	Comment 7 • 10 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?

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

(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!

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

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

	Arpad Borsos [:Swatinem] Assignee
	Comment 10 • 10 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?

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

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

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

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

	Arpad Borsos [:Swatinem] Assignee
	Comment 13 • 10 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.

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

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

	Arpad Borsos [:Swatinem] Assignee
	Comment 17 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/65a000342390
https://hg.mozilla.org/integration/mozilla-inbound/rev/73db6acaf49f

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

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.

	Luke Wagner [:luke]
	Comment 19 • 10 years ago

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?

	Arpad Borsos [:Swatinem] Assignee
	Comment 20 • 10 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.

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

https://hg.mozilla.org/mozilla-central/rev/65a000342390
https://hg.mozilla.org/mozilla-central/rev/73db6acaf49f

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

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

	Arpad Borsos [:Swatinem] Assignee
	Comment 23 • 10 years ago

Filed bug 1034972 about that.

	Luke Wagner [:luke]
	Comment 24 • 10 years ago

Thanks guys!

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

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.

	Arpad Borsos [:Swatinem] Assignee
	Comment 26 • 10 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.