rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.73k stars 175 forks source link

Cannot create recursive function in module #241

Closed SuperCuber closed 3 years ago

SuperCuber commented 3 years ago

Seems like I cannot create a recursive function inside a module.

extern crate rhai;

use rhai::{Engine, Module, Scope};

const MODULE_TEXT: &str = r#"
fn factorial(n) {
    if n == 1 {
        return 1;
    } else {
        return factorial(n-1);
    }
}
"#;

const SCRIPT: &str = r#"
    import "test_module" as test;
    print(test::factorial(5));
"#;

fn main() {
    let mut engine = Engine::new();
    let module_ast = engine.compile(MODULE_TEXT).unwrap();
    let module = Module::eval_ast_as_new(Scope::new(), &module_ast, &engine).unwrap();
    let mut static_modules = rhai::module_resolvers::StaticModuleResolver::new();
    static_modules.insert("test_module", module);
    engine.set_module_resolver(Some(static_modules));

    engine.consume(SCRIPT).unwrap();
}

Results in: ErrorInFunctionCall("factorial", ErrorFunctionNotFound("factorial (i64)", 6:16), 3:17)

Upon further testing, it also looks like I cannot access other functions defined in the same module either, even though I expect all of those to be in scope. Am I misunderstanding something, or is this a bug?

schungx commented 3 years ago

Rhai takes an unconventional view towards functions. Functions are compiled separately from the base code. They can be taken apart, regrouped, filtered and merged. In other words, treat each function as an independent entity. This is to make sure that functions remain pure.

Therefore, in a module, functions cannot depend on each other - there is no guarantee that the other function exist when you run your code.

The standard way to call another function from a function, especially within a module, is to pass a function pointer as an argument.

In your case, the recursive call to factorial attempts to look for a function called factorial in the global namespace, which does not exist. What exists at that moment is test::factorial.

So my statement above is not 100% correct - not only can you not depend on other functions in the same module, you also cannot depend on itself.

schungx commented 3 years ago

So you can probably try:

test_module.rhai:

fn factorial(n, func) {
    if n == 1 {
        return 1;
    } else {
        return func.call(n-1, func) * n;    // <- I'm sure you mean to * n
    }
}

main.rhai:

fn factorial(n, func) {
    import "test_module" as test;
    test::factorial(n, Fn("factorial"))
}
SuperCuber commented 3 years ago

Is there a way to have a function be able to access the other items anyways? For example, by manipulating the Scope::new() maybe?

Or maybe is it possible to define the function inside another function and have it be in scope then somehow?

schungx commented 3 years ago

A function inside a function is possible, but that's not the current design. It may be added in a later version.

The current design deliberately decouples functions from each other in order to avoid searching through a scope chain. That's why you also can't access global variables from functions etc.

EDIT: Not as easy as originally thought.

For example, consider:

fn foo() { ... }

fn bar() {
    fn baz() {
        foo();     // <- No problem!
    }
    fn boo() {
        baz();    // <- Is this allowed?  If yes, we got a scope chain.
    }
    fn x1() {
        fn baz() { ... }
        fn x2() {
            fn baz() { ... }
            fn x3() {
                baz();    // <- Now the scope chain is clear
            }
        }
    }

    foo();        // <- No problem!
    baz();        // <- No problem!
}
schungx commented 3 years ago

Is there a way to have a function be able to access the other items anyways? For example, by manipulating the Scope::new() maybe?

If you're willing to do some plumbing yes. For example:

test_module.rhai:

fn factorial(n) {
    if n == 1 {
        return 1;
    } else {
        return factorial(n-1) * n;    // <- I'm sure you mean to * n
    }
}

main.rhai:

fn factorial(n) {    // <- define a top-level function which simply delegates to the module function
    import "test_module" as test;
    test::factorial(n)
}

Notice that the call to factorial in the module matches the function definition of factorial in the global namespace. This essentially lifts test_module::factorial to the top level.

