Closed Bug 76634 Opened 24 years ago Closed 24 years ago

Has Function.prototype.toString() changed?

Categories

(Core :: JavaScript Engine, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: pschwartau, Assigned: brendan)

Details

(Keywords: js1.5)

Attachments

(2 files)

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 -)
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.
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
Assignee: rogerl → brendan
Severity: normal → critical
Keywords: js1.5, mozilla0.9
Priority: -- → P1
Target Milestone: --- → mozilla0.9
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
Status: NEW → ASSIGNED
Attached patch proposed fix (deleted) — Splinter Review
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
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.)
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 -
Attached patch comment for shaver (deleted) — Splinter Review
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())}
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
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 -
r=beard
a=asa for checkin to 0.9
Fix is in. /be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Testcase js/tests/js1_4/Regress/function-002.js has been corrected. The testcases are now passing on Linux and WinNT. Marking Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: