schungx / rhai Goto Github PK

Currently there are several places where I think we can make function calls/setters more efficient, more consistent across the language, and more like languages like js. I'm proposing these simultaneously because there's a fair bit of interaction between the different parts of the proposal.

Part 1: Introduction of get_mut

In index chains the built in types (arrays, maps, strings) efficiently create a Target which is basically just a mutable pointer to the type to be modified. In contrast foreign types don't do this, and instead we first call a getter to get the value, then we evaluate the thing to it's right in the chain, then we call set to set the value to the new value of the thing on the left.

I think we can do better. Specifically we can create

pub trait ObjectGetMutCallback<T, U>: Fn(&mut T) -> &mut U + [...] {}

pub fn register_get_mut<T, U, F>(&mut self, name: &str, callback: F)
where
    T: Variant + Clone,
    U: Variant + Clone,
    F: ObjectGetMutCallback<T, U>

And have the foreign types work like structs and arrays. They take in a pointer to themselves and they output a pointer to the type that needs modifying.

This eliminates roughly half the function calls in an index chain, and it lets user supplied code work with non-Dynamic values without constantly converting to and from Dynamic. It means that instead of having to create both a getter and a setter, users could just create a mut_getter. And finally it enables some of the proposals below which I believe are further improvements.

Obviously there's quite a bit of type level work that goes into convincing the rust compiler something like this is safe. I've finished what I believe are all the hard parts of this implementation here.

Part 2: Restricting setters to the ends of chains

Calling all the setters in chains is a lot more work, it's also unexpected behavior when coming from a lanuage like javascript that only calls the setter if it's the last element in the chain. Since with the get_mut proposal above we won't be losing much flexibility by adding a similar restriction, I'd propose that we limit setters to only being called if they are at the end of the chain.

In addition to the consistentcy with other languages, this lets us get rid of the whole backpropogating values back up the index chain to call setters. This lets us realize more of the performance wins that are possible with the get_mut and builtin types behavior of building a mutable pointer pointing at the right thing.

Moreover it doesn't lose much because we've implemented a substitute in get_mut. The only times get_mut isn't sufficient are probably also the times where calling the setter extra times is going to lead to surprising and undesired behavior (e.g. if the object is a database access object).

Part 3: Mutating methods first argument now a &mut T where T: Variant not a &mut Dynamic

This is fallout from part 1, but I view it as a positive. Suppose I have a

struct Foo{ x: i32 },
impl Foo{ fn get_mut_x(&mut self) -> &mut i32 }

and I register/call get_mut_x as a mut_getter. The Target that is being forwarded up the chain is now a &mut i32 (+ type information) instead of an &mut Dynamic, and it can't be changed to the latter since (amongst other things) there isn't enough memory allocated for a Dynamic. This means that if I want a mutable method to act on it, the method needs to be of the form Fn(val: Target, other_args...).

This ends up saving time not having to convert back and forth between a Dynamic for methods that actually want to act on a i32 (or a MyStructHere). It also introduces a separation between variables that users are intended to modify (passed as Target) and variables that should be treated as immutable (currently passed as &mut Dynamic, see Part 5).

Part 4 (Optional): Introduction of get_mut methods - aka chainable methods.

Optionally we could allow methods like the above get_mut getter that take in additional arguments. This would allow, e.g., mutable indexing of a user implemented container type. It would mean that we could do things like my_array.get(i) = 2 (and not throw away the 2 - which is what happens currently), and chain things like my_array_of_my_arrays.get(i).length.

For now these methods would be resticted to being used as methods, but it would be possible (just not obviously a good idea) to allow them in the form of mutable_function_call(a, x, y) = bar;

Part 4.1 (Optional): Introduction of user overridable indexing

Another possibility would be to do the above as register_index_get_mut::<Type>(fn). This would allow for the use of syntax like value_of_type[arg1, .., argn].

If we did this with getters and setters as well as get_mut we wouldn't have to special case the indexing of strings!

Part 5 (Optional): Foreign functions can take &[Dynamic] or &[&Dynamic] instead of &mut [&mut Dynamic].

A consequence of the above proposals is that we've now started dividing up function types inside the engine. Functions that take an argument they intend to modify take is as a Target consiting of a fat pointer directly to the object instead of a &mut Dynamic.

Since we need to make that change anyways, this is a perfect time to change the remaining arguments so that they are a &[Dynamic] (my current weak preference) or &[&Dynamic] (I suppose there might be a less-copying related argument for this type) instead of an &mut [&mut Dynamic].

This lets us get rid of extraneous allocations that builds an array of values, and then builds another array of references to those values (see below, taken from engine.rs:1251, I understand you might have removed the first set of allocations after I wrote this). It also makes the type signatures fit the intended use case, i.e. the type signatures will no longer suggest that users could modify arguments that in reality they could not modify.

let mut arg_values = arg_exprs
    .iter()
    .map(|expr| self.eval_expr(scope, state, fn_lib, expr, level))
    .collect::<Result<Vec<_>, _>>()?;

let mut args: Vec<_> = arg_values.iter_mut().collect();

Part 6: (Optional): Behavior of indexing non-existant fields on a map in a non-create fashion

Right now, if I've read the code right, accessing a non-existant property on a Map in a non-create fashion returns unit. This will change it to return an error - which feels moderately more consistent with indexing (but could be easily changed back if I'm wrong). Basically I'm trying to avoid the million dollar mistake being repeated with unit in place of null.

Proposal: namespaces

This proposal creates a new concept in Rhai called a namespace. The basics are:

• Namespaces live in a Scope as a third ScopeType. They do not conflict with variables.
• Every namespace has an alias, which is how Rhai code can access its contents.
• A namespace alias is created by the import statement in Rhai.
• Items within a namespace are accessed with a new "namespace access" operator, ::.
• Namespaces may contain only functions and constants.
• What defines the contents of a namespace is not defined by this proposal. It could be modules, packages, plugins, or custom modifications by downstream library users.

This proposal is designed to be minimal, and forward compatible with future decisions about namespace contents and access.

Syntax

In order to access a namespace, it must be bound to an alias with an import statement. There are two forms:

import "My New Namespace";
import "My Other Namespace" as ns2;

The primary argument to import is a UTF-8 string. This is not restricted to the characters in an identifier to allow greater flexibility in what can be keyed to a namespace, and to match import statements in more recent versions of JavaScript. (The "as" syntax was borrowed from Python.)

The second form explicitly gives the alias after the as token. The first form generates the alias by removing all tokens that are invalid in an identifier. The first example is equivalent to:

import "My New Namespace" as MyNewNamespace;

An import statement enables access to the contents of the namespace using the new "namespace access" operator, ::.

import "my namespace" as s;
let s = "abc";
let t = s[0]; // variables are not shadowed
let u = s::my_function();
let v = s::MY_CONST;

Final Thoughts

This is necessary for #3, but is turning out to be significant enough I'd like to nail down the syntax independently.

This has been talked about before, but it's not clear what decisions were made firmly about it, as many seemed tied to one particular implementation or another that was never finished.

I am also hoping, @schungx, that the code to parse the new syntax can be merged to master by itself. Even though no namespaces exist by default, it would still allow users to create them through interpreter modifications or custom scopes. I think that is a feature worth having, as it would partially help my use case for Rhai (a custom DSL) by itself.

I have the implementation about 20% done. If you approve, and can finish it faster, please do.

Tracking Issue

@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