Bug 27226

Summary: Possibly faulty handling of function statements enclosed in other statements (i.e. block, if, etc). JavaScriptCore test ecma_3/FunExpr/fe-001.js failed.
Product: WebKit Reporter: Carol Szabo <carol>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: barraclough, brendan, cscott, dp+webkit+bugzilla, ggaren, kangax, msaboff, oliver, syoichi, wingo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 65546    

Description Carol Szabo 2009-07-13 11:44:02 PDT
Despite the fact that section 10.6 of the ECMA 5th edition specifies that function declarations in a certain scope have to be added to the Execution environment when the environment is created, thus before any code in the scope is executed, which is in accordance with the way JavaScriptCore acts, section 13 of the same document when explaining how the process is executed may be interpreted to mean that function definition statements nested inside other statements should not be added to the execution environment until actually executed.
This later behavior is tested by ecma_3/FunExpr/fe-001.js an implemented by Firefox.
In case this second behavior is the desired behavior for WebKit (which I doubt since it is inconsistent with the variable declaration behavior, which adds all vars to the execution environment before any code is executed regardless of whether the declaration is nested in a statement such as an if or block statement) the test is valid and the code should be fixed.
In case the current behavior is the desired one, the test should probably be skipped.
Comment 1 Daniel Parks 2010-09-01 19:32:05 PDT
I just ran into this. I find the current behavior very surprising, since it means that

var foo = function () {}

is different than

function foo () {}

Note also that this is not consistent with other variable types — the foo variable is not only added to the scope (like other variables), but it's also initialized (too early).

The current behavior seems very un-JavaScript-like. Functions are otherwise not "special", and this can create very confusing bugs. For example, I recently tried to do the following in a library shared with web workers:

if ( self.document ) {

}
Comment 2 Daniel Parks 2010-09-01 19:35:27 PDT
ARGH. Tab leaves the textarea. The final bit:

if ( self.document ) {
  function importScripts () { ... }
}

Naturally this broke all the calls to importScripts.

Also, this brings up another point: a conditional around a importScripts() or similar call is now different than a conditional inside the included script. (Aside from the issue of the actual inclusion, naturally.)
Comment 3 C. Scott Ananian 2012-02-10 12:30:01 PST
Another interesting case:

function foo() {
  try {
    throw 1;
  } catch (e) {
    function f() { return e; }
    return f();
  }
}

This function returns 1 as expected on rhino and Firefox.  On webkit (node, chrome, etc) it throws "ReferenceError: e is not defined", because the function definition has been hoisted out of the catch block.
Comment 4 Oliver Hunt 2012-02-10 12:54:17 PST
Alas this is one of those icky bits in ecmascript, where we have been unable to define a common set of semantics for nested functions (they're prohibited in strict mode).

This is behaviour we can't change, but the basic problem is that the grammar does not _allow_ nested function statements.  Alas as with so many things in ES a few implementation bugs crept into an implementation that allowed nested function statements.  This was then picked up in various ways by the various JS engines (sans V8, which targeted bug-for-bug compat with JSC, at least initially).

So once you allow nested function statements you hit a "fun" problem.  The ES spec defines the behaviour of a function statement as being hoisted to the head of the function code.  Although a function declaration is similar to a var declaration (in the sense that they're both simply defining a variable with a local environment binding) function declarations are expected to hoist the definition as well.  That allows code like this to work:

f();
function f() { alert("yay!") }

But if you nest the function statement in a new scope (with(), catch(), etc) you get an interesting problem:
When you go to initialize the function the scope chain for the function isn't there yet.  So you can either break spec (again) and delay function initialization in that case (which i assume SM must do) or you accept that like so much else in ES, you should _ignore_ lexical scope.  Some engines do it one way, some do it the other (and there may even be an additional set of idiosyncrasies in another engine).

We tried to standardize this in TC39 (the ES standardization committee), but realized that the behaviors are mutually incompatible, that there is no differing semantic that is mutually compatible, and that there are sites that depend (hilariously) on the different implementations in order to work correctly, eg.

if (somethingThatIsTrueInMozillaAndWebKit())
    function foo() { somethingThatOnlyWorksInMozilla(); }
else
    function foo() { somethingThatWorksInIEAndWebKit(); }

In Mozilla this would work as expected (due to SM function statement semantics), it works in IE and WebKit because the second definition of foo() overwrites the first.  The real killer is that WebKit gets the second function, even though it ostensibly executes the true part of the if() statement.

The net result is that if IE and Webkit picked up the SM behaviour, this site would stop working in webkit but still work in IE and Mozilla.  If Mozilla moves to the behaviour of IE and WebKit this site will stop working in Mozilla (as they'll get the second IE/WebKit only function instead of the mozilla one).  There are alternative pieces of code that would work in both mozilla and webkit while breaking IE (WebKit is a chameleon, due to the large amounts of IE and Firefox specific code that exists we've got lots of the "features" of both of them so can run some "IE" only content and some "Mozilla" only content).

The end result was the realization that we simply could not standardize this behaviour, and so the ES5.1 spec now specifically calls out nested functions as being a place where there are dragons, and explicitly disallows them in strict mode.
Comment 5 Oliver Hunt 2012-02-11 10:18:38 PST
The other scenario that exists:

if (somethingThatIsTrueInAllCurrentBrowsers()) // Filter out IE using something that IE has now implemented
    function foo() { somethingThatWorksInMozillaAndWebKit(); }
else
    function foo() { somethingThatWorksInIEAndWebKit(); }

Now if everyone standardizes on the moz behaviour it breaks IE but nothing else, if everyone standardises on the ie/webkit behaviour it breaks moz but nothing else.  Weeeee!!!!
Comment 6 Andy Wingo 2012-02-15 07:22:17 PST
if (somethingThatIsTrueInMozillaAndWebKit())
>     function foo() { somethingThatOnlyWorksInMozilla(); }
> else
>     function foo() { somethingThatWorksInIEAndWebKit(); }

Keeping the existing webkit behavior in the presence of ES6 block-scoped variables is going to be really suprising to developers.

Can we change things?  We can have FunctionDeclaration still hoist the function and body as before, but add FunctionStatement, with semantics equivalent to var foo = function foo() { ... };.

I understand there are tradeoffs, of course, but we are in time to make life better for future JS developers, when let is the new var.
Comment 7 Oliver Hunt 2012-02-15 10:20:21 PST
(In reply to comment #6)
> if (somethingThatIsTrueInMozillaAndWebKit())
> >     function foo() { somethingThatOnlyWorksInMozilla(); }
> > else
> >     function foo() { somethingThatWorksInIEAndWebKit(); }
> 
> Keeping the existing webkit behavior in the presence of ES6 block-scoped variables is going to be really suprising to developers.
> 
> Can we change things?  We can have FunctionDeclaration still hoist the function and body as before, but add FunctionStatement, with semantics equivalent to var foo = function foo() { ... };.
> 
> I understand there are tradeoffs, of course, but we are in time to make life better for future JS developers, when let is the new var.

There is no problem here, if you use let, we enforce the spec: don't allow function statements nested inside of let statements. 

As let is new syntax we won't be breaking pre-existing code.