kjs doesn't allow named functions used for Function expressions, even though the ECMA-262 specification states: FunctionExpression : function Identifier opt ( FormalParameterList opt ) { FunctionBody } (Page 71 of ECMA-262 spec: http://www.ecma-international.org/publications/files/ECMA-ST/ Ecma-262.pdf) The identifier is optional, not forbidden. FF and IE also support named functions in function expressions.
Created attachment 3615 [details] preliminary patch for allowing named functions Now added to the right bug *cough*. In any case, this patch adds support for optionally named function expressions. It, however, causes a kjs test to fail: js1_5/Regress/regress-114491.js The test is supposed to fail because it does the following: if(true) function f(){}(); The test caused a crash in an early version of mozilla, but it should fail because the function expression should be parenthesized for the call syntax to be correct. Safari, however, failed the test because they used a named function expression, which it did not support. Safari 2.0 will accept and run this: function(){}(); which generates an error in Moz and IE. It should be (function(){})(); I'm still working on how to fix this requirement in grammar.y.
Created attachment 3625 [details] expanded patch implementing full ecma behavior for named FunctionExpr This patch puts a named function expression inside a new anonymous object so it can call itself recursively, but it won't register with the current scope, per ECMA spec. Unnamed function expressions are handled as before. A fix for the regression is not in this patch. However, after some more investigation it seems that there is a more fundamental flaw at work here. KJS allows unnamed function declarations and calls them at the end of the script block. An unnamed function declaration is a syntax error. KJS may not properly differentiate between func decls and exprs. This may have to be handled in a different bug.
Comment on attachment 3625 [details] expanded patch implementing full ecma behavior for named FunctionExpr This patch looks great. Here are my comments: A) There's no test case attached to this bug. We can't land this change without a test. B) Formatting-wise, the longer line in the yacc source file should follow the approach used elsewhere -- putting the C code on a separate line so that the left braces line up. C) In FuncExprNode::evaluate: + bool named = (ident != Identifier::null()); should be: bool named = !ident.isNull(); D) For null pointers in new code, I prefer NULL over 0, and 0 over 0L. So we definitely shouldn't initialize namedContainer to 0L. E) I slightly prefer just plain "new ObjectImp" to "new ObjectImp()". F) I think perhaps the member of FuncExprNode shoudl be named "name" rather than "ident". Would we refer to this as the name of the function in the expression or the identifier of the function? G) The code that makes the property DontDelete unless the codeType is EvalCode is confusing to me. Perhaps we could include a comment explaining that?
Created attachment 3630 [details] updated patch + testcase I applied changes B-E, added a /fast/js testcase that validates and runs successfully in the webkit tests and we agreed upon keeping the name "ident". Only thing remaining is an explanation of why DontDelete is turned off for eval code. It's not really important in this case as the anonymous container in which the function is stored is completely inaccessible and expires immediately after the function exits, but knowing the rationale behind it would be nice, then the comment in FunctionDeclaration can also be updated. (it now points simply to ECMA 10.2.2)
Comment on attachment 3630 [details] updated patch + testcase r=me
Created attachment 3645 [details] previous patch + fix for func decl/expr mixup + updated testcase This patch takes the previous patch and adds a fix for a bug in kjs where it would treat an unnamed function declaration as a function expression. The updated testcase adds a test for this behavior. It was decided to add this fix to this bug as these two problems are dependent on eachother. KJS's grammar defined a FunctionDeclaration simply as a function with an identifier and a FunctionExpr as a function without one. The second rule was incomplete and fixed with my previous patch, this in turn allowed this other behavior to be fixed by explicitly defining an unnamed separate function definition as illegal in the syntax and then checking for a function declaration before other SourceElement types. This works well, causes no regressions and puts KJS's function parsing on par with FF's engine and the ECMA spec.
Comment on attachment 3645 [details] previous patch + fix for func decl/expr mixup + updated testcase r=me (note, you shouldn't put yourself as the requestee -- you're the requester)
*** Bug 9495 has been marked as a duplicate of this bug. ***