Closed Bug 111816 Opened 23 years ago Closed 19 years ago

Function.prototype.toString() returns extra lines

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: WeirdAl, Assigned: mrbkap)

References

Details

(Keywords: testcase)

Attachments

(4 files, 1 obsolete file)

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.
Attached file Testcase
Keywords: testcase
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.
Assignee: rogerl → khanson
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
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
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.
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
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.
Marking Verified - thanks.
Status: RESOLVED → VERIFIED
*** Bug 140695 has been marked as a duplicate of this bug. ***
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.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
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
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
(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.
Alex: The availability of a workaround does not make a bug invalid.
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.
OS: Windows 98 → All
Attached patch trivial patch (obsolete) — Splinter Review
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.
Assignee: khanson → mrbkap
Status: REOPENED → ASSIGNED
Attachment #186604 - Flags: review?(brendan)
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).
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.)
Attached patch patch v2Splinter Review
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.
Attachment #186604 - Attachment is obsolete: true
Attachment #187041 - Flags: review?(shaver)
Attachment #186604 - Flags: review?(brendan)
Comment on attachment 187041 [details] [diff] [review] patch v2 r=shaver, let's get brendan's confirmation.
Attachment #187041 - Flags: superreview?(brendan)
Attachment #187041 - Flags: review?(shaver)
Attachment #187041 - Flags: review+
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
Attachment #187041 - Flags: superreview?(brendan)
Attachment #187041 - Flags: superreview+
Attachment #187041 - Flags: approval1.8b3+
Fix checked in (with Brendan's comments addressed).
Status: ASSIGNED → RESOLVED
Closed: 23 years ago19 years ago
Resolution: --- → FIXED
QA Contact: pschwartau → bob
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.
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
Status: RESOLVED → VERIFIED
Flags: testcase+
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.
Blocks: 301869
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: