paulbartrum / jurassic

A .NET library to parse and execute JavaScript code.
MIT License
873 stars 122 forks source link

Function declaration in inner function becomes undefined within a loop #140

Closed kpreisser closed 5 years ago

kpreisser commented 5 years ago

Hi, I encountered a strange bug today:

        var engine = new ScriptEngine();
        engine.SetGlobalValue("console", new FirebugConsole(engine));

        engine.Execute(@"

'use strict';
for (var i = 0; i < 3; i++) {
    (function () {
        function test() {
        }
        console.log(typeof test);
    }());
}

");

Expected output:

function
function
function

Actual output:

function
undefined
undefined

I was surprised that this was not caught/tested by the unit tests. I have not yet looked what could be the cause that the test function is undefined after the first iteration.

If you change the decaration of test to be a function expression that is assigned to a variable, it works:

for (var i = 0; i < 3; i++) {
    (function () {
        var test = function test() {
        };
        console.log(typeof test);
    }());
}

Also, if you extract the first inner function outside of the loop, the output is correct:

function loopTest() {
    var test = function test() {
    };
    console.log(typeof test);
}

for (var i = 0; i < 3; i++)
    loopTest();

Thanks!

kpreisser commented 5 years ago

It seems the problem arises due to a combination of two factors:

  1. The LoopStatement generates the code for its body (as well as the condition and increment) twice, as described in the GenerateCode() method: https://github.com/paulbartrum/jurassic/blob/e8ade10c448ccd99ca40217bf6221c89259f1d9f/Jurassic/Compiler/Statements/LoopStatement.cs#L115-L130

  2. Scope.GenerateDeclarations() checks if a variable has already been initialized, and in that case skips it: https://github.com/paulbartrum/jurassic/blob/e8ade10c448ccd99ca40217bf6221c89259f1d9f/Jurassic/Compiler/Scope/Scope.cs#L263-L265

However, when the LoopStatement generates the code for the body the second time, Scope.GenerateDeclarations() determines that the variable (test) has already been initialized, and therefore skips code generation to create a new FunctionInstance and store it into the test variable. This results in test being undefined except for the first loop iteration.

What would be the right fix for this issue? Change the LoopStatement so that it generates the code for the <initializer>, <condition> and <loop body> only once? (Is there a reason why it generates the code two times?) OK, I think I understood that generating the code two times is for the optimization of the loop variables. Then it seems the problem is in the condition in Scope.GenerateDeclarations() that skips generating the initialization code if the variable already been initialized, or maybe that the same Scope is used twice.

Thanks!

paulbartrum commented 5 years ago

Thanks for this. I ended up modifying your fix quite a bit, check out commit 875a93f for details.

kpreisser commented 5 years ago

Thank you!