New Plugins API

[schungx]

@jhwgh1968 there has been an open issue regarding the fact that creating an Engine is expensive: rhaiscript#142

It looks like registering all those core functions (such as basic arithmetic) each time during an Engine creation is causing it to be really slow.

The solution, obviously, is to run it only once (since they never change - all scripts will need them). One solution is to use lazy_static to make this package of functions a global constant, but the downside is that an additional crate must be pulled in: lazy_static.

An alternate solution is to really start breaking down the built-in's into separate packages that are immutable, so the packages themselves can be created only once, stored around, and then passed to all new Engine created.

Something of that sort:

let core_pkg = Library::CoreLib::create();

let mut engine = Engine::new_raw(&[ core_pkg ]);    // Pass in a list of packages to register
       :
let mut engine = Engine::new_raw(&[ core_pkg ]);
let mut engine = Engine::new_raw(&[ core_pkg ]);
       :
   create many engines

[jhwgh1968]

The version of plugins I am currently working on would solve this problem. It will allow an engine to delegate the lookups until the function is called.

It also will give more convenient Rust syntax for writing a package:

#[rhai::plugin]
mod MyPlugin {
    pub fn export_to_rhai(...) { /* do stuff* / }
    // more pub fns
}

Once defined, all that is needed will be something like

let mut engine = Engine::new();
rhai::register_plugin!(engine, MyModule);

If you are wondering how it works, it uses procedural macros to generate a calling function with a uniform interface.

After the plugin macro has done its work, the module will look like this:

mod MyModule {
    pub fn export_to_rhai(i: INT, f: FLOAT, t: MyStruct) { /* ... * }
    pub fn call<'e>(
        engine: &'e Engine,
        fn_name: &str,
        args: Iterator<Item=Dynamic + 'e>
) -> Result<(), rhai::EvalAltResult> {
        switch fn_name {
            "export_to_rhai" => Ok(export_to_rhai(args.next()?.as_ref().cast::<INT>()?,
                  args.next()?.as_ref().cast::<Float>()?,
                  args.next()?.as_ref().cast::<MyStruct>()?))
            },
            _ => Err(rhai::EvalAltResult::RuntimeError("cannot find function '{}' in 'MyModule'", fn_name),
        }
    }
}

This means that registering a module results in registering one "lookup function", and that is all. Otherwise, it is static code, generated at compile time.

This should be much more efficient, and more convenient for users.

[schungx]

Wow, this is something beyond my expectations. I think we can replace the entire packages implementation with this.

One question: when you register a plugin module, I suppose you register all the functions declared within the same module, right? I don't suppose you can "pick-n-choose"?

You'll also need a way to handle functions that have a first argument that is &mut for update functions. All other arguments must be passed by value.

Also, I'm thinking how this can handle generic functions with multiple data types. I suppose you must have some form of name mangling then... For example, a simple add function can be registered for the whole range of integer data types. If you're simply reflecting the Rust std API, then this is a non-issue because they have different names anyway... However, Rhai supports function overloading.

[jhwgh1968]

Wow, this is something beyond my expectations. I think we can replace the entire packages implementation with this.

I think we can replace most of it, but there might be some edge cases. See below.

One question: when you register a plugin module, I suppose you register all the functions declared within the same module, right? I don't suppose you can "pick-n-choose"?

In the current prototype, all pub items are exported to Rhai.

It would certainly be possible to add additional attributes to the functions themselves, like #[rhai::export_fn(name = "name_in_rhai")] or #[rhai::export_fn(ignore)]. But I'm focusing on the main cases for now.

You'll also need a way to handle functions that have a first argument that is &mut for update functions. All other arguments must be passed by value.

I would probably have a call_mut_reciever as a separately generated table. After all, what's a little awkwardness if the compiler is doing all the work, right?

Also, I'm thinking how this can handle generic functions with multiple data types. I suppose you must have some form of name mangling then...

Generics are something I am not sure if I can support or not.

The syntax wouldn't be difficult to detect and add, but Rust's type inference may be the limitation due to my reliance on the cast operation for Dynamic.

Let's take your example with plus on numeric types. The broadest way to implement that would be to implement a trait RhaiAddable for all ints, floats, etc, and then:

#[rhai::plugin]
mod numeric_operations {
    #[rhai::reg_op("+")]
    pub fn plus<T: RhaiAddable>(a: T, b: T) -> Result<T, rhai::EvalAltResult> {
        /* achieve this with a trait call or special casing... */
    }
}

As currently written, the auto-generated code would create a call to cast::<T> -- that is, the generic type passed in, rather than any specific type.

Even if T is bounded eventually -- i.e. plus allows clear type inference -- I suspect Rust's type inference would sometimes get confused. There might still have to be some alternative mechanism for generic functions in this case.

Also, please note that my use of reg_op in that example was just to explain it. I have no idea how to approach that, so it might be another special case. 😄

[jhwgh1968]

I have opened a PR for the foundational API I need for this work. Hopefully it will be quick to review and merge.

[schungx]

If you can modify your PR to merge into the plugins branch I'll do it right away.

As to generic functions, I think that is the main difference between Rhai and Rust... in this case, Rhai is more like JS. Not only can arguments be of different types, there can also be different number of arguments.

In many Rust std lib cases, multiple functions with different names can be mapped into the same function name in Rhai. I think we need to make the Rust API more "Rhai-centric" by leveraging overloading instead of a simple one-to-one mapping.