However, in this case the code becomes brittle - if you change the name of factorial it breaks your code.

SuperCuber commented 3 years ago

So what you're saying is that test_module::factorial's call to factorial is actually calling main::factorial in this case? Will that work out-of-the-box or do I need the plumbing you mentioned? In that case, can you elaborate a bit on what needs to be done?

I'll mention my use case just in case that helps, I want to make a utility function that operates on a recursive data structure and I want to put it inside a utility module. So

main.rhai:
fn run_this() {
    import "common" as common;
    let data = /* use a rust-implemented module to create the custom type */;
    let data = common::recursively_filter_stuff(data);
}

This works well when recursively_filter_stuff is in main.rhai as well but as I said I want it in a different file to be re-used across many scripts.

Is there currently no way to do some plumbing on the rust side and have it "just work" on the rhai side? Ideally I want to just be able to access the other functions of the module, so something like this works:

main.rhai:
fn run_this() {
    import "test" as test;
    test::foo();
}

test.rhai:
fn bar() {
    print("works!");
}

fn foo() {
    bar();
}
SuperCuber commented 3 years ago

Also maybe there's a way to get a similar result using closures? I'm not quite sure how they work.

SuperCuber commented 3 years ago

Oh and also, I'm not sure I understand what you mean by "scope chain". I see how you can describe this as a chain:

fn x1() {
    fn x1() {}
    fn x2() {
        x1(); // which x1 do we refer to? It makes sense that the inner x1() is shadowing the outer x1 in this case but this is probably non-trivial to implement
    }
}

But I'm not sure why you're describing this as a chain:

fn foo() { ... }

fn bar() {
    fn baz() {
        foo();     // <- No problem!
    }
    fn boo() {
        baz();    // <- Is this allowed?  If yes, we got a scope chain.
    }

baz inside foo resolves to the only baz in the file, and foo inside baz resolves to the only foo in the file, so I'm not even sure what the implementation problem is.

schungx commented 3 years ago

But I'm not sure why you're describing this as a chain:

Chain = 1) Top-level, 2) bar inner scope, 3) boo inner scope.

The chain extends with each level of internal function nesting. That's why functions cannot be nested in Rhai.

Searching through a scope chain is very slow and expensive; that's why JavaScript engines are so difficult to optimize. In many cases you cannot simply optimize the call info because you never know when something is going to be shadowed in the middle... And also don't forget eval...

schungx commented 3 years ago

baz inside foo resolves to the only baz in the file, and foo inside baz resolves to the only foo in the file, so I'm not even sure what the implementation problem is.

Yes, there is no problem with implementing it. Just need to search through a chain and it'd be slow. Plus the need to constantly push/pop scopes with each level of function calls.

So this is not done due to performance reasons.

SuperCuber commented 3 years ago

I see. What about my "ideal behavior" that I posted a couple messages back? You don't need to implement scope chains for that - functions should only be able to access other top-level functions from the same module as them (unless they're using an import of course, in which case it's not really a scope chain since a module import is basically a local variable as far as I understand)

schungx commented 3 years ago

Is there currently no way to do some plumbing on the rust side and have it "just work" on the rhai side? Ideally I want to just be able to access the other functions of the module, so something like this works:

Not easily. Rhai's modules and functions are very light-weight. Modules are intended for utilities and other stuff.

It is really not very convenient to write large-scale, multi-module programs in Rhai. You should try to move as much functionality into Rust as possible.

Or try other scripting languages that are more intended for large programs, such as Lua.

schungx commented 3 years ago

I see. What about my "ideal behavior" that I posted a couple messages back?

It is possible to develop a custom module resolver that would:

1) Load a module script 2) Parse it into an AST and then store it 3) Register each exported function in that script, delegating to evaluating the function from the AST. 4) Package it up as a new module (containing the AST)

This way, the entire script (together with all functions) will be packaged together with the module.

Such a module resolver is not built-in into Rhai, so somebody must develop it.

SuperCuber commented 3 years ago

Would this be something you consider implementing? I think it's really non-intuitive that a function in the "main module" can access its environment but a function in an imported module cannot... Or at least, that it cannot access its own name.

I'm not even sure why this distinction of main module vs imported module exists, wouldn't it be easier to not have it? Since modules are imported recursively, I think you can just implement the scopes by piggybacking on the call stack that you get naturally from that.

For example, say this is my implementation of a module resolver:

        let ast = engine.compile_file(path.into())?;
        // Can't function calls be resolved at this point?

        Ok(Module::eval_ast_as_new(Scope::new(), &ast, engine)?)
SuperCuber commented 3 years ago

Oh, I just saw your last message. I see we had the same kind of idea. Can you give me some pointers on how I would implement that?

schungx commented 3 years ago

Look into modules.rs.

There is FileModuleResolver which is a start. It is actually embarrassingly simple...

I suggest you extend it via this route;

1) Instead of calling eval_ast_as_new, store the AST first for future uses. 2) Create an empty module. 3) You can call AST::retain_functions to get a list of function names plus the number of arguments. 4) For each function, register a raw function into the module that evaluates the AST function based on name. Notice that registering a raw function is an advanced feature. Check out the chapter on Use the Low-Level API to Register a Rust Function in the Book. 5) In that raw function, use engine.call_fn_dynamic to call the AST function with the provided arguments.

SuperCuber commented 3 years ago

I don't think I understand how those raw functions will connect to the function call that is somewhere deep inside the AST... Let's say I do this

let ast = engine.compile_file(path)?;
let module = /* create a module that re-defines all the functions in the ast using a raw function and call_fn_dynamic */;

// Now what? The function calls inside `ast` are still not going to resolve properly

Also, it doesn't look like I can actually get a list of the names and arguments since retain_functions takes a Fn and not a FnMut which would allow me to append them to some vec as a side effect

SuperCuber commented 3 years ago

Would the AST.lib().iter_script_fn() be better suited here? What's the difference between .lib() and Module::eval_ast_as_new(ast)?

SuperCuber commented 3 years ago

Ahh. I think I see - call_fn_dynamic will use the lib variable to resolve function names while call_fn is probably what is being used by default. Am I getting this right?

So basically I'm getting all the functions out of the ast, patching them so they get called with the module in scope, then packing them up into a new module?

schungx commented 3 years ago

It is a bit more involved, it looks like... It requires more plumbing support that I probably need to add.

So, let me try to implement it and see...

schungx commented 3 years ago

I ended up having to modify some internal Rhai API's to implement this.

It seems to work just as you like, so I've made it the default FileModuleResolver. So in the next version, file-based modules will work as you expect.

schungx commented 3 years ago

@SuperCuber if you'd pull from this repo, modules should now work as you expect. Please test. Thanks.

SuperCuber commented 3 years ago

I will test it tomorrow, thanks for taking the time to make this happen :D

SuperCuber commented 3 years ago

I left some comments on commit c4ec93080e58eb1f48ca4042e35f018ef209e1d3 for now

SuperCuber commented 3 years ago

Just tested it out, looks like it's working exactly like I expect.

I'd add the change to "breaking changes" in RELEASES.md by the way since it's technically one.

One more thing - say I want a custom resolver that has the same functionality, I think this should be made more of a first-class feature by adding a method that makes it "just work" on AST or Module, so that implementing a custom resolver can focus on actually resolving and not just copying the iter_functions loop from FileModuleResolver.

Maybe Module::eval_ast_encapsulated or something?

schungx commented 3 years ago

Maybe Module::eval_ast_encapsulated or something?

Good idea. I'll put a boolean parameter to Module::eval_ast_as_new so it can go both ways.

SuperCuber commented 3 years ago

:+1:

schungx commented 3 years ago

Closing this for now. Thanks for the idea!