58479 - functions defined within conditional phrases are always created.

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect, P3)

Description

[David C. Navas]

This causes /mozilla/js/tests/js1_2/Array/tostring_2.js to fail
because the test thinks it's always running a 1.2 version of JS.
To wit:
    var VERSION = "JS_12";

    .
    .
    .
    if ( typeof version == "function" ) {
        writeLineToLog("version 120");
        version(120);
        VERSION = "120";
    } else {
        writeLineToLog(typeof version);
        function version() { return 0; };
    }

    testcases[tc++] = new TestCase ( SECTION,
        "a.toString()",
        ( VERSION == "120" ? "[]" : "" ),
            a.toString() );

The current Interpreter creates a version() function always, and then
runs the test to see if one already exists, which is the wrong order,
sadly.

Comment 1

[Norris Boyd]

Fixed. The name should now be created only when the conditional is executed.

Comment 2

[Phil Schwartau]

Could someone help me out on this one? If I simply go into the JS shell,
(SpiderMonkey or Rhino), I see this:


js> typeof version
function


So I don't follow what's going on here. It looks like "version" is an inbuilt 
function property of the global object. Isn't it supposed to be? How could the 
testcase ever have fallen into the "else" case above? 


David, what was the failure message you were getting from the test? 
Did you have to alter the test in any way to see the failure? 
I just need to know how to test this "before" and "after" the fix.


Thanks - I'm lost on this one

Comment 3

[David C. Navas]

As far as I can tell, version() should only be a function if you're running 
V120.  In other versions, apparently version() is unbound.

To test this, try changing the reference to 'version' to something else -- 
something that isn't defined.  Rhino [15b1, at least -- Norris seemed to 
indicate the problem existed in 15r1 as well] defines the function in
the 'else' clause regardless of program flow.  BTW, when I run SeaMonkey [PR3],
I get "Error: version is not defined" in the javascript console, which is what 
I expect to see, so I'm confused how you're seeing version() defined in 
SeaMonkey....

Comment 4

[Norris Boyd]

Phil, yes, the "version" function is defined in the shell, but not in the 
browser. That's actually what this piece of code is trying to do is detect
whether the code is executing inside the shell or the browser.

David's suggestion about using a variable other than "version" is a good one.

BTW, are you creating new test cases for this and all of [e-mail]'s 
bugs?

Comment 5

[Phil Schwartau]

Thanks for the explanations -

Yes, I will be adding testcases this week. I've been behind in doing this
while I've been doing the triaging, but expect to catch up this week.

Comment 6

[Phil Schwartau]

Following David's suggestion, I'm using an arbitrary variable name in his test.
Using Rhino on WinNT built 2000-10-23, I now see the bug:


THIS TEST FAILED (USING FUNCTION LITERAL): 

var res = (typeof(non_existing_function));

if (res == 'function') 
{
  print('\ntest FAILED - typeof(non_existing_function) = '  +  res +  '\n');
} 
else 
{       
  var f = function non_existing_function() { return 0; };
  print('\ntest PASSED - typeof(non_existing_function) = '  +  res  +  '\n');
}

OUTPUT: 
test FAILED - typeof(non_existing_function) = function




THIS TEST PASSED (USING FUNCTION CONSTRUCTOR):

var res = (typeof(non_existing_function));

if (res == 'function' ) 
{
  print('\ntest FAILED - typeof(non_existing_function) = '  +  res +  '\n');
} 
else 
{       
  non_existing_function =  new Function("return 0;");
  print('\ntest PASSED - typeof(non_existing_function) = '  +  res  +  '\n');
}

OUTPUT: 
test PASSED - typeof(non_existing_function) = undefined
 

Comment 7

[Norris Boyd]

It works for me with both the interpreted and compiled modes. Did you build
with the atest source?

Comment 8

[Phil Schwartau]

No: I was using Rhino on WinNT built 2000-10-23 to benchmark "before/after".
Using Rhino on WinNT and Linux built 2000-10-31, both tests pass.


I tried the tests against both the interpreted and compiled modes of Rhino.

                      On WinNT:  JDK 1.2.2
                      On Linux:  JDK 1.1.8


Marking Verified. The tests will be added to the test suite.

Comment 9

[David C. Navas]

I could not for the life of me get this to work in compiled mode until I
changed the following in Codegen.java (around line 1350):

<             // load boolean indicating whether fn name should be set in scope
<             boolean setFnName = str != null && str.length() > 0 &&
<                                 ((FunctionNode) def).getFunctionType() !=
<                                     FunctionNode.FUNCTION_EXPRESSION;

to:

>             // load boolean indicating whether fn name should be set in scope
>             boolean setFnName = str != null && str.length() > 0 &&
>                                 ((FunctionNode) def).getFunctionType() ==
>                                     FunctionNode.FUNCTION_STATEMENT;

I think this was a typo...?

Comment 10

[Norris Boyd]

What's your test case? I always have to go back and puzzle over ECMA to
figure these out.

Comment 11

[David C. Navas]

I believe if you take Phil's case and remove the 'var f = ' in front of the 
function declaration, you'll see the failure.  Either the compiled version 
shouldn't fail, or the unoptimized version should.  Not sure which is right,
but if it's the latter, then there's a whole slew of testcases under 
mozilla/js/tests that need to be looked at :(

-Dave

Comment 12

[Norris Boyd]

Checked in the suggested change--thanks.