Bug 4698 – kjs does not allow named functions in function expressions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Arthur Langereis]

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.

[Arthur Langereis]

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.

[Arthur Langereis]

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.

[Darin Adler]

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?

[Arthur Langereis]

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)

[Darin Adler]

Comment on attachment 3630 [details]
updated patch + testcase

r=me

[Arthur Langereis]

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.

[Darin Adler]

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)

[Alexey Proskuryakov]

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