Decompiler omits sequential-evaluation parens on RHS of destructuring assignment

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: luke)

Tracking

(Blocks: 1 bug, {regression, testcase})

Trunk
mozilla14
x86_64
Mac OS X
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)
(Assignee)

Comment 1

5 years ago
Created attachment 606890 [details] [diff] [review]
fix

Good find!  Who can forget about the precedence of comma in a destructuring assignment.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #606890 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Review ping.  Green on try.
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. :)
Attachment #606890 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d15eb9db765a
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/d15eb9db765a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.