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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jruderman, Unassigned)

Tracking

(Blocks 1 bug, {regression, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Regression from the patch in bug 460501.
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.
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.
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
Posted patch v1 (lame)Splinter Review
(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.
Attachment #344386 - Flags: review?(brendan)
Attachment #344386 - Flags: review?(brendan) → review+
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
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.
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
Pushed v1 with the suggested change.

http://hg.mozilla.org/tracemonkey/rev/7b641804f5fc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.