From e1242df5c8bfea4dd0142c844218dccf890ab17a Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Fri, 29 May 2020 18:15:58 +0800 Subject: [PATCH] Extract copy/restore of first argument in method call. --- README.md | 20 ++++++++--- RELEASES.md | 10 +++++- src/engine.rs | 82 +++++++++++++++++++++++++------------------- src/module.rs | 6 ++-- tests/method_call.rs | 2 +- 5 files changed, 74 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 89da8bad..89728438 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ It doesn't attempt to be a new language. For example: * No classes. Well, Rust doesn't either. On the other hand... * No traits... so it is also not Rust. Do your Rusty stuff in Rust. -* No structures - definte your types in Rust instead; Rhai can seamless work with _any Rust type_. +* No structures - define your types in Rust instead; Rhai can seamlessly work with _any Rust type_. * No first-class functions - Code your functions in Rust instead, and register them with Rhai. * No closures - do your closure magic in Rust instead; [turn a Rhai scripted function into a Rust closure](#calling-rhai-functions-from-rust). * It is best to expose an API in Rhai for scripts to call. All your core functionalities should be in Rust. @@ -2094,14 +2094,24 @@ Members and methods ------------------- Properties and methods in a Rust custom type registered with the [`Engine`] can be called just like in Rust. +Unlike functions defined in script (for which all arguments are passed by _value_), +native Rust functions may mutate the object (or the first argument if called in normal function call style). ```rust let a = new_ts(); // constructor function -a.field = 500; // property access -a.update(); // method call, 'a' can be changed +a.field = 500; // property setter +a.update(); // method call, 'a' can be modified -update(a); // this works, but 'a' is unchanged because only - // a COPY of 'a' is passed to 'update' by VALUE +update(a); // <- this de-sugars to 'a.update()' this if 'a' is a simple variable + // unlike scripted functions, 'a' can be modified and is not a copy + +let array = [ a ]; + +update(array[0]); // <- 'array[0]' is an expression returning a calculated value, + // a transient (i.e. a copy) so this statement has no effect + // except waste a lot of time cloning + +array[0].update(); // <- call this method-call style will update 'a' ``` Custom types, properties and methods can be disabled via the [`no_object`] feature. diff --git a/RELEASES.md b/RELEASES.md index bc5a2125..79e0af4c 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -22,13 +22,16 @@ Breaking changes This is to avoid excessive cloning of strings. All native-Rust functions taking string parameters should switch to `rhai::ImmutableString` (which is either `Rc` or `Arc` depending on whether the `sync` feature is used). +* Native Rust functions registered with the `Engine` also mutates the first argument when called in + normal function-call style (previously the first argument will be passed by _value_ if not called + in method-call style). Of course, if the first argument is a calculated value (e.g. result of an + expression), then mutating it has no effect, but at least it is not cloned. New features ------------ * Set limit on maximum level of nesting expressions and statements to avoid panics during parsing. * New `EvalPackage` to disable `eval`. -* More benchmarks. Speed enhancements ------------------ @@ -43,6 +46,11 @@ Speed enhancements (and therefore the `CorePackage`) because they are now always available, even under `Engine::new_raw`. * Operator-assignment statements (e.g. `+=`) are now handled directly and much faster. * Strings are now _immutable_ and use the `rhai::ImmutableString` type, eliminating large amounts of cloning. +* For Native Rust functions taking a first `&mut` parameter, the first argument is passed by reference instead of + by value, even if not called in method-call style. This allows many functions declared with `&mut` parameter to avoid + excessive cloning. For example, if `a` is a large array, getting its length in this manner: `len(a)` used to result + in a full clone of `a` before taking the length and throwing the copy away. Now, `a` is simply passed by reference, + avoiding the cloning altogether. Version 0.14.1 diff --git a/src/engine.rs b/src/engine.rs index 8903e4a3..433cd77e 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -615,33 +615,54 @@ impl Engine { return Err(Box::new(EvalAltResult::ErrorStackOverflow(pos))); } } + + let mut this_copy: Dynamic = Default::default(); + let mut old_this_ptr: Option<&mut Dynamic> = None; + + /// This function replaces the first argument of a method call with a clone copy. + /// This is to prevent a pure function unintentionally consuming the first argument. + fn normalize_first_arg<'a>( + normalize: bool, + this_copy: &mut Dynamic, + old_this_ptr: &mut Option<&'a mut Dynamic>, + args: &mut FnCallArgs<'a>, + ) { + // Only do it for method calls with arguments. + if !normalize || args.is_empty() { + return; + } + + // Clone the original value. + *this_copy = args[0].clone(); + + // Replace the first reference with a reference to the clone, force-casting the lifetime. + // Keep the original reference. Must remember to restore it later with `restore_first_arg_of_method_call`. + let this_pointer = mem::replace( + args.get_mut(0).unwrap(), + unsafe_mut_cast_to_lifetime(this_copy), + ); + + *old_this_ptr = Some(this_pointer); + } + + /// This function restores the first argument that was replaced by `normalize_first_arg_of_method_call`. + fn restore_first_arg<'a>(old_this_ptr: Option<&'a mut Dynamic>, args: &mut FnCallArgs<'a>) { + if let Some(this_pointer) = old_this_ptr { + mem::replace(args.get_mut(0).unwrap(), this_pointer); + } + } + // First search in script-defined functions (can override built-in) if !native_only { if let Some(fn_def) = lib.get(&hashes.1) { - let mut this_copy: Option; - let mut this_pointer: Option<&mut Dynamic> = None; - - if is_ref && !args.is_empty() { - // Clone the original value. It'll be consumed because the function - // is pure and doesn't know that the first value is a reference (i.e. `is_ref`) - this_copy = Some(args[0].clone()); - - // Replace the first reference with a reference to the clone, force-casting the lifetime. - // Keep the original reference. Must remember to restore it before existing this function. - this_pointer = Some(mem::replace( - args.get_mut(0).unwrap(), - unsafe_mut_cast_to_lifetime(this_copy.as_mut().unwrap()), - )); - } + normalize_first_arg(is_ref, &mut this_copy, &mut old_this_ptr, args); // Run scripted function let result = self.call_script_fn(scope, state, lib, fn_name, fn_def, args, pos, level)?; // Restore the original reference - if let Some(this_pointer) = this_pointer { - mem::replace(args.get_mut(0).unwrap(), this_pointer); - } + restore_first_arg(old_this_ptr, args); return Ok((result, false)); } @@ -654,29 +675,18 @@ impl Engine { .or_else(|| self.packages.get_fn(hashes.0)) { // Calling pure function in method-call? - let mut this_copy: Option; - let mut this_pointer: Option<&mut Dynamic> = None; - - if func.is_pure() && is_ref && !args.is_empty() { - // Clone the original value. It'll be consumed because the function - // is pure and doesn't know that the first value is a reference (i.e. `is_ref`) - this_copy = Some(args[0].clone()); - - // Replace the first reference with a reference to the clone, force-casting the lifetime. - // Keep the original reference. Must remember to restore it before existing this function. - this_pointer = Some(mem::replace( - args.get_mut(0).unwrap(), - unsafe_mut_cast_to_lifetime(this_copy.as_mut().unwrap()), - )); - } + normalize_first_arg( + func.is_pure() && is_ref, + &mut this_copy, + &mut old_this_ptr, + args, + ); // Run external function let result = func.get_native_fn()(args); // Restore the original reference - if let Some(this_pointer) = this_pointer { - mem::replace(args.get_mut(0).unwrap(), this_pointer); - } + restore_first_arg(old_this_ptr, args); let result = result.map_err(|err| err.new_position(pos))?; diff --git a/src/module.rs b/src/module.rs index 57bca2e3..5c5d3d67 100644 --- a/src/module.rs +++ b/src/module.rs @@ -544,7 +544,7 @@ impl Module { /// use rhai::Module; /// /// let mut module = Module::new(); - /// let hash = module.set_fn_3("calc", |x: i64, y: String, z: i64, _w: ()| { + /// let hash = module.set_fn_4("calc", |x: i64, y: String, z: i64, _w: ()| { /// Ok(x + y.len() as i64 + z) /// }); /// assert!(module.get_fn(hash).is_some()); @@ -583,7 +583,7 @@ impl Module { ) } - /// Set a Rust function taking three parameters (the first one mutable) into the module, + /// Set a Rust function taking four parameters (the first one mutable) into the module, /// returning a hash key. /// /// If there is a similar existing Rust function, it is replaced. @@ -594,7 +594,7 @@ impl Module { /// use rhai::Module; /// /// let mut module = Module::new(); - /// let hash = module.set_fn_3_mut("calc", |x: &mut i64, y: String, z: i64, _w: ()| { + /// let hash = module.set_fn_4_mut("calc", |x: &mut i64, y: String, z: i64, _w: ()| { /// *x += y.len() as i64 + z; Ok(*x) /// }); /// assert!(module.get_fn(hash).is_some()); diff --git a/tests/method_call.rs b/tests/method_call.rs index 850cf247..02c7a950 100644 --- a/tests/method_call.rs +++ b/tests/method_call.rs @@ -33,7 +33,7 @@ fn test_method_call() -> Result<(), Box> { assert_eq!( engine.eval::("let x = new_ts(); update(x, 1000); x")?, - TestStruct { x: 1 } + TestStruct { x: 1001 } ); Ok(())