352312 - Incorrect decompilation for "new" with unary

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> function() { new (-2) }
function () {
    new -2;
}

js> function () {
    new -2;
typein:4: SyntaxError: syntax error:

Comment 1

[Brendan Eich [:brendan]]

Attachment: Fix JSOP_NEW not to have unary op precedence, and deal with new-of-call-result hard cases

The JS grammar lets you write new foo(bar).baz to call constructor foo with arg bar to create a new object, then get property baz from it.  So over-parenthesizing it (as the decompiler does), that's (new foo(bar)).baz.  But until this patch, a variation of the form new (foo(bar).baz) would decompile without parens, wrongly. That expression is getting baz from the return value of the call foo(bar), then call the value of baz as a constructor with no arguments.

This can't be fixed without a special case, since new foo.baz should not have parens around (foo.baz).  The precedence of the . operator is all that the decompiler for operator new can see in both new (foo.baz) and new (foo(bar).baz). The special case is in JSOP_GROUP, which peeks ahead for JSOP_PUSHOBJ to discern that the parenthesized expression is a callable or constructor.  In all such new or call expressions that explicitly parenthesize the callable or constructor, we now preserve parens.

Fixing new (-x) was simply a matter of splitting precedence levels to give NEW its own level above the unary ops but below the member ops.

This is a safe patch, it should fix all new decompilations (Jesse, please pound on it and shout if you see anything wrong).

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix, v2

With the JSOP_GROUP special case peeking ahead for JSOP_PUSHOBJ, we don't need the previous special case in JSOP_NEW.  The new case is more general whle still unavoiably special due to the operator new grammar (by which new foo(bar).baz means (new foo(bar)).baz).

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 238010 [details] [diff] [review]
fix, v2

I should have noted that JSOP_NEW is given a precedence lower than the call and member operators so that JSOP_CALL binds more tightly than JSOP_NEW.  This is the reverse of the special case.  IOW,

function f(){(new x)(y)}

must not be decompiled into ... new x(y)....

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, combined with patch for bug 352375

To save people applying both from having to resolve conflicts.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v2 updated to merge trunk landing of patch for bug 352375

Comment 6

[Robert Sayre]

Comment on attachment 238020 [details] [diff] [review]
fix, v2 updated to merge trunk landing of patch for bug 352375

Before
-----------

js> x = function() { return new (-2) }   
function () {
    return new -2;
}
js> x = function() { return new (9%2) }  
function () {
    return new 1;
}
js> x = function() { return new 9%2 }    
function () {
    return new 9 % 2;
}

After
----------------
js> x = function() { return new (-2) }
function () {
    return new (-2);
}
js> x = function() { return new (9%2) } 
function () {
    return new (1);
}
js> x = function() { return new 9%2 } 
function () {
    return new (9) % 2;
}

That last one looks wrong.

Comment 7

[Robert Sayre]

Comment on attachment 238020 [details] [diff] [review]
fix, v2 updated to merge trunk landing of patch for bug 352375

nevermind, it's fine.

Comment 8

[Brendan Eich [:brendan]]

Yeah, the call there is (new 9) -- it would be nicer to see than than new (9), but I'm not going to add more special cases.

js> x = function() { return new 9%2 } 
function () {
    return new (9) % 2;
}
js> dis(x)
flags: LAMBDA
main:
00000:  uint16 9
00003:  pushobj
00004:  new 0
00007:  uint16 2
00010:  mod
00011:  return
00012:  stop

Source notes:
  0:     4 [   4] pcbase   offset 4
js> x = function() { return new (9)%2 }
function () {
    return new (9) % 2;
}
js> dis(x)
flags: LAMBDA
main:
00000:  uint16 9
00003:  group
00004:  pushobj
00005:  new 0
00008:  uint16 2
00011:  mod
00012:  return
00013:  stop

Source notes:
  0:     5 [   5] pcbase   offset 5
js> x = function() { return (new 9)%2 }
function () {
    return new (9) % 2;
}
js> dis(x)
flags: LAMBDA
main:
00000:  uint16 9
00003:  pushobj
00004:  new 0
00007:  group
00008:  uint16 2
00011:  mod
00012:  return
00013:  stop

Source notes:
  0:     4 [   4] pcbase   offset 4

/be

Comment 9

[Brendan Eich [:brendan]]

(9), because numbers have artificially lower precedence than member operators, level 16 (above unary ops and below new), in order to decompile 4..toString(16) as (4).toString(16).

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix v3, updated to tip and with code to avoid (yield x); statements

I'm landing with r=sayrer, the yield fix courtesy Jesse.

/be

Comment 11

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 12

[Mike Schroepfer]

Comment on attachment 238101 [details] [diff] [review]
fix v3, updated to tip and with code to avoid (yield x); statements

a=schrep for 181drivers for JS decompiler fuzz bugs.

Comment 13

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 14

[Bob Clary [:bc] (inactive)]

Checking in regress-352312.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352312.js,v  <--  regress-352312.js
initial revision: 1.1
done

Comment 15

[Bob Clary [:bc] (inactive)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 317283 on the branch, apparently.