From de906053ed468aa97f754207cceebac5ec43dc88 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 15 Nov 2021 11:13:00 +0800 Subject: [PATCH] Deprecate call_fn_dynamic into call_fn_raw. --- CHANGELOG.md | 7 ++-- src/api/deprecated.rs | 73 ++++++++++++++++++++++++++++++++ src/api/public.rs | 96 +++++++++++++++++++++---------------------- src/engine.rs | 46 +++++++++++++-------- src/func/call.rs | 48 ++++++++++++++-------- src/types/scope.rs | 10 +++++ tests/call_fn.rs | 30 ++++++++++---- 7 files changed, 218 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae7ec6b7..603545e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,8 @@ New features * `#[cfg(...)]` attributes can now be put directly on plugin functions or function defined in a plugin module. * A custom syntax parser can now return a symbol starting with `$$` to inform the implementation function which syntax variant was actually parsed. -* `AST::iter_literal_variables` extracts all top-level literal constant/variable definitions from a script without running it. -* `Scope::clone_visible` is added that copies only the last instance of each variable, omitting all shadowed variables. +* `AST::iter_literal_variables` is added to extract all top-level literal constant/variable definitions from a script without running it. +* `Engine::call_fn_dynamic` is deprecated and `Engine::call_fn_raw` is added which allows keeping new variables in the custom scope. Enhancements ------------ @@ -31,7 +31,8 @@ Enhancements * Array adds a `sort` method with no parameters which sorts homogeneous arrays of built-in comparable types (e.g. `INT`). * Inlining is disabled for error-path functions because errors are exceptional and scripts usually fail completely when an error is encountered. * The `optimize` module is completely eliminated under `no_optimize`, which should yield smaller code size. -* Add `NativeCallContext::position` to return the position of the function call. +* `NativeCallContext::position` is added to return the position of the function call. +* `Scope::clone_visible` is added that copies only the last instance of each variable, omitting all shadowed variables. Deprecated API's ---------------- diff --git a/src/api/deprecated.rs b/src/api/deprecated.rs index e4fa7e67..32ab5cff 100644 --- a/src/api/deprecated.rs +++ b/src/api/deprecated.rs @@ -111,6 +111,79 @@ impl Engine { ) -> Result<(), Box> { self.run_ast_with_scope(scope, ast) } + /// Call a script function defined in an [`AST`] with multiple [`Dynamic`] arguments + /// and optionally a value for binding to the `this` pointer. + /// + /// Not available under `no_function`. + /// + /// There is an option to evaluate the [`AST`] to load necessary modules before calling the function. + /// + /// # Deprecated + /// + /// This method is deprecated. Use [`run_ast_with_scope`][Engine::run_ast_with_scope] instead. + /// + /// This method will be removed in the next major version. + /// + /// # WARNING + /// + /// All the arguments are _consumed_, meaning that they're replaced by `()`. + /// This is to avoid unnecessarily cloning the arguments. + /// Do not use the arguments after this call. If they are needed afterwards, + /// clone them _before_ calling this function. + /// + /// # Example + /// + /// ``` + /// # fn main() -> Result<(), Box> { + /// # #[cfg(not(feature = "no_function"))] + /// # { + /// use rhai::{Engine, Scope, Dynamic}; + /// + /// let engine = Engine::new(); + /// + /// let ast = engine.compile(" + /// fn add(x, y) { len(x) + y + foo } + /// fn add1(x) { len(x) + 1 + foo } + /// fn bar() { foo/2 } + /// fn action(x) { this += x; } // function using 'this' pointer + /// ")?; + /// + /// let mut scope = Scope::new(); + /// scope.push("foo", 42_i64); + /// + /// // Call the script-defined function + /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "add", None, [ "abc".into(), 123_i64.into() ])?; + /// // ^^^^ no 'this' pointer + /// assert_eq!(result.cast::(), 168); + /// + /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "add1", None, [ "abc".into() ])?; + /// assert_eq!(result.cast::(), 46); + /// + /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "bar", None, [])?; + /// assert_eq!(result.cast::(), 21); + /// + /// let mut value: Dynamic = 1_i64.into(); + /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "action", Some(&mut value), [ 41_i64.into() ])?; + /// // ^^^^^^^^^^^^^^^^ binding the 'this' pointer + /// assert_eq!(value.as_int().expect("value should be INT"), 42); + /// # } + /// # Ok(()) + /// # } + /// ``` + #[deprecated(since = "1.1.0", note = "use `call_fn_raw` instead")] + #[cfg(not(feature = "no_function"))] + #[inline(always)] + pub fn call_fn_dynamic( + &self, + scope: &mut Scope, + ast: &AST, + eval_ast: bool, + name: impl AsRef, + this_ptr: Option<&mut Dynamic>, + arg_values: impl AsMut<[Dynamic]>, + ) -> RhaiResult { + self.call_fn_raw(scope, ast, eval_ast, true, name, this_ptr, arg_values) + } } impl Dynamic { diff --git a/src/api/public.rs b/src/api/public.rs index f29ea0d6..803036b1 100644 --- a/src/api/public.rs +++ b/src/api/public.rs @@ -1901,10 +1901,8 @@ impl Engine { ) -> Result> { let mut arg_values = crate::StaticVec::new(); args.parse(&mut arg_values); - let mut args: crate::StaticVec<_> = arg_values.iter_mut().collect(); - let name = name.as_ref(); - let result = self.call_fn_dynamic_raw(scope, ast, true, name, &mut None, &mut args)?; + let result = self.call_fn_raw(scope, ast, true, true, name, None, arg_values)?; let typ = self.map_type_name(result.type_name()); @@ -1918,12 +1916,14 @@ impl Engine { }) } /// Call a script function defined in an [`AST`] with multiple [`Dynamic`] arguments - /// and optionally a value for binding to the `this` pointer. + /// and the following options: + /// + /// * whether to evaluate the [`AST`] to load necessary modules before calling the function + /// * whether to rewind the [`Scope`] after the function call + /// * a value for binding to the `this` pointer (if any) /// /// Not available under `no_function`. /// - /// There is an option to evaluate the [`AST`] to load necessary modules before calling the function. - /// /// # WARNING /// /// All the arguments are _consumed_, meaning that they're replaced by `()`. @@ -1946,77 +1946,67 @@ impl Engine { /// fn add1(x) { len(x) + 1 + foo } /// fn bar() { foo/2 } /// fn action(x) { this += x; } // function using 'this' pointer + /// fn decl(x) { let hello = x; } // declaring variables /// ")?; /// /// let mut scope = Scope::new(); /// scope.push("foo", 42_i64); /// /// // Call the script-defined function - /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "add", None, [ "abc".into(), 123_i64.into() ])?; - /// // ^^^^ no 'this' pointer + /// let result = engine.call_fn_raw(&mut scope, &ast, true, true, "add", None, [ "abc".into(), 123_i64.into() ])?; + /// // ^^^^ no 'this' pointer /// assert_eq!(result.cast::(), 168); /// - /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "add1", None, [ "abc".into() ])?; + /// let result = engine.call_fn_raw(&mut scope, &ast, true, true, "add1", None, [ "abc".into() ])?; /// assert_eq!(result.cast::(), 46); /// - /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "bar", None, [])?; + /// let result = engine.call_fn_raw(&mut scope, &ast, true, true, "bar", None, [])?; /// assert_eq!(result.cast::(), 21); /// /// let mut value: Dynamic = 1_i64.into(); - /// let result = engine.call_fn_dynamic(&mut scope, &ast, true, "action", Some(&mut value), [ 41_i64.into() ])?; - /// // ^^^^^^^^^^^^^^^^ binding the 'this' pointer + /// let result = engine.call_fn_raw(&mut scope, &ast, true, true, "action", Some(&mut value), [ 41_i64.into() ])?; + /// // ^^^^^^^^^^^^^^^^ binding the 'this' pointer /// assert_eq!(value.as_int().expect("value should be INT"), 42); + /// + /// engine.call_fn_raw(&mut scope, &ast, true, false, "decl", None, [ 42_i64.into() ])?; + /// // ^^^^^ do not rewind scope + /// assert_eq!(scope.get_value::("hello").unwrap(), 42); /// # } /// # Ok(()) /// # } /// ``` #[cfg(not(feature = "no_function"))] #[inline] - pub fn call_fn_dynamic( + pub fn call_fn_raw( &self, scope: &mut Scope, ast: &AST, eval_ast: bool, + rewind_scope: bool, name: impl AsRef, - mut this_ptr: Option<&mut Dynamic>, - mut arg_values: impl AsMut<[Dynamic]>, - ) -> RhaiResult { - let name = name.as_ref(); - let mut args: crate::StaticVec<_> = arg_values.as_mut().iter_mut().collect(); - - self.call_fn_dynamic_raw(scope, ast, eval_ast, name, &mut this_ptr, &mut args) - } - /// Call a script function defined in an [`AST`] with multiple [`Dynamic`] arguments. - /// - /// # WARNING - /// - /// All the arguments are _consumed_, meaning that they're replaced by `()`. - /// This is to avoid unnecessarily cloning the arguments. - /// Do not use the arguments after this call. If they are needed afterwards, - /// clone them _before_ calling this function. - #[cfg(not(feature = "no_function"))] - #[inline] - pub(crate) fn call_fn_dynamic_raw( - &self, - scope: &mut Scope, - ast: &AST, - eval_ast: bool, - name: &str, - this_ptr: &mut Option<&mut Dynamic>, - args: &mut FnCallArgs, + this_ptr: Option<&mut Dynamic>, + arg_values: impl AsMut<[Dynamic]>, ) -> RhaiResult { let state = &mut EvalState::new(); let mods = &mut Imports::new(); - let lib = &[ast.lib()]; let statements = ast.statements(); + let orig_scope_len = scope.len(); + if eval_ast && !statements.is_empty() { // Make sure new variables introduced at global level do not _spill_ into the function call - let orig_scope_len = scope.len(); - self.eval_global_statements(scope, mods, state, statements, lib, 0)?; - scope.rewind(orig_scope_len); + self.eval_global_statements(scope, mods, state, statements, &[ast.lib()], 0)?; + + if rewind_scope { + scope.rewind(orig_scope_len); + } } + let name = name.as_ref(); + let mut this_ptr = this_ptr; + let mut arg_values = arg_values; + let mut args: crate::StaticVec<_> = arg_values.as_mut().iter_mut().collect(); + let fn_def = ast .lib() .get_script_fn(name, args.len()) @@ -2024,19 +2014,27 @@ impl Engine { // Check for data race. #[cfg(not(feature = "no_closure"))] - crate::func::call::ensure_no_data_race(name, args, false)?; + crate::func::call::ensure_no_data_race(name, &mut args, false)?; - self.call_script_fn( + let result = self.call_script_fn( scope, mods, state, - lib, - this_ptr, + &[ast.lib()], + &mut this_ptr, fn_def, - args, + &mut args, Position::NONE, + rewind_scope, 0, - ) + ); + + // Remove arguments + if !rewind_scope && !args.is_empty() { + scope.remove_range(orig_scope_len, args.len()) + } + + result } /// Optimize the [`AST`] with constants defined in an external Scope. /// An optimized copy of the [`AST`] is returned while the original [`AST`] is consumed. diff --git a/src/engine.rs b/src/engine.rs index 700225df..53d3669f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -2205,7 +2205,7 @@ impl Engine { // Statement block Expr::Stmt(x) if x.is_empty() => Ok(Dynamic::UNIT), Expr::Stmt(x) => { - self.eval_stmt_block(scope, mods, state, lib, this_ptr, x, true, level) + self.eval_stmt_block(scope, mods, state, lib, this_ptr, x, true, true, level) } // lhs[idx_expr] @@ -2374,6 +2374,7 @@ impl Engine { this_ptr: &mut Option<&mut Dynamic>, statements: &[Stmt], restore_prev_state: bool, + rewind_scope: bool, level: usize, ) -> RhaiResult { if statements.is_empty() { @@ -2385,7 +2386,7 @@ impl Engine { let prev_scope_len = scope.len(); let prev_mods_len = mods.len(); - if restore_prev_state { + if rewind_scope { state.scope_level += 1; } @@ -2427,10 +2428,12 @@ impl Engine { state.pop_fn_resolution_cache(); } - if restore_prev_state { + if rewind_scope { scope.rewind(prev_scope_len); - mods.truncate(prev_mods_len); state.scope_level -= 1; + } + if restore_prev_state { + mods.truncate(prev_mods_len); // The impact of new local variables goes away at the end of a block // because any new variables introduced will go out of scope @@ -2617,9 +2620,9 @@ impl Engine { // Block scope Stmt::Block(statements, _) if statements.is_empty() => Ok(Dynamic::UNIT), - Stmt::Block(statements, _) => { - self.eval_stmt_block(scope, mods, state, lib, this_ptr, statements, true, level) - } + Stmt::Block(statements, _) => self.eval_stmt_block( + scope, mods, state, lib, this_ptr, statements, true, true, level, + ), // If statement Stmt::If(expr, x, _) => { @@ -2630,13 +2633,17 @@ impl Engine { if guard_val { if !x.0.is_empty() { - self.eval_stmt_block(scope, mods, state, lib, this_ptr, &x.0, true, level) + self.eval_stmt_block( + scope, mods, state, lib, this_ptr, &x.0, true, true, level, + ) } else { Ok(Dynamic::UNIT) } } else { if !x.1.is_empty() { - self.eval_stmt_block(scope, mods, state, lib, this_ptr, &x.1, true, level) + self.eval_stmt_block( + scope, mods, state, lib, this_ptr, &x.1, true, true, level, + ) } else { Ok(Dynamic::UNIT) } @@ -2676,7 +2683,7 @@ impl Engine { Some(if !statements.is_empty() { self.eval_stmt_block( - scope, mods, state, lib, this_ptr, statements, true, level, + scope, mods, state, lib, this_ptr, statements, true, true, level, ) } else { Ok(Dynamic::UNIT) @@ -2690,7 +2697,7 @@ impl Engine { // Default match clause if !def_stmt.is_empty() { self.eval_stmt_block( - scope, mods, state, lib, this_ptr, def_stmt, true, level, + scope, mods, state, lib, this_ptr, def_stmt, true, true, level, ) } else { Ok(Dynamic::UNIT) @@ -2701,7 +2708,8 @@ impl Engine { // Loop Stmt::While(Expr::Unit(_), body, _) => loop { if !body.is_empty() { - match self.eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, level) + match self + .eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, true, level) { Ok(_) => (), Err(err) => match *err { @@ -2727,7 +2735,8 @@ impl Engine { return Ok(Dynamic::UNIT); } if !body.is_empty() { - match self.eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, level) + match self + .eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, true, level) { Ok(_) => (), Err(err) => match *err { @@ -2744,7 +2753,8 @@ impl Engine { let is_while = !options.contains(AST_OPTION_NEGATED); if !body.is_empty() { - match self.eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, level) + match self + .eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, true, level) { Ok(_) => (), Err(err) => match *err { @@ -2843,7 +2853,7 @@ impl Engine { } let result = self.eval_stmt_block( - scope, mods, state, lib, this_ptr, statements, true, level, + scope, mods, state, lib, this_ptr, statements, true, true, level, ); match result { @@ -2907,7 +2917,9 @@ impl Engine { let (try_stmt, err_var, catch_stmt) = x.as_ref(); let result = self - .eval_stmt_block(scope, mods, state, lib, this_ptr, try_stmt, true, level) + .eval_stmt_block( + scope, mods, state, lib, this_ptr, try_stmt, true, true, level, + ) .map(|_| Dynamic::UNIT); match result { @@ -2959,7 +2971,7 @@ impl Engine { }); let result = self.eval_stmt_block( - scope, mods, state, lib, this_ptr, catch_stmt, true, level, + scope, mods, state, lib, this_ptr, catch_stmt, true, true, level, ); scope.rewind(orig_scope_len); diff --git a/src/func/call.rs b/src/func/call.rs index 10bcb234..703bd677 100644 --- a/src/func/call.rs +++ b/src/func/call.rs @@ -487,6 +487,7 @@ impl Engine { fn_def: &crate::ast::ScriptFnDef, args: &mut FnCallArgs, pos: Position, + rewind_scope: bool, level: usize, ) -> RhaiResult { #[inline(never)] @@ -511,6 +512,8 @@ impl Engine { .into()) } + assert!(fn_def.params.len() == args.len()); + #[cfg(not(feature = "unchecked"))] self.inc_operations(&mut mods.num_operations, pos)?; @@ -565,7 +568,17 @@ impl Engine { // Evaluate the function let body = &fn_def.body; let result = self - .eval_stmt_block(scope, mods, state, unified_lib, this_ptr, body, true, level) + .eval_stmt_block( + scope, + mods, + state, + unified_lib, + this_ptr, + body, + true, + rewind_scope, + level, + ) .or_else(|err| match *err { // Convert return statement to return value EvalAltResult::Return(x, _) => Ok(x), @@ -589,7 +602,9 @@ impl Engine { }); // Remove all local variables - scope.rewind(prev_scope_len); + if rewind_scope { + scope.rewind(prev_scope_len); + } mods.truncate(prev_mods_len); if unified { @@ -735,7 +750,6 @@ impl Engine { empty_scope = Scope::new(); &mut empty_scope }; - let orig_scope_len = scope.len(); let result = if _is_method_call { // Method call of script function - map first argument to `this` @@ -755,6 +769,7 @@ impl Engine { func, rest_args, pos, + true, level, ); @@ -779,8 +794,9 @@ impl Engine { let level = _level + 1; - let result = - self.call_script_fn(scope, mods, state, lib, &mut None, func, args, pos, level); + let result = self.call_script_fn( + scope, mods, state, lib, &mut None, func, args, pos, true, level, + ); // Restore the original source mods.source = orig_source; @@ -793,8 +809,6 @@ impl Engine { result? }; - scope.rewind(orig_scope_len); - return Ok((result, false)); } @@ -817,14 +831,16 @@ impl Engine { lib: &[&Module], level: usize, ) -> RhaiResult { - self.eval_stmt_block(scope, mods, state, lib, &mut None, statements, false, level) - .or_else(|err| match *err { - EvalAltResult::Return(out, _) => Ok(out), - EvalAltResult::LoopBreak(_, _) => { - unreachable!("no outer loop scope to break out of") - } - _ => Err(err), - }) + self.eval_stmt_block( + scope, mods, state, lib, &mut None, statements, false, false, level, + ) + .or_else(|err| match *err { + EvalAltResult::Return(out, _) => Ok(out), + EvalAltResult::LoopBreak(_, _) => { + unreachable!("no outer loop scope to break out of") + } + _ => Err(err), + }) } /// Evaluate a text script in place - used primarily for 'eval'. @@ -1435,7 +1451,7 @@ impl Engine { let level = level + 1; let result = self.call_script_fn( - new_scope, mods, state, lib, &mut None, fn_def, &mut args, pos, level, + new_scope, mods, state, lib, &mut None, fn_def, &mut args, pos, true, level, ); mods.source = source; diff --git a/src/types/scope.rs b/src/types/scope.rs index 00c540cf..6e9e39ed 100644 --- a/src/types/scope.rs +++ b/src/types/scope.rs @@ -587,6 +587,16 @@ impl<'a> Scope<'a> { .zip(self.values.iter()) .map(|((name, _), value)| (name.as_ref(), value.is_read_only(), value)) } + /// Remove a range of entries within the [`Scope`]. + /// + /// # Panics + /// + /// Panics if the range is out of bounds. + #[inline] + pub(crate) fn remove_range(&mut self, start: usize, len: usize) { + self.values.drain(start..start + len).for_each(|_| {}); + self.names.drain(start..start + len).for_each(|_| {}); + } } impl<'a, K: Into>> Extend<(K, Dynamic)> for Scope<'a> { diff --git a/tests/call_fn.rs b/tests/call_fn.rs index f57b61bc..82d134c1 100644 --- a/tests/call_fn.rs +++ b/tests/call_fn.rs @@ -22,9 +22,9 @@ fn test_call_fn() -> Result<(), Box> { fn hello() { 41 + foo } - fn define_var() { + fn define_var(scale) { let bar = 21; - bar * 2 + bar * scale } ", )?; @@ -38,11 +38,6 @@ fn test_call_fn() -> Result<(), Box> { let r: INT = engine.call_fn(&mut scope, &ast, "hello", ())?; assert_eq!(r, 42); - let r: INT = engine.call_fn(&mut scope, &ast, "define_var", ())?; - assert_eq!(r, 42); - - assert!(!scope.contains("bar")); - assert_eq!( scope .get_value::("foo") @@ -50,6 +45,27 @@ fn test_call_fn() -> Result<(), Box> { 1 ); + let r: INT = engine.call_fn(&mut scope, &ast, "define_var", (2 as INT,))?; + assert_eq!(r, 42); + + assert!(!scope.contains("bar")); + + let args = [(2 as INT).into()]; + let r = engine + .call_fn_raw(&mut scope, &ast, false, false, "define_var", None, args)? + .as_int() + .unwrap(); + assert_eq!(r, 42); + + assert_eq!( + scope + .get_value::("bar") + .expect("variable bar should exist"), + 21 + ); + + assert!(!scope.contains("scale")); + Ok(()) }