As for binding multiple generic versions, maybe the plugin can provide a generic version that an outside macro can simply loop over. For example:

mod MyModule {
    #[rhai(name = "export_to_rhai")]
    #[rhai::reg(i8, f32)]
    #[rhai::reg(i32, f64)]
    pub fn export_to_rhai_3<T: Add, U, V>(i: T, f: U, t: MyStruct) -> String { /* ... * }

    #[rhai(name = "export_to_rhai")]
    #[rhai::reg(char, INT)]
    pub fn export_to_rhai_2<T, U, V>(i: T, f: U) -> bool { /* ... * }

    pub fn call<'e>(
        engine: &'e Engine,
        fn_name: &str,
        args_hash: u64,
        args: Iterator<Item=Dynamic + 'e>
) -> Result<(), rhai::EvalAltResult> {
        switch (fn_name, args.len()) {
            ("export_to_rhai", 3) => {
                  /* Somehow use a hash to map argument types */
                  match args_hash {
                      1234567 => Ok(export_to_rhai_3(args.next()?.as_ref().cast::<i8>()?,
                          args.next()?.as_ref().cast::<f32>()?,
                          args.next()?.as_ref().cast::<MyStruct>()?)),
                      98765 => Ok(export_to_rhai_3(args.next()?.as_ref().cast::<i32>()?,
                          args.next()?.as_ref().cast::<f64>()?,
                          args.next()?.as_ref().cast::<MyStruct>()?)),
                       _ => Err(......   argument types not matched .......),
                  }
           }

           ("export_to_rhai", 2) => ..... ,

            _ => Err(rhai::EvalAltResult::RuntimeError("cannot find function '{}' in 'MyModule'", fn_name),
        }
    }
}

[jhwgh1968]

If you can modify your PR to merge into the plugins branch I'll do it right away.

I opened a new PR against the plugins branch.

Please make sure to keep that feature branch updated with all your master changes. I'm relying on you to avoid big merge conflicts later.

In many Rust std lib cases, multiple functions with different names can be mapped into the same function name in Rhai. I think we need to make the Rust API more "Rhai-centric" by leveraging overloading instead of a simple one-to-one mapping.

I generally plan to handle overloading and multiple types with the flexibility in that call function, without using Rust generics. Sorry if I got confused by focusing on those.

To go back to a previous example of mine when I was thinking about varadic functions:

let forty_two = sum(40, 2);
let forty_two_again = sum(35, 5, 1, 1);
let forty_two_point_zero = sum(34.0, 6, 0.5, 0.5);

This is the kind of overloading you mean, right?

My plan for that was to make a function attribute that indicated a "varadic" function. If called, the iterator should be passed directly, and let the function itself handle it:

mod MathModule {
    #[rhai::export_fn(name = "sum", varadic)]
    pub fn add_anything(args: impl Iterator<Item=Dynamic + 'e) -> Result<Dynamic, EvalAltResult> {
        let total: FLOAT = 0.0;
        let return_float = false;
        for (n, i) in args.enumerate() {
            if let Some(i32_box) = i.down_cast::<i32>() {
                total += *i32_box as FLOAT;
            } else if let Some(f32_box) = i.down_cast::<f32>() {
                total += *f32_box;
                return_float = true;
            } else if let Some(i64_box) = i.down_cast::<i64>() {
                /* etc ... */
            } else {
                return Err(EvalAltResult::RuntimeError("argument {} is not a float or integer", n));
            }
        }
        Ok(Box::new(if return_float { total } else { total as INT }))
    }
}

I also hope this problem will be somewhat mitigated by broader use of this plugin style. It will hopefully encourage Rhai to develop more impl From<T> for Dynamic impl blocks for different Ts, to allow automatic casting to do more work.

[schungx]

Please make sure to keep that feature branch updated with all your master changes. I'm relying on you to avoid big merge conflicts later.

OK, I'll make sure I keep it up-to-date.

