395907 - eval of function declaration does not change existing variable

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Martin Honnen]

Test code looks like this:

var x = "string";
function f () {
    var x = 0;
    print('typeof x: ' + (typeof x));
    eval('function x() { return true; }');
    print('typeof x: ' + (typeof x));
    print('x(): ' + x());
}
f();

When run with Rhino the output is as follows:

typeof x: number
typeof x: function
x(): true

so the eval on the function declaration assigns the created function as the valu of the local variable x.

With Spidermonkey however the result is as follows:

typeof x: number

typeof x: number

and then a script error trying to call x with the message "x is not a function".

Tested with Spidermonkey in the latest Firefox nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre) but this seems to be a long standing Spidermonkey bug, not a regression.

Spidermonkey's behaviour is a bug in my view as the ECMAScript edition 3 in section 10.1.3 says:
  "For each FunctionDeclaration in the code, in source text order, create a property of the variable object whose
name is the Identifier in the FunctionDeclaration, whose value is the result returned by creating a Function object
as described in section 13, and whose attributes are determined by the type of code. If the variable object
already has a property with this name, replace its value and attributes."

So the value of the already existing variable x should be replaced with the function object resulting from the evaluation of the function declaration.

Comment 1

[Martin Honnen]

Attachment: test case to be run in script engine shell

Comment 2

[Martin Honnen]

Attachment: test case to be run in the browser

Comment 3

[Martin Honnen]

I have tried https://bugzilla.mozilla.org/attachment.cgi?id=280605 with Opera and with IE and the result is the same as with Rhino.

Comment 5

[Brendan Eich [:brendan]]

Old bug.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix

I considered trying to set TCF_IN_FUNCTION for a cg created indirectly by eval, but the object in which to search for a hidden property varies too. So brute force testing in the TOK_FUNCTION case of js_EmitTree seems best, and fixes the bug.

/be

Comment 7

[mozilla]

Should have turned off my precognative abilities...
The attached fix now triggers Bug 395868 (which wasn't there before).

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Should have turned off my precognative abilities...
> The attached fix now triggers Bug 395868 (which wasn't there before).

Triggers or fixes? The attached fix here has not been checked in, so it can't be the cause of another bug report.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 280693 [details] [diff] [review]
fix

>-            JS_ASSERT(prop && pobj == obj);
>-            sprop = (JSScopeProperty *) prop;
>-            JS_ASSERT(sprop->getter == js_GetLocalVariable);
>-            slot = sprop->shortid;
>-            OBJ_DROP_PROPERTY(cx, pobj, prop);
>+            if (prop) {

Worth asserting that !(cg->treeContext.flags & TCF_IN_FUNCTION) if prop is null?

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, with assertion

Sure.

/be

Comment 11

[Brendan Eich [:brendan]]

Fixed:

js/src/jsemit.c 3.273

/be

Comment 12

[mozilla]

I applied the patch by hand, and now Bug 395868 is relevant. As I said, I submitted the bug-report about 24h too early. Only with this fix, the bug-report actually makes sense. I have hence reopened Bug 395868.

Comment 13

[Martin Honnen]

Verified as fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091504 Minefield/3.0a8pre

Comment 14

[Bob Clary [:bc] (inactive)]

resurrected old test id with new test...

Checking in 10.1.3-2.js;
/cvsroot/mozilla/js/tests/ecma_3/ExecutionContexts/10.1.3-2.js,v  <--  10.1.3-2.js
new revision: 1.5; previous revision: 1.4