461233 - Incorrect decompilation of ({0: (4, <></>) })

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> f = (function() { return ({0: (4, <></>) }); })
function () {
    return {0: 4, <></>};
}

js> eval(uneval(f))
typein:3: SyntaxError: invalid property id:
typein:3: (function () {return {0: 4, <></>};})
typein:3: ............................^

Parens are needed in this context so the "sequential evaluation" comma won't become an "object literal property separation" comma.  This is a regression from the patch in bug 460501 (eliminating JSOP_GROUP).

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: fix

This fixes the above and a separate bug where significant parentheses were dropped:

js> (function() [(1, 2)])
function () [1, 2]

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 344390 [details] [diff] [review]
fix

>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -4367,24 +4367,18 @@ Decompile(SprintStack *ss, jsbytecode *p
>                 break;
>               }
> 
>               {
>                 JSBool isFirst;
>                 const char *maybeComma;
> 
>               case JSOP_INITELEM:
>-                /* Turn off most parens (all if there's only one initialiser). */
>-                LOCAL_ASSERT(pc + len < endpc);
>-                isFirst = (ss->opcodes[ss->top - 3] == JSOP_NEWINIT);
>-                op = (isFirst &&
>-                      GetStr(ss, ss->top - 2)[0] == '0' &&
>-                      (JSOp) pc[len] == JSOP_ENDINIT)
>-                     ? JSOP_NOP
>-                     : JSOP_SETNAME;
>+                /* Turn off most parens. */
>+                op = JSOP_SETNAME;

Can't avoid setting isFirst because control might flow...

>                 rval = POP_STR();
> 
>                 /* Turn off all parens for xval and lval, which we control. */
>                 op = JSOP_NOP;
>                 xval = POP_STR();
>                 lval = POP_STR();
>                 sn = js_GetSrcNote(jp->script, pc);
> 
[missing context supplied]
>                 if (sn && SN_TYPE(sn) == SRC_INITPROP) {
>                     atom = NULL;
>                     goto do_initprop;
>                 }

and code at do_initprop: uses isFirst:

              do_initprop:
                maybeComma = isFirst ? "" : ", ";

I forget which bug led to the isFirst/maybeComma work, but ripping it out seems like it might reopen that bug -- yet if losing JSOP_GROUP regressed the current bug, the fix ought to be elsewhere, unless the isFirst/etc. stuff was depending on JSOP_GROUP.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Oops -- I don't mean to remove isFirst/maybeComma.

The extra haggling over the op was introduced in bug 381113, and there is a test for it (which I'm now failing), but that behavior doesn't make any sense to me.  There is nothing syntactically special about an array with just one initializer.

I think the way to make that test pass (with or without JSOP_GROUP) is to give JSOP_LEAVEBLOCKEXPR the correct precedence both inside and out.

Then I ran across bug 461457.  Working on it!

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

This patch also fixes bug 382400.

It breaks the following tests by harmlessly removing parentheses around some let-expressions.  :(

js1_7/block/regress-352609.js
js1_7/decompilation/regress-349602.js
js1_7/decompilation/regress-352011.js
js1_7/decompilation/regress-352022.js
js1_7/extensions/regress-353249.js
js1_7/regress/regress-420399.js

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Oops -- I don't mean to remove isFirst/maybeComma.

These came in with patches for bug 376957 and bug 410571.

> The extra haggling over the op was introduced in bug 381113, and there is a
> test for it (which I'm now failing), but that behavior doesn't make any sense
> to me.  There is nothing syntactically special about an array with just one
> initializer.

Confusion: do you mean the isFirst/maybeComma haggling? Bug 381113 was for expression closures, and I don't see anything there that specializes [foo] decompilation.

> I think the way to make that test pass (with or without JSOP_GROUP) is to give
> JSOP_LEAVEBLOCKEXPR the correct precedence both inside and out.
> 
> Then I ran across bug 461457.  Working on it!

Scary! If those let expressions need not be parenthesized, great. Reviewing...

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 344577 [details] [diff] [review]
v2

>@@ -4353,17 +4348,17 @@ Decompile(SprintStack *ss, jsbytecode *p
>               case JSOP_INITPROP:
>                 LOAD_ATOM(0);
>                 xval = QuoteString(&ss->sprinter, ATOM_TO_STRING(atom),
>                                    (jschar)
>                                    (ATOM_IS_IDENTIFIER(atom) ? 0 : '\''));
>                 if (!xval)
>                     return NULL;
>                 isFirst = (ss->opcodes[ss->top - 2] == JSOP_NEWINIT);
>-                rval = POP_STR();
>+                rval = PopStr(ss, JSOP_SETLOCAL);

Any reason for JSOP_SETLOCAL here? or for this change at all? JSOP_INITPROP has precedence 3, same as all the other JSOP_SET* ops.

>                 lval = POP_STR();

This has high precedence (JSOP_NEWINIT, 19) so no need to farble op or force precedence.

>+ * Likewise, let expressions have precedence 3 to retain the parentheses in
>+ * (let (a=0) x)[a] .

Nit: space before period.

More significant: the indexing operation _[a] involves precedence 18 ops (GETPROP/ELEM). Popping the left operand of [ will automatically parenthesize lower-precedence expressions including let exprs, whether or not let exprs have prec 1 or 3. So this comment doesn't explain the change from 1 to 3.

Also, yield theoretically should be around 3, but it's 1. Comment might want to say why (I know, I'm talking to myself here, but I forgot the reason and you may have rediscovered it :-P).

>+ * Function expressions have the maximum precedence because the decompiler
>+ * unconditionally parenthesizes them.

This is misworded: the decompiler does not explicitly parenthesize function expressions:

js> function f(){g(function(){}, function(){})}
js> f
function f() {
    g(function () {}, function () {});
}

And having maximum precedence would produce parenthesization only when the function expression were popped into an even higher-precedence operator's operand context. Were you pointing out that function expressions are primary expressions, so prec 19 along with numbers and such? Probably could just lose this sentence.

/be

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v3

(In reply to comment #6)
> (From update of attachment 344577 [details] [diff] [review])
> >@@ -4353,17 +4348,17 @@ Decompile(SprintStack *ss, jsbytecode *p
> >               case JSOP_INITPROP:
> >                 LOAD_ATOM(0);
> >                 xval = QuoteString(&ss->sprinter, ATOM_TO_STRING(atom),
> >                                    (jschar)
> >                                    (ATOM_IS_IDENTIFIER(atom) ? 0 : '\''));
> >                 if (!xval)
> >                     return NULL;
> >                 isFirst = (ss->opcodes[ss->top - 2] == JSOP_NEWINIT);
> >-                rval = POP_STR();
> >+                rval = PopStr(ss, JSOP_SETLOCAL);
> 
> Any reason for JSOP_SETLOCAL here? or for this change at all?

Um, no, I can't remember why I touched that.  I've removed the change in v3.

> >+ * Likewise, let expressions have precedence 3 to retain the parentheses in
> >+ * (let (a=0) x)[a] .
> 
> Nit: space before period.
> 
> More significant: the indexing operation _[a] involves precedence 18 ops
> (GETPROP/ELEM). Popping the left operand of [ will automatically parenthesize
> lower-precedence expressions including let exprs, whether or not let exprs have
> prec 1 or 3. So this comment doesn't explain the change from 1 to 3.

There are two things going on here.

The change from 1 to 3 is so let-expressions are not overparenthesized around the comma operator, in argument lists, or in array initializers.

The comment was NOT meant to explain the change from 1 to 3 but rather why let-expressions don't have the same precedence as you would guess from the grammar, or equivalently the place in jsparse where they're parsed (i.e. why it's 3 and not 19).

> Also, yield theoretically should be around 3, but it's 1. Comment might want to
> say why (I know, I'm talking to myself here, but I forgot the reason and you
> may have rediscovered it :-P).

I haven't, but I'd be happy to check in a comment for you if you remember now.  :)

> >+ * Function expressions have the maximum precedence because the decompiler
> >+ * unconditionally parenthesizes them.
> 
> This is misworded: the decompiler does not explicitly parenthesize function
> expressions:
> 
> js> function f(){g(function(){}, function(){})}
> js> f
> function f() {
>     g(function () {}, function () {});
> }

You're right.  I was thinking about expression closures when I wrote that.  New wording in v3.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 345061 [details] [diff] [review]
v3

>               {
>                 JSBool isFirst;
>                 const char *maybeComma;
> 
>               case JSOP_INITELEM:
>-                /* Turn off most parens (all if there's only one initialiser). */
>-                LOCAL_ASSERT(pc + len < endpc);
>                 isFirst = (ss->opcodes[ss->top - 3] == JSOP_NEWINIT);
>-                op = (isFirst &&
>-                      GetStr(ss, ss->top - 2)[0] == '0' &&
>-                      (JSOp) pc[len] == JSOP_ENDINIT)
>-                     ? JSOP_NOP
>-                     : JSOP_SETNAME;
>+                /* Turn off most parens. */

Nit: one blank line before comment-taking-one-or-more lines. Or you could make the comment go on the next line:

>+                op = JSOP_SETNAME;

tabbed over on the right, without capitalization and full stop (other "turn off" comments do this).

>                 rval = POP_STR();
> 
>                 /* Turn off all parens for xval and lval, which we control. */
>                 op = JSOP_NOP;

Here's a reason to stick with comment-before-on-its-own-line, but this case has a nice blank line before the comment line.

>+ * Likewise, let expressions have precedence 3, not 19, to retain the
>+ * parentheses in, for example, (let (a=0) x) || a.

Here we see the top-down hack that is ambiguous (reduce-reduce conflict) that Waldemar pointed out on [e-mail] in a bottom-up grammar: let expressions are "primary" when viewed from the left, but they eat up ops to the right as if assignment expressions.

So you might advert to this more explicitly (without too much detail ;-) and contrast to prec 2-4 ops (, = ?:).

>+                                                    Expression closures, though
>+ * syntactically similar, have precedence 19 because the decompiler
>+ * unconditionally parenthesizes them.

This bit about expression closures does not match the tabulated mention of JSOP_ANONFUNOBJ, JSOP_NAMEFUNOBJ (which are used for block-bodied as well as expression-bodied lambdas). Maybe just lose it and let the table speak for itself.

On the JSOP_YIELD being precedence 1, I forget -- it set op to JSOP_SETNAME before popping its parameter, so it doesn't need to be 3, but if it were 3, then could it stop setting op to JSOP_SETNAME? I hope we have test coverage; I'll try this today.

r=me with nits picked.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

- * Likewise, let expressions have precedence 3, not 19, to retain the
- * parentheses in, for example, (let (a=0) x) || a. Expression closures, though
- * syntactically similar, have precedence 19 because the decompiler
- * unconditionally parenthesizes them.
+ * Let expressions are "primary" when viewed from the left, but they eat up ops
+ * to the right as if assignment expressions and therefore have precedence 3.
+ * This makes the decompiler retain the parentheses in (let (a=0) x) ? a : 0
+ * but omit the superfluous ones in (let (a=0) x), a.
+ *
+ * Yield expressions must be parenthesized even in comma-expressions and
+ * argument lists, so they have the lowest precedence.

http://hg.mozilla.org/tracemonkey/rev/4702f17a8589

Comment 10

[Brendan Eich [:brendan]]

Perfect -- I wasn't remembering the yield rationale because I wrote over that RAM to record the ES4 evolution, which would not overparenthesize yield -- but that is a dead letter now (and probably broken, per the bottom-up grammar objection from es-discuss).

/be

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #1)

> js> (function() [(1, 2)])
> function () [1, 2]

this is not fixed.

Comment 12

[Brendan Eich [:brendan]]

It's fixed in tracemonkey, not in mozilla-central -- yes, we are overdue to merge. Today or bust!

/be

Comment 13

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/37b3fdbb0f07
/cvsroot/mozilla/js/tests/e4x/decompilation/regress-461233.js,v  <--  regress-461233.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/decompilation/regress-461233.js,v  <--  regress-461233.js
initial revision: 1.1