Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bound function should not inherit from FunctionInstance #1044

Merged

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Jan 9, 2022

From spec:

Bound function exotic objects do not have the internal slots of ECMAScript function objects listed in Table 34. Instead they have the internal slots listed in Table 35, in addition to [[Prototype]] and [[Extensible]].

@lahma lahma changed the title Bound function is not a FunctionInstance Bound function should not inherit from FunctionInstance Jan 9, 2022
@lahma lahma force-pushed the bound-function-not-function-instance branch from aa130b2 to 21e3bee Compare January 9, 2022 18:25
@lahma lahma merged commit f2e4404 into sebastienros:main Jan 9, 2022
@lahma lahma deleted the bound-function-not-function-instance branch January 9, 2022 18:36
@paynesworld
Copy link
Contributor

@lahma BindFunctionInstance was moved from public to internal as part of this change, which prevents casting of JsValues to BindFunctionInstance. Can this be moved back to public?

@lahma
Copy link
Collaborator Author

lahma commented Jun 13, 2022

Why do you need to cast it?

@paynesworld
Copy link
Contributor

paynesworld commented Jun 13, 2022

I'm the first to admit I'm weak on JavaScript -- but below is a quick sample that runs in Jint 3.0.0-beta-2033 but fails in 3.0.0-beta-2037:

using Jint;
using Jint.Native;
using Jint.Native.Function;
using System;

namespace JintTest
{
    class Program
    {
        private Engine _jintEngine;

        static void Main(string[] args)
        {
            new Program().Run();

            Console.ReadLine();
        }

        public Program()
        {
            var opts = new Options();
            opts.AllowClr(typeof(Program).Assembly);

            _jintEngine = new Engine(opts);
            _jintEngine.SetValue("MyCSharpClass", this);
        }

        public void log(string logStr)
        {
            Console.WriteLine($"From JS: '{logStr}'");
        }

        public void executeScript(JsValue script)
        {
            if (script is FunctionInstance func)
            {
                _jintEngine.Invoke(func);
            }
            else
            {
                var parser = new Esprima.JavaScriptParser(script.ToString());
                var compiledScript = parser.ParseScript();
                _jintEngine.Execute(compiledScript);
            }
        }

        public void Run()
        { 
            var js = @"

var outer = {

    innerFunc() {
        MyCSharpClass.log('test');
    },

    executeThis(script) {
        MyCSharpClass.executeScript(script);
    }
}

outer.executeThis(outer.innerFunc.bind(this));

";
            _jintEngine.Execute(js);
        }
    }
}

The value passed to the CS executeScript() method is a BindFunctionInstance but I can no longer cast it to that type and invoke the function it represents like I could when it derived from FunctionInstance:

image

In this simple example the .bind() operation isn't necessary other than to demonstrate the issue, but in our production code we use it regularly.

Appreciate your input and any guidance you might be able to provide, thanks!

@lahma
Copy link
Collaborator Author

lahma commented Jun 14, 2022

I didn't quite catch the idea behind using the bind function with interop. Why do you need different this inside interop?

@paynesworld
Copy link
Contributor

Well this is just a contrived example I built from the much-more-complex javascript code we're using. We have lots of valid use cases for the .bind() function with the same basic calling convention I'm demonstrating. Again I'm not the JS expert on our team but I can get our expert to help me create a better example if it will make the issue more clear? Seems like I should be able to use the bind() function since it's valid script, but maybe there's something unexpected we're doing.

Again thanks for your help.

@lahma
Copy link
Collaborator Author

lahma commented Jun 14, 2022

Yes it would be great if you could elaborate the valid use case you are referring, it would make the ask clearer and we could possibly then make samples utilizing the paradigm and ensure it can be used.

@paynesworld
Copy link
Contributor

I spoke with my JS expert this morning and it turns out there are several cases of our use of .bind() that in hindsight are not necessary -- so perhaps this change actually highlights a need for refactoring we should be doing anyway. We're working through the code to see if we can find a valid use case -- I'll update my example code if we find one. Really appreciate the responses, thanks.

@paynesworld
Copy link
Contributor

We've come across an example usage of .bind() that I think justifies making BindFunctionInstance public.

In this example, we've implemented our own version of the JS setTimeout() method in C#, in order to support deferred execution of JS functions. We pass an object to be used in the context of the deferred execution by using the .bind() statement.

This currently results in the same issue, where the JsValue we receive in the C# executeScript() method is an instance of BindFunctionInstance, but we cannot cast to that class because the class is now marked as internal.

This is another watered-down example of much-more-complex code in our solution, but again this is a valid usage of JavaScript that I'm hoping can be supported in Jint. Appreciate your attention, thanks!

using Jint;
using Jint.Native;
using Jint.Native.Function;
using System;

namespace JintTest
{
    class Program
    {
        private Engine _jintEngine;

        static void Main(string[] args)
        {
            new Program().Run();

            Console.ReadLine();
        }

        public Program()
        {
            var opts = new Options();
            opts.AllowClr(typeof(Program).Assembly);

            _jintEngine = new Engine(opts);
            _jintEngine.SetValue("MyCSharpClass", this);
        }

        public void log(string logStr)
        {
            Console.WriteLine($"From JS: '{logStr}'");
        }

        public void executeScript(JsValue script)
        {
            if (script is FunctionInstance func)
            {
                _jintEngine.Invoke(func);
            }
            else
            {
                var parser = new Esprima.JavaScriptParser(script.ToString());
                var compiledScript = parser.ParseScript();
                _jintEngine.Execute(compiledScript);
            }
        }

        public void setTimeout(JsValue script, object timeout)
        {
            // Not shown: queue the script object for execution on a background thread.
            // Not shown: the background thread executes the script in the JintEngine after the timeout expires.
        }

        public void Run()
        { 
            var js = @"

var coolingObject = {
    coolDownTime: 1000,
    cooledDown: false
}

MyCSharpClass.setTimeout(function() {
    coolingObject.cooledDown = true;
    }.bind(coolingObject), coolingObject.coolDownTime);

";
            _jintEngine.Execute(js);
        }
    }
}

@lahma
Copy link
Collaborator Author

lahma commented Sep 4, 2022

@paynesworld would you like to create a PR for a new project in solution Jint.Samples.Interop and a class showing the usage and missing public visibility (and changing visibility to public)?. We just need these sample projects with usage to maintain API for anyone needing them to be public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants