Plugins TODO

[schungx]

Tracking Issue

[jhwgh1968]

Your link has an extra underscore at the end, which does not work. If you edit the text and remove it, then it points to the comment.

[schungx]

@jhwgh1968 I suggest we make new conversation here. The original issue is getting way too long and sometimes it takes forever to load...

[jhwgh1968]

Alright. I will post a new To-Do list comment right after this, so you can link to it.

[jhwgh1968]

Former To-Do List (edit: everything is done! 🎉 )

• Improve macro hygiene in case the user renames the rhai crate -- Improve proc macro hygiene #23
• Create attribute parameter to allow renaming the rhai exports -- export_fn: allow duplicate Rust fn names before rename #26
• Create attribute parameter to disable casting with Dynamic::from, for functions that return Result<_, EvalAltResult> -- export_fn: add return_raw attribute #30
• Ensure that functions can be generated in bulk -- Create functional test for generated bulk exports #29
• Allow registration of functions on Engine, in addition to inside a Module -- Split function register macro, add Engine register support #32
• Implement rhai_fn attribute for functions nested within modules (until this, attributes cannot be applied to them) -- Add rhai_fn nested attribute and skip fn parameter #36
• Implement rhai_mod attribute for submodules nested within modules (until this, nested modules do not work) -- codegen: add rhai_mod and submodule support #40
• Allow registration of names that are not valid Rust identifiers, e.g. "+"
• Ensure compile-time error occurs if exported function names collide (due to use of the name attribute)
• Allow #[cfg] attributes on modules and submodules (right now it causes errors) -- Add #[cfg] support in submodules #42
• Add clearer exporting rules based on attributes, rather than pub mod is and mod is not
• Allow get=XXX, set=XXX, index_get, index_set in rhai_fn as alternatives to name -- Reorganize get/set/index code into FnSpecialAccess #55
• Remove Position parameter in call to reduce function call overhead
• Allow rhai_generate to dump into an existing module instead of always creating a new module -- Add combine_with_exported_module and a type cast fix for constants. #56?
• A module_init function to support arbitrary extra behavior in module::generate() -- not in 0.19.0
• Support exportingimpl blocks with self parameters on types -- not in 0.19.0
• Rhai User Documentation Updates for new macros

Code Examples

Here are the current top-level code examples from the crate to give an idea of the syntax:

Exporting a Module to Rhai

use rhai::plugin::*; // <-- required import for macros

#[rhai::export_module(name = "Math::Advanced")]
pub mod advanced_math {
    use rhai::FLOAT;

    // public constants are exported to Rhai by name
    pub const MYSTIC_NUMBER: FLOAT = 42.0 as FLOAT;

    // public functions are exported to Rhai, and can have attributes on them
    #[rhai_fn(name = "euclidean_distance")]
    pub fn distance(x1: FLOAT, y1: FLOAT, x2: FLOAT, y2: FLOAT) -> FLOAT {
        ((y2 - y1).abs().powf(2.0) + (x2 -x1).abs().powf(2.0)).sqrt()
    }

    // public submodules are recursively exported, and can have attributes via
    // the 'rhai_mod' attribute
    pub mod submodule {
        /* inner items here */
    }
}

use rhai::{EvalAltResult, FLOAT};

fn main() -> Result<(), Box<EvalAltResult>> {
    let mut engine = Engine::new();
    let m = rhai::exported_module!(advanced_math);
    engine.load_package(m);

    assert_eq!(engine.eval::<FLOAT>(
        r#"import "Math::Advanced" as math;
           let m = math::MYSTIC_NUMBER;
           let x = math::euclidean_distance(0.0, 1.0, 0.0, m);
           x"#)?, 41.0);
    Ok(())
}

Exporting a Raw Function to Rhai

use rhai::plugin::*; // <-- required import for macros

#[rhai::export_fn]
pub fn distance_function(x1: FLOAT, y1: FLOAT, x2: FLOAT, y2: FLOAT) -> FLOAT {
    ((y2 - y1).abs().powf(2.0) + (x2 -x1).abs().powf(2.0)).sqrt()
}

use rhai::{EvalAltResult, FLOAT, Module, RegisterFn};
use rhai::module_resolvers::*;

fn main() -> Result<(), Box<EvalAltResult>> {

    let mut engine = Engine::new();
    engine.register_fn("get_mystic_number", || { 42 as FLOAT });
    let mut m = Module::new();
    rhai::register_exported_fn!(m, "euclidean_distance", distance_function);
    let mut r = StaticModuleResolver::new();
    r.insert("Math::Advanced".to_string(), m);
    engine.set_module_resolver(Some(r));

    assert_eq!(engine.eval::<FLOAT>(
        r#"import "Math::Advanced" as math;
           let m = get_mystic_number();
           let x = math::euclidean_distance(0.0, 1.0, 0.0, m);
           x"#)?, 41.0);
    Ok(())
}

[jhwgh1968]

I also updated the code to use load_package instead of a static module resolver -- I hope that is correct @schungx?

[schungx]

Yes, load_package is the simplest for testing, because it simply converts a Module to a PackageLibrary.

[schungx]

BTW, I find that errors, especially return type conflicts when use return_raw, are very difficult to diagnose. They do not point to any location, and the error messages are quite cryptic. I just spent hours finding out a type bug in one of the module functions.

The best way to clean these errors seem to be to copy the module out, remove #[export_module] and all the #[rhai_fn], then compile it as a separate module. That'll pick out all the errors.

It would be great if #[rhai_fn] can be put on a normal module that does not have #[export_module] and does nothing. RIght now it is a syntax error. If I can just comment out the #[export_module] line and get lints, then it would help coding a great deal.

[jhwgh1968]

BTW, I find that errors, especially return type conflicts when use return_raw, are very difficult to diagnose. They do not point to any location, and the error messages are quite cryptic.

This is why I have tried to focus on error handling and diagnostic tests every time I add a new error. You are probably finding the issues I was planning to find with negative testing later.

Post examples of all the ones you find, and I will try to fix them.

[schungx]

error[E0277]: the trait bound `utils::ImmutableString: std::convert::From<u128>` is not satisfied
  |
  = help: the following implementations were found:
            <utils::ImmutableString as std::convert::From<&str>>
            <utils::ImmutableString as std::convert::From<std::boxed::Box<std::string::String>>>
            <utils::ImmutableString as std::convert::From<std::string::String>>
  = note: required because of the requirements on the impl of `std::convert::Into<utils::ImmutableString>` for `u128`
  = note: required because of the requirements on the impl of `std::convert::From<u128>` for `any::Dynamic`
  = note: required because of the requirements on the impl of `std::convert::Into<any::Dynamic>` for `u128`

The location of the error is pointing to the first character of lib.rs, which is not useful.

This error, or something similar, is generated whenever I made a mistake by using .into() to convert a type into Dynamic that doesn't support it.

For example:

Ok(x.into())

will generate this error if the type of x is not Into<Dynamic>.

Ok(Dynamic::from(x))

solves this problem.

[jhwgh1968]

Ah, I see.

I am currently working on a large refactoring (so no plugin changes please!), but I will have a commit in there that makes the error message clearer.

[jhwgh1968]

As @schungx noted:

OK, I tested the error diagnostics.

#[export_fn] successfully puts out an error for return_raw which does not return Result.
#[export_fn] functions have good error diagnostics.

#[export_module] functions do not highlight return_raw errors, not do they highlight interior errors.

Therefore, it seems like we're still missing error position info for modules.

I think this means I should probably start shaking the trees harder to see what fruit falls. I will start with the return_raw reporting, and try some things.

[schungx]

@jhwgh1968 BTW... is it possible for #[set_exported_fn] to take a closure instead of a #[export_fn] fn?

The code base right now is a bit littered with a number of #[export_fn] simple functions just because a function is needed.

Since you're doing generating a module, I'm not sure if you can splice it in at the registration call site...

[schungx]

BTW, just fyi... I rerun the benchmarks and it seems plugins do not have any material impacts. Good news!

Still, I would suggest removing the Position parameter in call. Even though it is only 32 bits, still one fewer argument is one fewer argument...

I'll put an item in the TODO...

[schungx]

Also, I'll start on the plugins documentation.

[jhwgh1968]

BTW, just fyi... I rerun the benchmarks and it seems plugins do not have any material impacts. Good news!

Indeed it is! 🎉

Still, I would suggest removing the Position parameter in call. Even though it is only 32 bits, still one fewer argument is one fewer argument...

The Position parameter was something I missed when I turned parameter checking from a runtime error (which requires it) into a debug_assert!. That should be an easy fix.

BTW... is it possible for #[set_exported_fn] to take a closure instead of a #[export_fn] fn?

Unfortunately. no. You should think of set_exported_fn! as the equivalent of this code:

macro_rules set_exported_fn! {
    (module:$ident, export_name:$literal, fn_path:$path) => {{
        macro_let!($generated_module_path = {
              let fn_name = module_path.segments.pop();
              let rhai_fn_module = format!("rhai_fn_{}", fn_name);
              module_path_segments.join(rhai_fn_module);
        });
        macro_let!($input_types_fn = concat!($generated_module_path, "input_token_types"));
        macro_let!($callable_token_fn = concat!($generated_module_path, "token_callable"));
        $module.set_fn($export_name, FnAccess::Public,
                       $input_types_fn().as_ref(), // array of argument types
                       $callable_token_fn()); // the token implementing PluginFunction
    }}
}

In other words, it simply navigates to the generated module, and calls the functions defined therein to return the list of arguments and the PluginFunction to add.

The only way to get that module generated in its current form requires an fn.

The code base right now is a bit littered with a number of #[export_fn] simple functions just because a function is needed.

I am hoping that can be cleaned up once I allow including a function in a module "remotely". Then, it should be possible to generate those functions with macros, not tag them as #[export_fn], and then pull them into the module tagged with #[export_module].

[schungx]

BTW, I just merged in the first draft of the Plugins section in the documentation.

[schungx]

Example of error:

macro_rules! gen_cmp_functions {
    ($($arg_type:ident),+) => {
        mod test { $(pub mod $arg_type {
            use super::super::*;

            #[export_module]
            pub mod functions {
                #[rhai_fn(name = "<")]
                pub fn lt(x: $arg_type, y: $arg_type) -> bool {
                    x
                }
            }
        })* }
    };
}

gen_cmp_functions!(i8);

mismatched types
expected `bool` because of return type rustc(E0308) [1, 1]

The same code without macro_rules! will highlight the error just fine.

[schungx]

@jhwgh1968 Actually, I feel that plugins is ready to merge into master. What do you think?

Or leave it on a branch just in case we need to do bug fixes in master. Merge when we're ready to release 0.19.0.

[jhwgh1968]

The same code without macro_rules! will highlight the error just fine.

Hmm... I think that might be the order of how macros are invoked vs procedural macros. I will look into it.

Actually, I feel that plugins is ready to merge into master. What do you think?

I'd say merge the current branch to master, but keep the branch going a little longer.

The merge to master (and then to upstream master) allows us to discover integration issues, and lets other people give feedback on what we have so far. I think it is definitely a "beta version" now, so we should do that.

However, I'd still like the plugins branch until I have all the features done. The only ones left:

1. Add optional export control attributes
2. Add a mod_init function, i.e. a user function which can change the generated module in an #[export_fn]
3. Add a new #[export_type] macro for impl blocks

(I'm considering leaving the last one out of the 0.19 release, because it's a bit of a mess right now.)

Everything else on the to-do list is fixes or usability improvements (such as the items for extern imports or generate flexibility). Those can be individual PRs, so it doesn't matter as much.

[jhwgh1968]

Okay, I am very confused. Checking out the current plugins branch, I get 32 failed doc tests for Rhai if I use the only_i32 feature. This is both with the nightly and stable compilers, even though the CI for commit 91b4f8a6 is all green!

I'll try opening my PR as-is, and see if CI passes anyway...

[schungx]

I get 32 failed doc tests for Rhai if I use the only_i32 feature.

Which tests failed? Can you send me a log?

[schungx]

It seems that export_prefix = "..." checks the registered name of the function, instead of the actual name of the function. For example, #[rhai_fn(name = "foo")] fn bar()... will match foo instead of bar.

You should match the actual function name instead because, if I put a rhai_fn(name = "...") on a function, most likely I would like to export it! export_prefix is only useful when I don't need to rename functions and I want to only include some of them.

[jhwgh1968]

You should match the actual function name instead because, if I put a rhai_fn(name = "...") on a function, most likely I would like to export it! export_prefix is only useful when I don't need to rename functions and I want to only include some of them.

You are absolutely right. I went back and forth on this for a while, and without thinking, implemented the wrong way! I will fix it.

---

Which tests failed? Can you send me a log?

Attached is the log of failures on 1.46.0 stable (nightly is very similar if not identical): cargo-test.txt

It seems that changing INT to i32 instead of i64 is causing a lot issues with casting and method matching.

[schungx]

Actually, I find that it probably would be more useful to have a skip_prefix and skip_postfix versions. I can then put functions like pub get_XXX_internal and have sip_postfix = "_internal" if I need that function elsewhere. Or skip_prefix = "_" to skip all functions beginning with underscores...

export_prefix is less useful because it is unlikely that an entire API will begin with the same text prefix...

[schungx]

While I think it might be a small speed win (in theory), I was actually hoping that it would allow some simplification of the Rhai stdlib. I saw several cases where libraries are built, piece by piece, and then added to the actual package one at a time. Perhaps I could do better by flatten-ing them down into a single structure?

Well, module construction should not happen frequently, so it is usually not a bottleneck. Auto-flattening is nice, but not critical. That's because the user can always call combine_flatten to recursively add the functions. All you'll be saving is the construction of a few sub-module types, and it is not a problem for something not on the hot path.

I recommend skipping it for now.

Instead, auto getters/setters/indexers will be a huge feature.

One thing you may consider is that sometimes the same function is used to register multiple Rhai functions. For example, the user may have an + operator, and an add function, with + delegating to add. Also, it is always useful to have functions plus properties on the same field. For example: foo.bar() and foo.bar both return the same thing.

Right now, I must define two functions, with one call the other. Some way to automate this? Maybe allowing rhai_fn(name = "...","...","...") with multiple names? Or, #[rhai_fn(name = "prop", name = "->", get = "prop")] fn get_prop()... will put prop, -> and get$prop into name which is turned into a Vec.

[schungx]

I've put in a PR that supports multiple name parameters.

[schungx]

I've added your ID to the authors of Rhai since it is a pretty major addition you have made on the project.

Would you like jhwgh1968 or your full name?

PS I've also removed my name from codegen since I really did not work on it other than using it.

[jhwgh1968]

Since my committer full name is a "Nom De Plume" rather than a real name (it's a long story), I think jhwgh1968 would be better.

As to the authorship of the codegen crate, I will send you an e-mail about that.

[schungx]

Do you think 0.19.0 is ready to launch?

[jhwgh1968]

Do you think 0.19.0 is ready to launch?

I would like to go down the checklist one more time, and perhaps discuss one or two of the items. But unless one of those turns into a blocker in your mind, I think it's ready!

As to the authorship of the codegen crate, I will send you an e-mail about that.

Just so it's on the record, our discussion via e-mail ended up here: I'll stay primary author of the codegen crate, and the pointer to this repo will still point people where to file issues.

[schungx]

Just so it's on the record, our discussion via e-mail ended up here: I'll stay primary author of the codegen crate, and the pointer to this repo will still point people where to file issues.

OK. I take that you mean the repository field in codegen/Cargo.toml should point to the parent repo (as it is right now), correct?

[jhwgh1968]

OK. I take that you mean the repository field in codegen/Cargo.toml should point to the parent repo (as it is right now), correct?

Correct.

[schungx]

Sorry, accidentally merged the latest PR. But it looks OK, except for a feature test that I'll fix.

[schungx]

I think I might have found what went wrong. I did merges on my plugins_dev. I guess it is probably a bad idea to do a rebase after merging... It probably replays everything on those merges again...

So if I just delete the branch after merge, and then reopen a new one... as long as I haven't merged that one, I can freely rebase, I suppose.

[jhwgh1968]

Your statement is correct. The workflow is:

1. Set plugins_dev to plugins tip.
2. Put commits on the dev branch.
3. When ready for review, open a PR on the dev branch.
4. Add new commits as needed.
5. Once approved, rebase it onto plugins.
6. Merge once CI passes. This will result in a "fast forward" merge (i.e. no conflicts).
7. Pull plugins_dev to plugins, because they should now be equal except for the last merge commit you did.

That is the workflow I use. The only difference is, I create "feature branches" for one thing at a time, and delete them once they merge.

As noted in #58, some stuff did get reverted. I will fix it.

[jhwgh1968]

On second thought, I think you're right. It ended up merged okay.

I'll add a second PR for a rustfmt CI job, though.

[schungx]

Pull plugins_dev to plugins, because they should now be equal except for the last merge commit you did.

If I do a rebase afterwards, will it screw up the root of the branch? Since it'll be moving the branch to the current head, which is after the previous merge...

[jhwgh1968]

It shouldn't.

The first step of the rebase command is to figure out where to put it. To see the commit it will decide on, you can run this yourself: git merge-base plugins plugins_dev.

If the commit it shows is the tip of the plugins branch, then rebase will do the right thing. If the commit is below the tip somewhere in the middle, then you will get into trouble.

If you do a rebase onto a branch which is ahead, it will point to the head of your current branch. That means there is nothing above it to rebase, and the branch will just fast forward.

See if this longer explanation helps.

[schungx]

If the commit it shows is the tip of the plugins branch, then rebase will do the right thing. If the commit is below the tip somewhere in the middle, then you will get into trouble.

Ah! Gotcha!

[schungx]

BTW there is a diag_test directory. Is it used?

I suppose we are just about ready to put out 0.19.0...

[jhwgh1968]

BTW there is a diag_test directory. Is it used?

That was an experiment at a previous point in development, and was committed by mistake. Please do remove it.

I suppose we are just about ready to put out 0.19.0...

Very close. I will double check my list when I get time this week.

[jhwgh1968]

On the current list, I checked off a couple more things to bring it up to date. At the very least, I think we can merge the plugins branch to master now.

Could you please double check, and make sure you agree all the items are done? In particular, that your PR handled the module_generate question, and the current state of get/set/index is good enough to ship.

The two items left are:

1. A module_init function. I put this in because I suspected this could solve some of your special cases. However, it seems the Rhai standard library is pretty good without it, so I would drop this unless you see value.

2. Rhai documentation. I have seen a lot of changes from you go by, and I would like to do one quick review of them before I check that box. Is the current "nightly" book updated?

I will double check to make sure I haven't missed anything later this week.

[schungx]

Is the current "nightly" book updated?

Not really... I manually update the vnext book once in a while. It might be easier to run mdbook serve on the latest sources to check the current version...

[schungx]

@jhwgh1968 I think the nightly compiler has some new changes. Types wrapped up in repeat blocks in macros now reside inside Group(TypeGroup { ... }) instead of being a simple Path. This breaks detection of &mut parameter types.

I've put in a simple flatten_groups function to by-pass all these grouping in rhai_module.rs.

[jhwgh1968]

I'm glad you've got a fix for it.

Meanwhile, after a long thread on the Rust Forums, apparently the lint I suppressed was not saying what I thought it was. I have removed an extra clone() from the generated code, and will have a PR in a moment.

[schungx]

@jhwgh1968 I fixed a problem with rename checking. FnSpecialAccess was not checked, so a function with only a getter etc. was treated as a function with no renames. I included special in rename checks.

[jhwgh1968]

@schungx, I have double-checked the to do list, and checked everything off! 🎉

I think both this issue and #3 can be closed. Any enhancements in the future can be separate issues.

[schungx]

I'll leave the current version on the official repo for a few more days, then we can probably push out 0.19.0!

[schungx]

I'll delete the plugins branch. For new work, let's open up a new branch in the future.

Thanks for all this work!

[schungx]

@jhwgh1968 Just to let you know that I've added a new feature to plugins so that functions take a virtual NativeCallContext parameter, if it is the first parameter. The type encapsulates the current executing engine and namespace.

It makes it completely unnecessary to ever call Module::set_fn_raw again if I need to access Engine for some reason...

I must admit the plugins system is making it almost embarrassingly easy to define functions for Rhai!