111816 - Function.prototype.toString() returns extra lines

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alex Vincent [:WeirdAl]]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.6+) Gecko/20011123
BuildID:    Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.6+) Gecko/20011123

I've been having inconsistencies with the length of a function in characters as
long as I can remember, but it turns out to be slightly more serious.  

When I call toString() on a function, the response has an extra line before and
after the function body.  This is causing me problems with regard to editing
functions and copies of script source code retrieved from the DOM.

Reproducible: Always
Steps to Reproduce:
1.  Run attached testcase (coming up)
2.
3.

Actual Results:  An alert revealing six asterisked lines:  one before the
function, the four function lines, and one after the function.

Expected Results:  An alert revealing four asterisked function lines.

The character count from Function.prototype.toString() has been incorrect for as
long as I can remember.  When I try to manipulate a function's source code from
JavaScript, it invariably causes problems.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: Testcase

Comment 2

[Phil Schwartau]

cc'ing Brendan for a final judgement -

This bug might be invalid. Function.prototype.toString is
implementation-dependent. As I understand it, is free to return  
anything that recompiles to the function itself. Compare bug 76634,
"Has Function.prototype.toString() changed?"

Note: the Rhino shell does the same as SpiderMonkey is doing:
it adds "\n" both before and after the function declaration.


From ECMA:

15.3.4.2 Function.prototype.toString ( )

An implementation-dependent representation of the function is returned. 
This representation has the syntax of a FunctionDeclaration. Note in
particular that the use and placement of white space, line terminators,
and semicolons within the representation string is implementation-dependent.

Comment 3

[Brendan Eich [:brendan]]

ajvincent: what problems are you having?  Now that you know about the "extra"
newlines, can't your script cope?

This bug is invalid, for the reason Phil gave.

/be

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: Script depending on Function.prototype.toString().length

In that case, I respectfully request this bug be reopened as a request for
enhancement, with severity set appropriately.

Brendan, for my scripts to cope, I will have to create implementation-dependent
forks in my ECMAScript code.

The above attachment, in theory, should perform a bit of cleanup on its own
code -- moving the top-level commands I give in it into an initialization
function.  This works by first removing all the functions the script defines
first.	Without an accurate line count, that becomes problematic at best.  I
really do not look forward to the idea of having to sniff the userAgent in
order to determine how many lines or newline characters to remove for
consistency in my own scripts.

Comment 5

[Brendan Eich [:brendan]]

First, saying "I will have to create implementation-dependent forks in my
ECMAScript code" when the topic (Function.prototype.toString()'s return value)
is explicitly implementation-dependent in the ECMA spec is a way of saying "I
will follow the spec."

I presume your script works as desired in IE because IE uses source text
recovery via the DOM, and not bytecode decompilation, to compute the result of
Function.prototype.toString().  SpiderMonkey uses bytecode decompilation, and
returns a string containing a canonical, comment-free version of the function. 
This satisfies the ECMA spec, but you can't use the length of the returned
string to index further into the DOM-reflected script node source text.  The two
strings have no such "linear" relationship to one another.

I suggest that if you want to pull out function source from a DOM script node's
text, that you parse each function at least enough to balance {  and } in each
function's body (beware commented-out braces!).

I'm still not clear on *why* you need to do all this work, however -- can you
clue me in?  Thanks.

/be

Comment 6

[Alex Vincent [:WeirdAl]]

Okay, okay.  I'll back off.

This came about because I like to play around with function source codes in
strings.  It's not just because of the DOM.  If I want to dynamically change a
function's source code using JavaScript (say, if I built a JS function editor
using JS only), it's nice to have the actual length of the function.

And forget the part about implementation-dependent forks.  I just now realized
that I could use String.prototype.lastIndexOf("}") to find the end of the
function, and
String.prototype.indexOf(Function.prototype.toString().match(/\S/)) to find the
beginning.

In other words, I'm not using my head.  Sorry for the spambug, everyone.

Comment 7

[Phil Schwartau]

Marking Verified - thanks.

Comment 8

[Phil Schwartau]

*** Bug 140695 has been marked as a duplicate of this bug. ***

Comment 9

[Jesse Ruderman]

This bug doesn't cause Mozilla to violate ECMA specs, but that doesn't make it
invalid.  Adding newlines between statements and fixing indentation makes sense,
but adding newlines above and below the function just makes the output from
scripts like mine ugly.

Comment 10

[Brendan Eich [:brendan]]

jruderman: Scripts like what?  You didn't attach the Testcase.

I'm an inch away from WONTFIXing if we can't agree on INVALID.  But I argue that
this bug as stated by its reporter is INVALID, and I suggest that you file a new
one rather than morph this one.

/be

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Brendan, Jesse did file another bug.  It was the one which was recently duped to
this one.  See bug 140695.

The testcase is the "view variables" bookmarklet available from
http://www.squarefree.com/bookmarklets/webdevel.html

Comment 12

[Alex Vincent [:WeirdAl]]

(1) Does this bug violate a standard?  No.  The standard allows Mozilla to do this.

(2) Does this bug have an adverse effect on Mozilla suite?  No.  Mozilla works
just fine without fixing this.

(3) Is there an easy workaround for this bug?  Yes.  Just use regular
expressions and string functions as I describe in comment 6.

