paulbartrum / jurassic

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

Scoping and CompiledScript.Execute vs ScriptEngine.Evaluate #213

Closed lsim closed 2 years ago

lsim commented 2 years ago

Hi Paul

I'm seeing some strange behavior. Just wondering if there is an explanation I'm overlooking.

I have two scripts A and B such as

function main() {
  console.log('Script A');
}
main();

and

function main() {
  console.log('Script B');
}
main();

The key thing is they both rely on the ability to declare the function main in the global scope.

What I am seeing is that running script A via CompiledScript.Execute and then running script B via ScriptEngine.Evaluate results in 'Script A' being logged twice.

Running both via CompiledScript.Execute works as expected.

Running both via ScriptEngine.Evaluate works as expected.

Running script A first via ScriptEngine.Evaluate and then script B via CompiledScript.Execute also works as expected.

It looks as if toplevel scope variables declared via CompiledScript.Execute cannot be overridden via ScriptEngine.Evaluate.

Please note that we still haven't gotten around to updating our engine from v2.2.2, so it is entirely possible that a later version fixes this issue.

paulbartrum commented 2 years ago

Yep, that's definitely wrong, later declarations should override earlier ones.

I tried running this code in the latest version (v3.2.6):

static void Main(string[] args)
{
    var engine = new ScriptEngine();
    engine.SetGlobalValue("console", new Jurassic.Library.FirebugConsole(engine));
    engine.Execute(@"
        function main() {
            console.log('Script A');
        }
        main();");
    engine.Evaluate(@"
        function main() {
            console.log('Script B');
        }
        main();");
}

And it worked as expected. Likely it was fixed when I did a big overhaul of how scoping works a couple years back.

Note that this code prints "Script B" twice, and that's also correct (declarations are processed before code):

static void Main(string[] args)
{
    var engine = new ScriptEngine();
    engine.SetGlobalValue("console", new Jurassic.Library.FirebugConsole(engine));
    engine.Execute(@"
        function main() {
            console.log('Script A');
        }
        main();
        function main() {
            console.log('Script B');
        }
        main();");
}
lsim commented 2 years ago

Thanks for your speedy reply (as always)!

I somehow thought it would be more work testing it out on 3.2.6. I should have given it a go.

I played around with your code and I'm seeing the same thing you're seeing.

Good to know that it's fixed in 3.2.6. We'll have to put some effort into getting the upgrade under way.

paulbartrum commented 2 years ago

If you don't want to update you may be able to clear out the value of "main" in between the two executions, e.g. something like engine.SetGlobalValue("main", Undefined.Value);. I haven't tried it but I guess that would fix it(?)

lsim commented 2 years ago

Our scripts are entirely dynamic, and we do have some cases where we rely on variables declared in one script being accessed from others, so scope wiping between runs will break things.

I have started the upgrade. Looks doable for now.

paulbartrum commented 2 years ago

The code I pasted would only touch the value of "main" and nothing else (so not really "scope wiping"), but if the name of the function is dynamic then yeah that'd be problematic.