352013 - More incorrect decompilation with "new" operator (e4x/decompilation/regress-352013.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

js> function() { new (x(z))(w) }
function () {
    new x(z)(w);
}

js> function () {
    new x(z)(w);
}
function () {
    (new x(z))(w);
}

Comment 1

[Brendan Eich [:brendan]]

Where's the bug?

js> x = Function
function Function() {
    [native code]
}
js> z = "print(arguments[0])"
print(arguments[0])
js> w = 42
42
js> new x(z)(w)
42
js> (new x(z))(w)
42
js> new (x(z)(w))
42
typein:6: TypeError: x(z)(w) is not a constructor

See ECMA-262 Ed. 3 11.2, the grammar is tricky.  The upshot is that you can string property operators together till you get to an argument list, and once you've done that, no matter the space between the new and the MemberExpression Arguments, you're calling that constructor.

Any Arguments appended to that form a CallExpression.

/be

Comment 2

[Jesse Ruderman]

The first decompilation is incorrect; you forgot to test the original.  (Round-trip changes are often a hint that the original decompilation is incorrect.)

js> new (x(z))(w)
42
[object Object]

js> new x(z)(w)
42

js> function() { h = new (x(z))(w) }
function () {
    h = new x(z)(w);
}

Comment 3

[Brendan Eich [:brendan]]

Sorry, taking.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

Jesse, if you could test this, I'd appreciate it.

/be

Comment 5

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 6

[Mike Schroepfer]

Comment on attachment 237849 [details] [diff] [review]
fix

a=schrep

Comment 7

[Jesse Ruderman]

Are you sure you checked this in?  I still see the bug, and I don't see a checkin referencing this bug number.

Comment 8

[Brendan Eich [:brendan]]

Sorry, I did forget to check it in.  Done now:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.175; previous revision: 3.174
done


/be

Comment 9

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

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

Comment 11

[Bob Clary [:bc] (inactive)]

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

Comment 12

[Bob Clary [:bc] (inactive)]

this regressed on 1.8, 1.9 20061002

expect: function ( ) { new ( x ( z ) ) ( w ) ; }
actual: function ( ) { new x ( z ) ( w ) ; } 

file new bug or reopen this?

Comment 13

[Bob Clary [:bc] (inactive)]

still failing 1.8, 1.9 20061009. reopening and clearing verified flags to get an answer.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> still failing 1.8, 1.9 20061009. reopening and clearing verified flags to get
> an answer.

Sorry I missed the bugmail from the previous comment.  This is correct, the input expression is overparenthesized.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.2; previous revision: 1.1
done

Comment 16

[Jesse Ruderman]

No, the previous version of the testcase was correct.  "new ( x ( z ) ) ( w )" is not overparenthesized.

js> x = Function; z = "print(arguments[0])"; w = 42;
42
js> f = function() { return new ( x ( z ) ) ( w ) ; }
function () {
    return new x(z)(w);
}
js> f()
42
[object Object]
js> (eval("" + f))()
42

Comment 17

[Bob Clary [:bc] (inactive)]

ok. I'll let you two fight it out. ;-)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Brendan, does this actually block 1.8.1, remembering that we're trying to keep the RC3 baseline set to only-critical, not-ridealong patches? Can this wait for 1.8.1.1?

Comment 19

[Brendan Eich [:brendan]]

This bug's patch did land on the 1.8 branch already:

revision 3.89.2.49
date: 2006/09/12 23:37:34;  author: brendan%mozilla.org;  state: Exp;  lines: +2
1 -3
Fix 352013, a=schrep.

Obviously it wasn't the full fix.  Jesse, sorry about the deja vu all over again.  We can leave this for 1.8.1.1.

/be

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to comment #19)
> This bug's patch did land on the 1.8 branch already:

yes and it was fixed and verified:

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

now it has regressed on 1.8 and 1.9

reverting test.
Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.3; previous revision: 1.2
done

if this bug isn't fixed on 1.8, it shouldn't have the fixed1.8.1 keyword imo.

Comment 21

[Brendan Eich [:brendan]]

Anyone know what checkin regressed this?

/be

Comment 22

[Bob Clary [:bc] (inactive)]

(In reply to comment #21)

the rough range is 20060930-20061002. The only checkin to jsopcode.c in that range is bug 355004 but that is a wild *** guess.

Comment 23

[Daniel Veditz [:dveditz]]

This seems to have gotten forgotten. Marking a blocker so Brendan can decide whether this is actually important for the branch or whether we clear the "wanted" flag

Comment 24

[Brendan Eich [:brendan]]

I see why I was confused. Comment 16's function assigned to f is overparenthesized but this one is not:

f = function() { return new ( x ( z ) ( w ) ); }

Yet it decompiles without the necessary parens around the new operand:

function () {
    return new x(z)(w);
}

Bob, that says a test is wrongly requiring parens where they are unnecessary, and we are missing a different test.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: second fix

This really was a separate bug; attaching here to keep it all together with the comments.

/be

Comment 26

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.4; previous revision: 1.3

Comment 27

[Brendan Eich [:brendan]]

The test covers new (x(z))(w), (new x(z))(w), new (x(y)(z)), new (x(y).z), and (new x(y)).z, all good. For complete coverage, you'd want the "left", "middle", and "right" associated new forms for other "addressing modes":

[z]
.@a
.@n::a
.n::z
.n::[z]

"left" is (new x(y))..., "middle" is new (x(y))..., and "right" new (x(y)...).

/be

Comment 28

[Brendan Eich [:brendan]]

Second fix landed on trunk:

js/src/jsopcode.c 3.223

Thoughts on branch approval likelihood?

/be

Comment 29

[Bob Clary [:bc] (inactive)]

Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/e4x/decompilation/regress-352013.js,v  <--  regress-352013.js
initial revision: 1.1

Comment 30

[Bob Clary [:bc] (inactive)]

verified fixed on the trunk with both tests.