Your idea of keeping all the argument resolution inside the function implementation itself is a good solution to the problem of overloading. However, this puts the burden on the plugin author, who must then use a mass number of .is::<T>() or downcast::<T> calls to detect the correct argument types before dispatching to the correct implementation. This kind of dynamic dispatching is going to make calling each function slower (although I don't have benchmarks to prove that).

One way we can avoid this is to pass in &[Dynamic] instead of an iterator. This way, at least the author knows the number of arguments, and their types (he can run Dynamic.type_id() on each on to get the TypeId. He can then hash these TypeId's to quickly find the correct version of the function to dispatch to, and coerce the correct parameter types.

[schungx]

In a perfect world, we should provide macro facilities to do this kind of dispatching so the author doesn't have to worry about it!

[jhwgh1968]

Your idea of keeping all the argument resolution inside the function implementation itself is a good solution to the problem of overloading. However, this puts the burden on the plugin author, who must then use a mass number of .is::<T>() or downcast::<T> calls to detect the correct argument types before dispatching to the correct implementation.

I personally don't think this is a significant burden. As shown in my example, I originally came up with this design for algorithms which "fold types together" naturally in the course of their processing. It would be difficult to make sum any shorter using generics if you allowed such type mixing.

That said, I see your concern for functions which are "do the same thing, just accept different types." It may be possible to expand the procedural macro to support this, but my first idea is to expand the types on the Rust side.

For example, take add. It is a binary operation (not varadic, like my sum), and works on ints or floats in any combination.

I would write this definition:

mod CoreMath {
    pub fn add(x: Either<INT, FLOAT>, y: Either<INT, FLOAT>) -> Dynamic {
        match (x, y) {
            (Either::Left(i), Either::Left(j) => Dynamic::from(i + j),
            (Either::Right(i), Either::Right(j) => Dynamic::from(i + j),
            (Either::Left(i), Either::Right(j) => Dynamic::from(i as FLOAT + j),
            (Either::Right(i), Either::Left(j) => Dynamic::from(i + j as FLOAT),
        }
    }
}

Rhai would just need to ensure Dynamic could downcast from an INT or a FLOAT to that Either. (This might be the case already. It is difficult for me to follow the low-level type system APIs you are using.)

This would work with the procedural macro I already have in mind.

This kind of dynamic dispatching is going to make calling each function slower (although I don't have benchmarks to prove that).

My understanding is limited, but the impression I get is that the match shown above and the casting done in sum will be of the same cost after codegen.

Both of them will end up looking like a switch statement in C: choose where to jump based on whether this long integer's value (the type ID or pair of type IDs) is A, B, C, D, or anything else.

Accessing the type ID during the cast shouldn't be slow, either. The functional arguments will surely be in the CPU cache -- after the first conditional check, at the latest.

One way we can avoid this is to pass in &[Dynamic] instead of an iterator. This way, at least the author knows the number of arguments, and their types (he can run Dynamic.type_id() on each on to get the TypeId. He can then hash these TypeId's to quickly find the correct version of the function to dispatch to, and coerce the correct parameter types.

I find this direct use of TypeId unintuitive. I would rather write code which has Rust doing it for me in things like if and match statements.

The reason I picked a type of impl Iterator was because it wasn't clear to me whether you wanted to gather up all the arguments into one place before calling anything. Depending on the size of your Rhai stack, that might be a little slow compared to just writing an iterator to skip to the ones needed.

If you want callers able to know how many arguments they have -- for example, to support "wrong number of arguments" checks early on -- then I would happily change it to an iterator wrapper which implements ExactSizeIterator.

In a perfect world, we should provide macro facilities to do this kind of dispatching so the author doesn't have to worry about it!

My goal is to have procedural macros do as much work as I can. However, I am still figuring things out. A number of your ideas I agree with in general, but I hesitate to make many promises right now.

As things get beyond my first, simplest cases -- currently, all functions accept types passed by value, and return Dynamic -- I will more deeply consider some of your other ideas, and understand what is necessary and possible.

[jhwgh1968]

Also, a separate question: name spaces.

Currently my Plugin definition requires providing a static name. My original idea was to use this as a namespace. Perhaps something like:

import "crypto";
let encrypted_message = crypto::aes256_block("key", "message");

The use of crypto:: in this code would be the signal to Rhai to find the plugin with the name of crypto, and then invoke MyCryptoPlugin::call (the function generated by the procedural macro) without any regular function lookup steps.

Has any previous work on modules or packages nailed down a syntax I could hook into?

[schungx]

Has any previous work on modules or packages nailed down a syntax I could hook into?

Not really, so you can "modularize" the plugins. However, I'd say let's keep the namespacing/modules syntax in sync so we don't get into a conflict later on. At the least we'd need to support user-defined namespaces/modules which can be imported in the same manner.

And are you going to support importing just one function inside a module?

[schungx]

(This might be the case already. It is difficult for me to follow the low-level type system APIs you are using.)

Unfortunately the particular design used in Rhai depends heavily on dyn Any trait objects, meaning that their types are erased. Just about the only thing you can do is to use is::<T>() or the TypeId to check if it is of a particular type - you need to know the type you're checking in advance.

For example, you won't even know if a dyn Any type is Clone or Copy etc. This means that any use of a type outside of the built-in's in Dynamic (e.g. u16) will be converted into a heap-allocated, boxed trait object of dyn Any with all type info erased except its TypeId.

This is the way Rhai handles custom objects that it doesn't know about.

[schungx]

mod CoreMath {
    pub fn add(x: Either<INT, FLOAT>, y: Either<INT, FLOAT>) -> Dynamic {
        match (x, y) {
            (Either::Left(i), Either::Left(j) => Dynamic::from(i + j),
            (Either::Right(i), Either::Right(j) => Dynamic::from(i + j),
            (Either::Left(i), Either::Right(j) => Dynamic::from(i as FLOAT + j),
            (Either::Right(i), Either::Left(j) => Dynamic::from(i + j as FLOAT),
        }
    }
}

Usually this is enough, if we're only supporting INT and FLOAT without regards to other integer types. So far, most of the use for generics is actually for the wide list of integer types, and things like print, to_string that have the same name working on different types.

to_string is actually a good example. Remember, Rhai functions can be called method-style, meaning you should be able to do:

import aes256_block from "crypto";
let encrypted_message = "key".aes256_block("message");

Meaning that a function call may not always be possible to be qualified with a namespace. Of course, you can force everybody to use namespaced calls instead and disallow method-style, but that would be very un-Rhai...

For a function like to_string, naturally you have tons of implementations, one for each type.

Now, image somebody writes a new plugin handling yet another data type. He/she would obviously want to implement to_string for those types, and be able to just call it postfix-style. Therefore, you STILL need to have a way to resolve the same function name purely based on the parameter types just to handle this kind of overloading.

In your system, everything must be pre-baked-in. That is, you'll have one to_string function with a massive switch statement inside that detects all the types it can handle. If the user adds a new type in a new plugin, the stock to_string implementation won't know about it.

[schungx]

The plugins branch is caught up with the latest changes - nothing touched in the function libraries other than minor bug fixes.

[jhwgh1968]

a function call may not always be possible to be qualified with a namespace. Of course, you can force everybody to use namespaced calls instead and disallow method-style, but that would be very un-Rhai...

I am currently prototyping the simplest answer to that question: a module can only define receiver methods on types that it exports. When a type is imported by Rhai, all receiver methods come with it.

This is sufficient for my use case, which is about writing types and functions in Rust, and easily exporting them to Rhai. But I recognize that this will not suffice cases where users want to extend the standard library, or do a more dynamic use module::trait the way that Rust can.

In other words, my current prototype allows this Rhai code:

import "crypto";
// use a type in the crypto module, which is now in global namespace
let cipherstate = Aes256::CBC::new("my key", 0 /* the IV */);
let block_one = cipherstate.encrypt("message 1");
block_one /* returns as a string, or perhaps a JS like ArrayBuffer type */

But not something like this, which is what you seem to be describing:

let s = "abc";
let r = s.reverse(); // ERROR: no such method "reverse" on type "string"
import "string_utils" with traits;
let r = s.reverse(); // OK, because the plugin defined "reverse" on &mut String

While it would be easy to write a separate attribute to tie into such syntax, there is the problem of conflicts.

Currently, Rhai plugin loads are presumed infallible. I wasn't going to write them this way at first, but everything they do to the Engine seems to be infallible. It's not clear what happens if two modules try to add a reverse method, for example, to the built-in string.

If there were two calls to register_receiver_fn, with the same name and signature, would the second one panic? Or would it just overwrite the first?

[jhwgh1968]

In addition, let me go back to to_string for a moment.

The function to_string has an implementation for a lot of different types. As you said, what if a user wanted to print their own type with to_string?

My off-the-cuff answer would be: there is a global to_string the way there is now, and if a new type is defined in a plugin, it could "derive" that one:

module Crypto {
    pub struct CryptoState { /* ... */ }
    impl CryptoState {
        #[rhai::plugin::derive(to_string)

        /* other explicit methods here */
    }
}

The derive would simply cause code something like this to execute:

engine.register_receiver_fn("to_string", |st: &mut CryptoState| {
    // get the current global to_string function
    let dyn_fn_ptr = engine.get_dynamic_fn("to_string");
    //execute it on our parameter, returning the result
    dyn_fn_ptr(Dynamic::from(st))
})

Code achieving that is TBD.

[schungx]

If there were two calls to register_receiver_fn, with the same name and signature, would the second one panic? Or would it just overwrite the first?

I believe the second one overwrites the first right now...

[schungx]

Regarding to_string, currently string handling in Rhai is a bit ad hoc...

For a new type, you need to define to_string, print (which lets it to print), debug (letting it debug print), as well as the +(string, type) and +(type, string) operators to do string concatenation.

Not only that, you'd also want to override Array.push to let it be added into an Array, plus maybe a few other combinations to work with other standard types.

Not the best design, I admit, but that's how it is originally structured. Any ideas on your side to really streamline/automate this would be appreciated!

[schungx]

import "string_utils" with traits;
let r = s.reverse(); // OK, because the plugin defined "reverse" on &mut String

Can we just do:

import "string_utils";

if reverse is defined there?

[jhwgh1968]

Can we just do:

import "string_utils";

if reverse is defined there?

The only reason I wrote that was thinking about Rust's #[macro_export].

They wanted you to opt-in to procedural macros, because the macros would show up in the global namespace. That might cause conflicts you didn't want.

In this case, the conflicts would be between modules, if "traits" were always imported:

import "string_utils"; // contains a reverse method on &mut String
import "string_cesar_cipher"; // also contains a reverse method on &mut String. Oh no!

The with traits is outside of the namespaces proposal, though, since I haven't planned that far ahead.

[schungx]

Good point here, about potential name conflicts. In that case,, even if we force the user to with traits, there is always a chance that the user wants one trait from one module and another trait from another module, which happens to have a conflicting function.

We'd probably need to do something similar to JS:

import reverse from "string_utils";

s.reverse();

or in case of conflicts:

import reverse as reverse1 from "string.utils";
import reverse as reverse2 from "my_string_utils";

s.reverse1();
s1.reverse2();

And if we have this, mind as well open this up to all other imports:

import "crypto";           // crypto::encrypt(...);    import all under namespace
import * from "crypto";     // encrypt(...);   import all under global
import encrypt as en from "crypto";    // en(...);
import { ??? as ???, ???, ??? as ??? } from "????";    // multiple imports

[schungx]

@jhwgh1968 I've kept the plugins branch up-to-date with the modules work.

Now you can program your macros to the modules. Except there is no way to do the with traits thing right now to dynamically add methods to existing types...

Your idea of mapping an import statement to an entry in the current Scope works wonders and basically makes modules relatively free to implement. You have any good ideas on how to do the with traits?

[jhwgh1968]

The with traits idea is something I have not spent much thought on, I'm afraid. I just got time to review your modules implementation, and that is what I am currently focused on.

My design instinct tells me:

1. Split trait functions into a separate FunctionsLib in every Module.
2. Make get_fn and friends take an extra boolean that means "include traits in the search."
3. Add a boolean to the Stmt::Import enum in the AST, to indicate whether with traits was provided (which is itself only two tokens, or perhaps even one).
4. Have the engine set the get_fn boolean based on the Stmt::Import flag used on the corresponding module.

Does that help? I know this leaves unanswered questions, such as "how do I know this trait is for another type, rather than my type?" but I am deferring those until you can answer a higher priority question below.

---

As for the modules implementation, it took me a while to get my head around what I need to do for plugins, but I think I can make progress.

In particular, I hope to commit a basic plugins implementation soon, which can create new Modules and attach them to a StaticModuleResolver.

However, I'm only committing it because I am a big fan of the old saying, "do it first, do it well second." If I were to start on the procedural macro from that PR, it would fail to achieve some of the benefits, and might make some of the features we discussed above more difficult to implement.

Everything I wrote up there relied on my low-overhead lookup strategy: resolve the namespace to a plugin ahead of time, but defer resolving namespace members until a script accesses them. That is what the macro would generate.

Your current modules implementation is still incompatible with that approach. A plugin can now enable the StaticModuleResolver, and then set_fn and set_var for all of its contents; but that will box everything up into Dynamic and Arc<FnDef>.

I think my approach would require writing a PluginModuleResolver, which could perform the namespace lookups in a different way; and that would then conflict with a significant amount of your code.

In the process of writing this post, I now realize I need to separate these two related ideas in my head:

1. Plugins based on procedural macros help Rhai users with the syntax of adding Rust items into the Rhai namespace.
2. Plugins based on procedural macros help Rhai users add Rust items into the Rhai namespace with less overhead than was previously possible.

How would you rate those, @schungx? Is one worthwhile without the other? I remember you expressed praise for the procedural macro idea, but it might have just been item 1 you were thinking of.

[schungx]

do it first, do it well second

Wise words indeed! That's how I define "hacking".

I think my approach would require writing a PluginModuleResolver, which could perform the namespace lookups in a different way; and that would then conflict with a significant amount of your code.

I don't think a custom module resolver can help in this case, because it seems like you'd want to lazy-load functions and variables, defer until the first time they're used. Correct? That means that, when the module is attached to an Engine, it doesn't contain anything.

However, currently in the interest of speed, I hash up all the functions and variables in the entire tree once a module is loaded. Doing this has the great advantage of making module-qualified function calls and variable access as fast as normal. However, this also means that you'd need to have all of them ready in the very beginning. Sort of a catch-22...

but defer resolving namespace members until a script accesses them

If this is lazy-loading, I think that's a wonderful idea. Let's not lose it just because of the current architecture. Let's think of a way to have our cake and eat it.

How would you rate those

Well, I must admin I was thinking of No.1. Haven't really thought about No.2 as being possible at all, until you raise it up now. If it is at all possible, then No.2 is clearly the better solution. That would make loading up Engine's extremely fast because none of the built-in functions need to be loaded until they are needed!

However, I would really like to know more about what you're delaying. Which workload is put off until needed? Because I think you'll need a large lookup table just to know the names of all the possible functions to match for, and their parameter types... so essentially you're not loading anything less than what's done currently. You're essentially only saving a boxed function pointer, which is one allocation.

[jhwgh1968]

Also, as a general comment since it has now scrolled off the page: this to-do list I am keeping up-to-date. If you find it useful, perhaps clean up the original issue text, and link to it?

[jhwgh1968]

Today, I was going to work on supporting nested submodules, which would require significant refactoring. After that, I would do the handling of names that are not valid Rust idents.

When I just looked at the plugins branch and your PR comments, however, it seems you really are eager to get that fixed. Alright, I can do that today instead. I have fetched the branch in its current state, and will use your work as a starting point.

However, if you want me to open a PR for it, please either fix the CI or delete the commits that broke it.

[schungx]

When I just looked at the plugins branch and your PR comments, however, it seems you really are eager to get that fixed. Alright, I can do that today instead. I have fetched the branch in its current state, and will use your work as a starting point.

Yes, I would pick fixing the naming as the highest priority, because that's preventing a large amount of work from being moved to plugins. Sub-modules is a nice to have, but can be solved simply by splitting into separate modules.

However, I can say making feature gates #[cfg(...)] work inside modules is a higher priority than sub-modules, because mostly sub-modules are used to conditionally include functions based on feature gates. If feature gates work inside modules, then we don't really need sub-modules.

However, if you want me to open a PR for it, please either fix the CI or delete the commits that broke it.

My commits are just an experiment to see if the idea works and whether it simplifies writing libraries. I wasn't even sure if it is implemented correctly. Feel free to overwrite it with something that is more consistent. Nevertheless, I'll fix the CI tests so at least it is a singe unit.

I wonder whether I can just open a "PR" for my own repo instead of always having to commit directly into it so I won't muck things up... maybe the right thing to do is to open up a new branch?

[schungx]

OK, I've tried fixing the CI, but I hit on a problem with ui_tests.

Some of the outputs don't match simply by having different spans pointing to the error. For example:

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: this type in this position passes from Rhai by value
  --> $DIR/second_shared_ref.rs:12:41
   |
12 | pub fn test_fn(input: Clonable, factor: &bool) -> bool {
   |                                         ^^^^^

error[E0425]: cannot find function `test_fn` in this scope
  --> $DIR/second_shared_ref.rs:23:8
   |
23 |     if test_fn(n, &true) {
   |        ^^^^^^^ not found in this scope
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: this type in this position passes from Rhai by value
  --> $DIR/second_shared_ref.rs:12:41
   |
12 | pub fn test_fn(input: Clonable, factor: &bool) -> bool {
   |                                         ^

error[E0425]: cannot find function `test_fn` in this scope
  --> $DIR/second_shared_ref.rs:23:8
   |
23 |     if test_fn(n, &true) {
   |        ^^^^^^^ not found in this scope
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

From what I can see, the errors are identical except for the position of the arrows. I am not sure if this is a difference with the compilers we are using.

I hesitate to fix them just to find out that it is failing for you...

EDIT:

So, OK, yes it is passing on Linux. So probably a Windows/Linux thing.

The CI is now passing.

[jhwgh1968]

I wonder whether I can just open a "PR" for my own repo instead of always having to commit directly into it so I won't muck things up... maybe the right thing to do is to open up a new branch?

I think PRs against your own repo are the best way to go. It would not only run CI, but let me review it.

I will update you on my progress below, which will include comments that I would have put in a code review.

From what I can see, the errors are identical except for the position of the arrows. I am not sure if this is a difference with the compilers we are using.

Yes, that is another small difference in macro behavior between the current stable and nightly. I was going to warn you, but I couldn't find the time to mention it. I guess this is it! 😄

I'm not 100% sure what causes it, but I wrote my tests to enforce the more "readable" behavior of nightly, where the entire parameter type is highlighted. That is the token span I'm really interested in making sure is preserved, when I am coding away.

However, this error does not affect CI. I specifically set up the Codegen Build job to only use nightly so that it would match the expected output.

I was planning to add a "trybuild" feature at some point, which would be on by default, and you could disable if you were going to do codegen library development on stable. I guess that day is here.

---

Now, an update on the change in name behavior.

It turns out, your code didn't need any changes. When I open my PR, you will find your commit cherry-picked as-is.

The main issue I had with your change was that the tests were not updated for the behavior change you made. At a minimum:

• One of the tests in test_functions is now invalid, since #[export_fn(name = ...)] now does nothing
• A new test should be written in test_modules, which replaces that test, by putting everything inside a module to check the renames

I have already fixed this in my soon-to-be PR. But then, I hit a second issue. This demonstrates why macro development is so test heavy.

When I cherry-picked your second commit which updated Rhai to use #[rhai_fn(name = ...)] in the new way, it compiled... but some very strange test failures popped up. As I messed around with features, or tried to comment out specific things to see what other tests did, this snowballed more and more!

All of them were along the same lines: a function called in a script didn't do what it was supposed to, didn't return what it was supposed to, or didn't return the type it was supposed to.

Based on that, I thought: "Hmm... perhaps when @schungx did those renames, he created some name conflicts in his modules by accident, and functions overwrote each other. This tells me that I should implement rename collision detection, and throw an error on that."

So that's what I am working on now.

(And yes, I do remember that I have to check both the name and all the parameters before I call something a "collision".)

[schungx]

I think PRs against your own repo are the best way to go. It would not only run CI, but let me review it.

Do you know how to do that? I searched a bit and didn't come up with anything. Seems like I need to create a branch...

When I cherry-picked your second commit which updated Rhai to use #[rhai_fn(name = ...)] in the new way, it compiled... but some very strange test failures popped up. As I messed around with features, or tried to comment out specific things to see what other tests did, this snowballed more and more!

This is strange because the tests pass on my side... I suspect when my changes are merged with your existing changes, some logic gets confused in the middle...

I'd advise just junking all my changes and then reapplying them on top of your existing version.

However, keep my changes to the packages files. They are already written to take advantage of the new naming arrangement as an experiment. You can use them as a first test to make sure that everything works as planned.

[jhwgh1968]

Do you know how to do that? I searched a bit and didn't come up with anything. Seems like I need to create a branch...

Oh! Yeah, sorry, I misread your question.

My suggestion is, create a plugins_dev branch, and then open pull requests against plugins from that. If you go to Pull Requests and click New Pull Request, you can manually select plugins as the destination and plugins_dev as the source of the PR.

However, keep my changes to the packages files. They are already written to take advantage of the new naming arrangement as an experiment. You can use them as a first test to make sure that everything works as planned.

I am planning to do that. However, that is what caused my issues.

... Actually, I just realized I don't have the very last commits that made the CI green. One of them made changes to the string package. I wonder if that will fix the issues I'm seeing?

Even if it does, though, I still think it suggests rename duplicate detection is a good idea. I will open my PR once I am done with that.

[schungx]

... Actually, I just realized I don't have the very last commits that made the CI green. One of them made changes to the string package. I wonder if that will fix the issues I'm seeing?

Yes I fixed a bug in the Strings package. It wasn't due to naming conflict, but I put in the wrong operator.

[schungx]

My suggestion is, create a plugins_dev branch, and then open pull requests against plugins from that. If you go to Pull Requests and click New Pull Request, you can manually select plugins as the destination and plugins_dev as the source of the PR.

plugins_dev created. If I push something in there, I suppose you can just compare it with your fork to get the changes, right?

[schungx]

FYI... I pushed some commits with a bug fix for method-call detection of plugins.

9e7516b

It was not recognizing method calls from plugins correctly.

[jhwgh1968]

plugins_dev created. If I push something in there, I suppose you can just compare it with your fork to get the changes, right?

I was thinking you would actually open a PR. That is also so I could review it, and point out things like: you should probably write a test for X case.

FYI... I pushed some commits with a bug fix for method-call detection of plugins.

Thanks for catching that. I never would have!

---

I have opened a PR with duplicate detection, and some test fixes.

[schungx]

I was thinking you would actually open a PR. That is also so I could review it, and point out things like: you should probably write a test for X case.

Yes, I plan to open PR's from plugins_dev to plugins but it was a simple bug fix so I just pushed it in. Next time I'll open a PR instead.

I tried creating a PR to your fork, but it generates a godzillion commits dating all the way back to history, so I'm not sure if that's a good idea...

[jhwgh1968]

I tried creating a PR to your fork, but it generates a godzillion commits dating all the way back to history, so I'm not sure if that's a good idea...

To clarify, I really meant: open a pull request to merge from schungx/plugins_dev to schungx/plugins, and @ me to review them.

You are correct, my repo branches are out of date, because I don't keep my copy in very good shape except for rebasing purposes.

I plan to open PR's from plugins_dev to plugins but it was a simple bug fix so I just pushed it in. Next time I'll open a PR instead.

In fact, I would ask you to open a PR and let me review any macro change, even if it is small or fixing a bug. (Your most recent bug fix regarding methods was fine, because it did not touch files in codegen/.)

Even if I don't have much to say, I would still like a review for two things:

1. To suggest test updates. While we are both big fans of "hack first, polish second," I think macros need a higher bar. Like the Rust compiler itself, the number of uses and interactions is so large, every "hack" also needs to have tests to guard against future regressions that can be subtle but serious.

2. To understand what the change affects. As I noted in the last PR, I wrote a rule narrowly because I hadn't figured out all the effects of changing the way the name attribute worked. A review is encouragement and focus for me to sit down and do that. Flipping through a group of commits when I'm thinking about something else is a different mindset.

At the moment, both of these are currently missing for the getter and setter generators you implemented. I don't actually know what that code does at the moment, and I don't know if I will accidentally break it in some subtle way as I continue forward.

I have it on my personal list to back-fill, after I get submodules (including submodule attributes) finished. I would like that list to not get any longer in the mean time.

[jhwgh1968]

I updated the current state of the to-do list, and it got me thinking about the eventual release.

Since we are making good progress, my understanding is that this will ship for Rhai 0.19.0. However, like the Rust 2018 edition and some other features in the Rust compiler, this is a big change.

Even if the main parts of Rhai use it, I imagine end users will find new and clever ways to break the macros. They will do things that are wrong that "experts" (you and I) would not do, and cause it to panic! and not tell them why.

While I plan to try a bunch of experiments for negative testing, I know it won't catch even half of the problems. Every time something is called "fool proof", the universe sends a bigger fool to disprove it. 😄

Given that, how would you suggest we release this? I can think of several ideas:

1. Just release 0.19.0, and expect to make a lot of point releases before 0.20.0 (which every single macro fix would require, due to cargo dependencies). My only worry is, I don't know how many bugs will show up, or how easy they'll be to fix. If you can fix or help triage them, then maybe this will work. (The main thing we have not discussed is my process for adding more readable error messages, which I imagine will be some number of the fixes.)

2. A "soft release" for a public beta. Cut and tag version 0.19.0, but don't push it to crates.io for a while. Keep the macro documentation in the Rhai book separate. Encourage people to try the new version and report bugs. For my own personal use case of Rhai, and several other people who had early interest in things like game development, this would work just fine.

3. Release version 0.19.0, but don't document the macro parts at all. Maybe try to feature gate some parts until 0.20.0, and ask people to opt-in if they can. I don't think we could feature gate all of it because of the crate split between rhai and rhai_codegen, so this might be more confusing than helpful.

What do you think, @schungx?

[schungx]

I would suggest release 0.19.0 with macros, but before that merge it into the master repo so people can have some head warning to flush out bugs. But the real bugs do tend to show up after we release to crates.io, so I guess we have to go through that step anyway.

Right now the macros are restricted to implementing the standard library (and some portions are still not ported to macros yet - I'm waiting for support of feature gates in modules). As long as all the tests pass, that shouldn't affect stuff much.

The section in the documentation on how to create custom packages need to be scrapped and rewritten. It should be a new complete section on macros and how to use them. I can start on that, if you don't think the public API is going to change much. As it stands, it is already extremely useful and quite clean.

[schungx]

I think as it stand right now, plugins is probably ready to merge into schungx:master.

What do you think?

[jhwgh1968]

I was thinking you might be able to clean things up more after I merge submodules and #[rhai_mod(...)] attribute support. But if you can make progress on other things on master based on what is already here, then you'll get no push back from me!

EDIT: I'm presuming "your master", not "the official master" where anyone else could use it. I think it is currently for "internal use only" at the moment.

[jhwgh1968]

The section in the documentation on how to create custom packages need to be scrapped and rewritten. It should be a new complete section on macros and how to use them. I can start on that, if you don't think the public API is going to change much. As it stands, it is already extremely useful and quite clean.

I am confident the API I currently have will expand not much, and what does will be backward compatible. Getting started on that might not be a bad idea.

[schungx]

I was thinking you might be able to clean things up more after I merge submodules and #[rhai_mod(...)] attribute support. But if you can make progress on other things on master based on what is already here, then you'll get no push back from me!

Well, sub-modules are not as useful without feature gates. The main attraction of sub-modules is to put them under feature gates to include/exclude entire sections of functions at once.

EDIT: I'm presuming "your master", not "the official master" where anyone else could use it. I think it is currently for "internal use only" at the moment.

Yes, I mean schungx : master

[schungx]

BTW, I have run some benchmarks on plugins. Seems like we are looking at roughly 10% speed regression.

We might want to start fine-tuning the macros to generate code that has less regression.

The regression might be coming from the fact that all function calls go through an additional level of function call (i.e. the PluginFunction::call interface, although I suppose a smart compiler should be inlining this...

[jhwgh1968]

Well, sub-modules are not as useful without feature gates. The main attraction of sub-modules is to put them under feature gates to include/exclude entire sections of functions at once.

Yes, I am working on it now.

BTW, I have run some benchmarks on plugins. Seems like we are looking at roughly 10% speed regression.

We might want to start fine-tuning the macros to generate code that has less regression.

I am a little surprised it's that high, but I have not looked at any assembly in many, many revisions.

The regression might be coming from the fact that all function calls go through an additional level of function call (i.e. the PluginFunction::call interface, although I suppose a smart compiler should be inlining this...

Avoiding that runtime dispatch would require speculative devirtualization, which is being worked on for LLVM, but is not implemented yet to my knowledge.

When I was designing this, my intention was to make that the one runtime call in the entire code path. Given the amount of previous indirection in packages and modules when I looked at it, I presumed this would be similar in performance, if not faster. That is why I'm surprised to see that the penalty is so high.

[jhwgh1968]

Could you also try adding debug symbols to your benchmark test binary and running it with valgrind --tool=cachegrind? That is where I always start when I'm investigating performance.

That would tell us if there are instruction cache misses (and if they are tied to the dynamic lookup), or if there are data fetch misses (caused by data structures not fitting well or cold/random accesses).

[schungx]

I'll still investigating the performance angle. Seems that many of the benchmarks simply do not even touch on plugin functions. Therefore the regression probably has nothing to do with plugins.

Could be my machine running some other tasks in the background (virus scanners can be sneaky and start scanning when you don't notice).

No need to be alarmed right now... The differences are not significant enough to be over the error bars. However, they do show a systematic bias which, as I mentioned, may be simply due to my machine.

[jhwgh1968]

As I'm working through #[cfg] handling, I've hit something strange.

Currently, in the string package, there is this code:

#[cfg(not(feature = "no_object"))]
#[export_fn]
pub fn format_map(x: &mut Map) -> ImmutableString {
    format!("#{:?}", x).into()
}

Believe it or not, that shouldn't work.

When I run cargo expand to see what code is generated, I do indeed get what I expected:

#[cfg(not(feature = "no_object"))]
pub fn format_map(x: &mut Map) -> ImmutableString {
    {
        let res = /* format macro expansion */;
        res
    }
    .into()
}
#[allow(unused)]
pub mod rhai_fn_format_map {
    use super::*;
    struct Token();
    impl PluginFunction for Token {
        fn call(
          &self,
            args: &mut [&mut Dynamic],
            pos: Position,
        ) -> Result<Dynamic, Box<EvalAltResult>> {
            /* debug assert macro expansion */
            let arg0: &mut _ = &mut args[0usize].write_lock::<Map>().unwrap();
            Ok(Dynamic::from(format_map(arg0)))
       }
       /* rest of the macro output ... */
}            

If the #[cfg] block expression is false, the function will be ignored by the compiler, and not the generated module next to it. This would cause the error cannot find function 'format_map' on the Ok(...) line of the macro, as it does on a playground repro.

Yet somehow, when the #[cfg] block is false, both the function and the entirety of the macro output disappears? 🤷‍♂️

Since I don't trust this magic, I am going to forbid mixing #[cfg] and #[export_fn]. I'm having to detect and forbid #[cfg] in other places anyway.

This is the sole place in your code you did that, so I have no problem updating this to the new way to use #[cfg] as part of my next PR.

[jhwgh1968]

I'll still investigating the performance angle. Seems that many of the benchmarks simply do not even touch on plugin functions. Therefore the regression probably has nothing to do with plugins.

Ah. I was under the impression you changed enough core parts that users couldn't avoid them. In that case, I'll ignore it for now.

[schungx]

Since I don't trust this magic, I am going to forbid mixing #[cfg] and #[export_fn]. I'm having to detect and forbid #[cfg] in other places anyway.

Well, as far as I can tell, feature gates and #[export_fn] worked just fine together. Before the name= parameter, I used them extensively.

Try expanding with no_object is true...

EDIT: I just tested it and there is no pub mod rhai_XXX when the feature gate is false. Nor is there the function.

EDIT 2: If the feature gate is true, then it errors out because the function is still missing. Actually, in the strings package, I put it inside a module and put the feature gate on the module. That's why it worked.

#[cfg(not(feature = "no_object"))]
mod format_map {
    use super::*;
    #[inline]
    #[export_fn]
    pub fn format_map(x: &mut Map) -> ImmutableString {
        format!("#{:?}", x).into()
    }
}

[jhwgh1968]

I made the latter change as part of my PR.

I'm glad I put that rule in!