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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

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!

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Jeff Walden [:Waldo]]

(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.

Comment 3

[Brendan Eich [:brendan]]

(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

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix precedence-driven parenthesization

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

Comment 5

[Robert Sayre]

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>

Comment 6

[Brendan Eich [:brendan]]

(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 7

[Brendan Eich [:brendan]]

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

Fixed on trunk.

/be

Comment 8

[Mike Schroepfer]

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

a=schrep

Comment 9

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 10

[Bob Clary [:bc:]]

Checking in regress-352026.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-352026.js,v  <--  regress-352026.js
initial revision: 1.1
done

Comment 11

[Bob Clary [:bc:]]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux

Comment 12

[Bob Clary [:bc:]]

updated test to uniquely identify subtests: http://hg.mozilla.org/mozilla-central/rev/09fac5307612