Closed Bug 352026 Opened 13 years ago Closed 13 years ago

Decompiler doesn't put parens around yield expression in argument list, and compiler doesn't like that

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, verified1.8.1)

Attachments

(1 file)

js> function() { z((yield 3)) }
function () {
    z(yield 3);
}
js> function () {
    z(yield 3);
typein:5: SyntaxError: yield expression must be parenthesized:

I'm not sure exactly what the bug is...
(1) The compiler should accept "z(yield 3)"
(2) The decompiler should keep the extra parens.

If it's (2), then probably also...
(3) When the compiler rejects "z(yield 3)", it should give an error message that's easier to understand than "yield expression must be parenthesized".  It looks like it's parenthesized in this example!
Python says the bug is (2).  Anent the diagnostic, you should know that Python just says "syntax error" for f(yield x) ;-).

If we follow Python, double parens are mandatory in a call -- f((yield x)).  This seems wrong to me.  Comments?

/be
Assignee: general → brendan
Blocks: geniter
OS: Mac OS X 10.4 → All
Priority: -- → P2
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
(In reply to comment #1)
> If we follow Python, double parens are mandatory in a call -- f((yield x)). 
> This seems wrong to me.  Comments?

In Python, if no parens were required, |f(yield x, y)| and |f((yield x), y)| would have different meaning; the former yields a tuple (I think).  Requiring parens for |f((yield x))| is probably a consequence of requiring them as |f((yield x), y)|, since if |f(yield x)| is fine and |f| changes from a single-arg function to a two-arg function, you can't just add |, y| and have it just work.

If we already require parens for |f(yield x, y)|, I'd say require them for |f(yield x)|, for consistency.
(In reply to comment #2)
> Requiring
> parens for |f((yield x))| is probably a consequence of requiring them as
> |f((yield x), y)|, since if |f(yield x)| is fine and |f| changes from a
> single-arg function to a two-arg function, you can't just add |, y| and have it
> just work.

Sure.  Actually it all falls out of the grammar.  I understand why it works that way, but it's not obvious that users will *want* it to work that way.  But for now let's stick with the Pythonic grammar.

The decompiler needs help anyway, and not just for yield.  Consider:

js> function f(){g((let(a=b)c,d),e)}
js> f
function f() {
    g(let (a = b) c, d, e);
}

See bug 352084 (Jesse beat me to it).

/be

Blocks: 352084
Status: NEW → ASSIGNED
1.  The jsopcode.tbl changes give JSOP_INITPROP, JSOP_INITELEM, and JSOP_ARRAYPUSH all the assignment opcode precedence level, 3.

2.  JSOP_POP/JSOP_POPV formatting for expression statements must parenthesize any expression starting with '{'.

3.  JSOP_YIELD must parenthesize any yield operand staring with "yield ".

4.  Actual argument list elements are ECMA-262 AssignmentExpressions (AssignExpr in jsparse.c terms), so op must be an assignment op before we POP_STR them -- use JSOP_SETNAME as canonical assignment opcode here.

5.  JSOP_ENDINIT needs high precedence, and has it now thanks to bug 351625, but its "input precedence" should be 0, to turn off parens.  It closes a bracketed form which should never be auto-parenthesized.

I verified by looking through all calls to AssignExpr in jsparse.c that we either use the right precedence by default (yay, opcode table precedence), or force it (in the actual argument popping case) in the decompiler.

/be
Attachment #237772 - Flags: review?(mrbkap)
Attachment #237772 - Flags: review?(sayrer)
Attachment #237772 - Flags: review?(mrbkap) → review+
Comment on attachment 237772 [details] [diff] [review]
fix precedence-driven parenthesization

this patch seems to work as advertised, but I'm not sure why the let block below compiles differently if you surround it with parens, assign it, or return it.

js> var y = function() { let(s=4){foo:"bar"} }   
js> y
function () {
    let (s = 4) {
    foo:
        "bar";
    }
}
js> dis(y)
flags: LAMBDA
main:
00000:  enterblock depth 0 {s: 0}
00003:  uint16 4
00006:  setlocal 0
00009:  pop
00010:  nop
00011:  string "bar"
00014:  pop
00015:  leaveblock 1
00018:  stop

Source notes:
  0:     9 [   9] xdelta  
  1:     9 [   0] decl     offset 5
  3:    10 [   1] label    atom 1 (foo)
js> var x = function() { (let(s=4){foo:"bar"}) }
js> x
function () {
    let (s = 4) ({foo:"bar"});
}
js> dis(x)
flags: LAMBDA
main:
00000:  enterblock depth 0 {s: 0}
00003:  uint16 4
00006:  setlocal 0
00009:  pop
00010:  name "Object"
00013:  pushobj
00014:  newinit
00015:  string "bar"
00018:  initprop "foo"
00021:  endinit
00022:  leaveblockexpr 1
00025:  group
00026:  pop
00027:  stop

Source notes:
  0:     9 [   9] xdelta  
  1:     9 [   0] decl     offset 12
js>
Attachment #237772 - Flags: review?(sayrer) → review+
(In reply to comment #5)
> (From update of attachment 237772 [details] [diff] [review] [edit])
> this patch seems to work as advertised, but I'm not sure why the let block
> below compiles differently if you surround it with parens, assign it, or return
> it.

See http://developer.mozilla.org/es4/proposals/block_expressions.html and http://developer.mozilla.org/presentations/xtech2006/javascript/ -- let block vs. let expression.

/be
Comment on attachment 237772 [details] [diff] [review]
fix precedence-driven parenthesization

Fixed on trunk.

/be
Attachment #237772 - Flags: approval1.8.1?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 352011
Blocks: 352025
Comment on attachment 237772 [details] [diff] [review]
fix precedence-driven parenthesization

a=schrep
Attachment #237772 - Flags: approval1.8.1? → approval1.8.1+
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1
Checking in regress-352026.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-352026.js,v  <--  regress-352026.js
initial revision: 1.1
done
Flags: in-testsuite+
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Status: RESOLVED → VERIFIED
updated test to uniquely identify subtests: http://hg.mozilla.org/mozilla-central/rev/09fac5307612
You need to log in before you can comment on or make changes to this bug.