461111 - Extra parens in decompilation of "if(a, b)"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> (function() { if(a, b) { } })
function () {
    if ((a, b)) {
    }
}

Regression from the patch in bug 460501.

Comment 1

[Jason Orendorff [:jorendorff]]

I knew about this when I wrote the patch.  I think we should call this INVALID.

The fuzzer doesn't choke on this, does it?  The new spelling produced by the decompiler should round-trip just as well as the old one did.

Comment 2

[Jesse Ruderman]

Why is this behavior better?

The fuzzer has a "checkForExtraParens" test, in addition to the round-trip test.  I can make the fuzzer skip this test whenever an if/while condition includes a comma, or something like that, if you and Brendan agree this is the desired behavior.

Comment 3

[Brendan Eich [:brendan]]

Again I'd rather not over-parenthesize anything, or change output (we don't know all the downstream consumers of decompilations -- we may be inflicting make-work on a bunch of folks).

Also, the decompiler should be able to order ops so that its precedence-based automatic parenthesization works here too. Is it hard to do so?

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1 (lame)

(In reply to comment #3)
> Again I'd rather not over-parenthesize anything, or change output (we don't
> know all the downstream consumers of decompilations -- we may be inflicting
> make-work on a bunch of folks).

OK.

> Also, the decompiler should be able to order ops so that its precedence-based
> automatic parenthesization works here too. Is it hard to do so?

The problem is that = gets the same opcode as += and the like.

The attached patch fixes the bug under its current summary, but it still overparenthesizes +=.  I think I see a nice way to fix that, but I need to think about it.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 344386 [details] [diff] [review]
v1 (lame)

>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -1760,17 +1760,20 @@ Decompile(SprintStack *ss, jsbytecode *p
> #define NEXT_OP(pc)           (((pc) + (len) == endpc) ? nextop : pc[len])
> #define POP_STR()             PopStr(ss, op)
> #define POP_STR_PREC(prec)    PopStrPrec(ss, prec)
> 
> /*
>  * Pop a condition expression for if/for/while. JSOP_IFEQ's precedence forces
>  * extra parens around assignment, which avoids a strict-mode warning.
>  */
>-#define POP_COND_STR()        PopStr(ss, JSOP_IFEQ)
>+#define POP_COND_STR() \
>+    PopStr(ss, (js_CodeSpec[ss->opcodes[ss->top - 1]].format & JOF_ASSIGNING)\

r=me with JOF_SET instead of JOF_ASSIGNING -- JOF_ASSIGNING is the old, formerly distinct but now redundant and overlong alias.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Instead of POP_COND_STR, I could have the JSOP_SET* code look ahead (many opcodes but they all share code in Decompile), see if the next op is one of the few conditionals we care about, and wrap the assignment in parens if so.  That makes for nicer behavior (doesn't wrap +=) but even more brittle code.

Comment 7

[Bob Clary [:bc] (inactive)]

from bug 443074 comment 23: currently also covered by

js1_8/genexps/regress-380237-03.js

Expected value ' function ( ) { if ( 1 , ( x for ( x in [ ] ) ) ) { } } ',
Actual value ' function ( ) { if ( ( 1 , ( x for ( x in [ ] ) ) ) ) { } } '

this test will be obsoleted for the 1.9.1 branch soon and the new test will be in js1_8_1/decompilation/regress-380237-03.js

Comment 8

[Jason Orendorff [:jorendorff]]

Pushed v1 with the suggested change.

http://hg.mozilla.org/tracemonkey/rev/7b641804f5fc

Comment 9

[Bob Clary [:bc] (inactive)]

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