76634 - Has Function.prototype.toString() changed?

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Phil Schwartau]

These testcases are erroring because Function.prototype.toString() 
is not behaving as it used to:

               js1_2/function/Function_object.js
               js1_2/function/tostring-1.js
               js1_4/Regress/function-002.js


For example, the first two tests fail because of this:

js> var f = new Function()
js> f.toString()

(function () {
})


The testcases expect

function () {
}


Meanwhile, js1_4/Regress/function-002.js fails because of this:

js> print(f2.toString())

function f2() {
    var y;
    f1(1, 2), y = (function g(x) {return Math.exp(x);});
    print(y.toString());
}


The testcase expects no line breaks:

js> print(dec2)
function f2(){var y; f1(1,2), y=function g(x){return Math.exp(x);}; 
print(y.toString())} 

(Of course, Bugzilla is wrapping this text -)

Comment 1

[Phil Schwartau]

Also note the extra spaces being inserted in f2.toString() above:

e.g. f1(1, 2)
vs.  f1(1,2)

and  y = (function g(x) etc. 
vs.  y=function g(x)    etc.

Comment 2

[Brendan Eich [:brendan]]

Argh.  I broke ECMA-262 15.3.4.2 in trying to fix bug 73760.  This is a
standards conformance regression that adds noise to the js/tests suite, so I'd
like to fix for 0.9.  Patch coming up.

/be

Comment 3

[Brendan Eich [:brendan]]

phil, the decompiler has *always* put spaces on either side of =, and after
commas in arg lists.  Since 1995, so I don't think the test should expect to get
back whatever white space style it used in its source.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 5

[Brendan Eich [:brendan]]

I was running the testsuite these past few days, but I had convinced myself that
ECMA allowed enough latitude in Function.prototype.toString that the tests were
wrong and the change checked in to fix bug 73760 were right.  But ECMA requires
an implementation-dependent string that parses as a FunctionDeclaration, and the
parentheses break that.

The patch distinguishes toSource from toString down in the bowels of the
decompiler, by testing for the don't-pretty-print flag.  Shaver loves my old
decompiler, so he gets sr= duties.  Beard, if rogerl is not around, can you r=
today, for 0.9?  Thanks,

/be

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Boy, I can't get enough of that decompiler.  sr=shaver.  (But a comment in the
code to indicate why you're checking ->pretty would be very helpful the next
time someone other than you has to wade into that code.)

Comment 7

[Phil Schwartau]

Brendan: thank you for pointing out the whitespace behavior. On closer
examination of the js1_4 testcase, I see it actually compares these:

          StripSpaces(f2.toString()) == StripSpaces(dec2));


where StripSpaces() strips out spaces, carriage returns, etc.
Sorry about that -

Comment 8

[Brendan Eich [:brendan]]

Attachment: comment for shaver

Comment 9

[Phil Schwartau]

With patch id=31445 , the two js1_2 testcases no longer error. 
But the js1_4 testcase still fails:

[//d/JS_EXP/mozilla/js/src/WINNT4.0_DBG.OBJ]
$ ./js
js> load('../../tests/js1_4/shell.js')
js> load('../../tests/js1_4/Regress/function-002.js')

BUGNUMBER: 330462
function-002.js Regression test case for 325843
typeof f1 = function PASSED!
f1.toString() == dec1 = true PASSED!
typeof f2 = function PASSED!
f2.toString() == dec2 = false FAILED! expected: true

js> StripSpaces(f2.toString())
functionf2(){varyf1(1,2),y=(functiong(x){returnMath.exp(x)})print(y.toString())}
js> StripSpaces(dec2)
functionf2(){varyf1(1,2),y=functiong(x){returnMath.exp(x)}print(y.toString())}

Comment 10

[Brendan Eich [:brendan]]

Phil: that test is requiring something that ECMA-262 Edition 3 15.3.42. does
not: that the outer function's toString use Function.prototype.toString on the
inner function when decompiling or otherwise recovering the source.  There is no
requirement.  The decompilation of f2 is free to parenthesize lambda expressions
such as the one assigned to var y, named function g.

You'll need to tweak that test, somehow.  Short of making it allow for parens
around nested function expressions, what should it do?  What is it really trying
to test that's not covered by other tests?

/be

Comment 11

[Phil Schwartau]

The body of that outer function is just something I made up to replace:

                  dec2 = "function f2(){1,2}"; 
 
I was just writing anything that had some complexity. I will tweak the test -

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr=shaver (again), thanks.

Comment 13

[Patrick C. Beard]

r=beard

Comment 14

[Asa Dotzler [:asa]]

a=asa for checkin to 0.9

Comment 15

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 16

[Phil Schwartau]

Testcase js/tests/js1_4/Regress/function-002.js has been corrected. 
The testcases are now passing on Linux and WinNT. Marking Verified.