The first step to confirming there is a bug in someone else's work is confirming
there are no bugs in your own.  Jesse, your bookmarklet is already over eight
hundred characters long; how hard would it be for you to add the code needed to
work around it, given the suggestion I made earlier?

Recommend INVALID and let this bug die.

Comment 13

[Jesse Ruderman]

Alex: The availability of a workaround does not make a bug invalid.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes the standard does allow us to return whatever we want, but it seems awfully
strange for the first character to be a newline.  Just like it seems strange for
the first 5 characters of this comment to be newlines.

I believe that all Jesse, and this bug, ask for is consistency.  Jesse, can you
write up a straightforward comparison testcase which does toString() outputs
side by side for each JS core data type?  Eg. Number, Date, Object, Function,
etc. I see the problem when using the bookmarklet, but that involves getting the
bookmarklet, finding a page which will demonstrate it and then running the
bookmarklet.  It would be great if we could just have this all as an attachment.

I believe the original report is the same as what Jesse is asking for and that
it is a valid request.  Regarding regular expressions as mentioned by Alex in
comment 12, there should be no need to do replaces on the string representation
of a variable.  The string representation should be useful itself as-is.  If it
is not, then that is a problem, and is the one being discussed here.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: trivial patch

After reading this bug, it's clear that there needs to be some resolution. This
trivial patch makes us not output the leading and trailing newlines. Brendan,
feel free to r-, in which case this should be WONTFIX, I think.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Brendan asked me to go through CVS blame and see if there were any clues as to
why the newlines were put in, but except for a trivial whitespace change (in the
source!), those lines haven't been touched since March, 1998. I think by this
point it comes down to taste (or WONTFIX if we opt for consistency).

Comment 17

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

My dim recollection is that this change went in when we added Script, to avoid
having function decls all jammed up against other top-level statements, but it
also applies to the decompilation of single functions, or functions nested
inside other functiony objects.

My preference would be to see us strip the newlines from the function
decompilation, and instead put them in the code for SRC_FUNCDEF and suchlike, I
guess.

(I've heard requests in the past that function decompilation result in the
shortest possible equivalent function, even to the extent of renaming variables
where that can be proved safe.  My purist instinct tells me that I should
support WONTFIXing this bug, because people should not have specific
expectations of the source produced by Function.prototype.toString unless they
are looking to parse JS.  Certainly, future changes to the engine might involve
us simply capturing the source of the declaration itself, in which case you will
get whitespace that is to the author's taste, and not according to any reliable
algorithm.

But for all that, I think the extra newlines are kinda ugly.)

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

By moving the newlines into Decompile(), they decorate functions only in
Scripts, instead of every function. This isn't the optimal solution (there are
still some extra newlines in the js shell when using new Script().toString()),
but other solutions would likely be much more complicated.

Comment 19

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

Comment on attachment 187041 [details] [diff] [review]
patch v2

r=shaver, let's get brendan's confirmation.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 187041 [details] [diff] [review]
patch v2

>                   do_function:
>                     obj = ATOM_TO_OBJECT(atom);
>                     fun = (JSFunction *) JS_GetPrivate(cx, obj);
>                     jp2 = js_NewPrinter(cx, JS_GetFunctionName(fun),
>                                         jp->indent, jp->pretty);
>                     if (!jp2)
>                         return JS_FALSE;
>                     jp2->scope = jp->scope;
>+                    js_puts(jp2, "\n");
>                     if (js_DecompileFunction(jp2, fun)) {
>+                        js_puts(jp2, "\n");
>                         str = js_GetPrinterOutput(jp2);
>                         if (str)
>                             js_printf(jp, "%s\n", JS_GetStringBytes(str));
>                     }
>                     js_DestroyPrinter(jp2);

Hrm, bad old code (from me?!) doesn't propagate a js_DecompileFunction failure.
 Do that by setting ok and testing it after the js_DestroyPrinter(jp2) to
return JS_FALSE early if !ok, and sr+a=me.

/be

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in (with Brendan's comments addressed).

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: changes to existing tests

This change requires the following js test cases to be modified.

mozilla/js/tests/js1_2/function/Function_object.js
mozilla/js/tests/js1_2/function/tostring-1.js
mozilla/js/tests/js1_2/function/tostring-2.js
mozilla/js/tests/js1_5/Regress/regress-245795.js
mozilla/js/tests/js1_5/Regress/regress-252892.js

This will cause non-trunk builds and perhaps rhino to fail these tests. If no
one has an objection, I will commit these before the end of the day.

Comment 23

[Bob Clary [:bc] (inactive)]

Slightly modified form of patch to tests checked in.

/cvsroot/mozilla/js/tests/js1_2/function/Function_object.js,v 
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/js/tests/js1_2/function/tostring-1.js,v
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/js/tests/js1_2/function/tostring-2.js,v
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-245795.js,v
new revision: 1.3; previous revision: 1.2

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-252892.js,v
new revision: 1.2; previous revision: 1.1

Comment 24

[timeless]

this change also breaks any code which naively does:
"Foo.prototype.bar="+bar+
"Foo.prototype.baz="+baz;
now you could say i was asking for it. and i was. and i'm not asking for this 
to be reverted since i just added some code to deal with it.