Last Comment Bug 736742 - Decompiler omits sequential-evaluation parens on RHS of destructuring assignment
: Decompiler omits sequential-evaluation parens on RHS of destructuring assignment
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: jsfunfuzz 692274
  Show dependency treegraph
 
Reported: 2012-03-17 09:31 PDT by Jesse Ruderman
Modified: 2012-03-29 08:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (6.66 KB, patch)
2012-03-17 11:40 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-03-17 09:31:26 PDT
The decompiler changed the meaning here:
  js> (function() { [m] = (a, b) })
  (function () {[m] = a, b;})

jsfunfuzz discovered this because some variants result in a syntax error, e.g.
  js> (function() { var [m] = (1, b.c); })
  (function () {var [m] = 1, b.c;})

The first bad revision is:
changeset:       38344f96b3e3
user:            Luke Wagner
date:            Fri Oct 07 12:02:50 2011 -0700
pushed to m-c:   Fri Dec 23 15:56:40 2011 -0800
summary:         Bug 692274, part 4 - Rewrite parsing, emitting and decompiling of let to fix scoping properly (r=jorendorff)
Comment 1 Luke Wagner [:luke] 2012-03-17 11:40:06 PDT
Created attachment 606890 [details] [diff] [review]
fix

Good find!  Who can forget about the precedence of comma in a destructuring assignment.
Comment 2 Luke Wagner [:luke] 2012-03-24 14:56:30 PDT
Review ping.  Green on try.
Comment 3 Jason Orendorff [:jorendorff] 2012-03-27 11:13:35 PDT
Comment on attachment 606890 [details] [diff] [review]
fix

OK, the change to rval I can understand, the JSOP_SETNAME means "context requires precedence at least that of the = operator" but why JSOP_DUP for lval?

According to jsopcode.tbl the precedence of JSOP_DUP is 0... but it seems like a funny thing to depend on. JSOP_POP makes more sense to me (that's the opcode for the , operator) but actually just about any precedence will do, since left-hand sides are pretty much all primitive expressions.

Anyway. r=me with that explained. :)
Comment 4 Luke Wagner [:luke] 2012-03-27 11:28:43 PDT
Oh, the only reason I was using JSOP_DUP is that is what was being implicitly passed by the POP_STR() macro before (op == JSOP_DUP).  But JSOP_POP would be clearer, so I'll use that and add a comment.
Comment 6 Marco Bonardo [::mak] 2012-03-29 08:48:30 PDT
https://hg.mozilla.org/mozilla-central/rev/d15eb9db765a

Note You need to log in before you can comment on or make changes to this bug.