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.