Bug 65546

Summary: function statements are evaluated even if enclosed in a not-executed code block (see #27226)
Product: WebKit Reporter: eeelnico <eeelnico>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Major CC: barraclough, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 27226    
Bug Blocks:    
Attachments:
Description Flags
a simple test case to show how this bug can affect a real application. none

Description eeelnico 2011-08-02 09:47:05 PDT
Created attachment 102660 [details]
a simple test case to show how this bug can affect a real application.

this may seem strange, and with not severe consequences, but if you take a look at the supplied test case, you'll see how this can cause runtime errors.
(that are not easy to debug or detect)

Explanation of the attachments:
If you have a js-class, defined by a function statement that uses the "this" keyword in its body, like this:
function A(){ this.name = "A"; }

and you add behaviour to its prototype:
A.prototype.foo = function(){ ... }

and later you add a JS file to your document, with the above code surrounded by an if(condition) with a false-condition,
then the function statement is evaluated AGAIN, and then the A Object is set back to its initial state (without .foo), 
causing a runtime error when you try to:
( new A() ).foo();


Carol Szabo's description in #27226 (from 2009-07-13) is very important, 
and I agree that the desired (and intuitive) behavior is the one in section 13, as Firefox does.


It's also interesting what Daniel Parks says, as this bug does not happen if you code like this:
var A = function(){ this.name = "A"; }
but we know most js developers define js-classes the other way.

(NOTE: in http://www.webkit.org/quality/bugwriting.html it said there would be a "Depends on" field, but I could not find it, sorry).
Comment 1 Oliver Hunt 2011-08-03 13:18:47 PDT
Okay, so the basic problem here is that ES3 states (quite clearly) that a function statement is not allowed anywhere other than the root scope of a function/global code/eval code block, and yet for compatibility reasons we acquired it.  Couple this with the spec defined behavior of hoisting all function statements (rather than just the declarations) and you get our current behavior.  For the most part this matches IE, I believe V8 also takes this approach.

So in the world of ES3 we got into this awkward situation.  We tried to rectify this in ES5, but it turns out that there are sites that depend on our current behavior in some browsers, and depend on firefox's behavior in others.  It turned out to be such a mess that we determined it impossible to spec a standard behavior for this that would ever be adopted.  ES implementations that currently perform function hoisting can't change to not hoist, because we will break content that currently works in them.  Likewise ES implementations that currently don't hoist for exactly the same reason.

The ES5 spec actually goes out of its way to acknowledge this:
"NOTE Several widely used implementations of ECMAScript are known to support the use of FunctionDeclaration as a Statement. However there are significant and irreconcilable variations among the implementations in the semantics applied to such FunctionDeclarations. Because of these irreconcilable differences, the use of a FunctionDeclaration as a Statement results in code that is not reliably portable among implementations. It is recommended that ECMAScript implementations either disallow this usage of FunctionDeclaration or issue a warning when such a usage is encountered. Future editions of ECMAScript may define alternative portable means for declaring functions in a Statement context." (Section 12 of ES5)

JSC explicitly prevents nested function declarations entirely in strict mode.

TLDR; yes this is an annoying incompatibility, but we can change our behavior or we break existing content.  Yes it sucks.  Use strict mode.
Comment 2 Oliver Hunt 2011-08-03 13:25:40 PDT
> TLDR; yes this is an annoying incompatibility, but we can change our behavior or we break existing content.  Yes it sucks.  Use strict mode.

can't *sigh*