From 98707912e0249a17553427a758d96a598f339e32 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 15 Nov 2021 14:30:00 +0800 Subject: [PATCH] Convert for loop to iterator. --- CHANGELOG.md | 10 +--- src/api/public.rs | 5 -- src/engine.rs | 90 ++++++++++++++--------------- src/func/call.rs | 127 +++++++++++++++++++---------------------- src/optimizer.rs | 9 +-- src/serde/serialize.rs | 5 +- 6 files changed, 115 insertions(+), 131 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 603545e0..01b635c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,15 +4,11 @@ Rhai Release Notes Version 1.2.0 ============= -Breaking changes ----------------- +Breaking changes for scripts +--------------------------- * As originally intended, function calls with a bang (`!`) now operates directly on the caller's scope, allowing variables inside the scope to be mutated. - -Bug fixes ---------- - -* `Engine::XXX_with_scope` API's now properly propagate constants within the provided scope also to _functions_ in the script. +* As originally intended, `Engine::XXX_with_scope` API's now properly propagate constants within the provided scope also to _functions_ in the script. New features ------------ diff --git a/src/api/public.rs b/src/api/public.rs index 803036b1..101bcb37 100644 --- a/src/api/public.rs +++ b/src/api/public.rs @@ -2029,11 +2029,6 @@ impl Engine { 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. diff --git a/src/engine.rs b/src/engine.rs index 53d3669f..294b9bab 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1898,15 +1898,15 @@ impl Engine { let mut arg_values = StaticVec::with_capacity(args.len()); let mut first_arg_pos = Position::NONE; - for index in 0..args.len() { - let (value, pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args, constants, index, - )?; - arg_values.push(value.flatten()); - if index == 0 { - first_arg_pos = pos - } - } + args.iter().try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, pos)| { + if arg_values.is_empty() { + first_arg_pos = pos + } + arg_values.push(value.flatten()); + }) + })?; idx_values.push((arg_values, first_arg_pos).into()); } @@ -1942,15 +1942,17 @@ impl Engine { let mut arg_values = StaticVec::with_capacity(args.len()); let mut first_arg_pos = Position::NONE; - for index in 0..args.len() { - let (value, pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args, constants, index, - )?; - arg_values.push(value.flatten()); - if index == 0 { - first_arg_pos = pos; - } - } + args.iter().try_for_each(|expr| { + self.get_arg_value( + scope, mods, state, lib, this_ptr, level, expr, constants, + ) + .map(|(value, pos)| { + if arg_values.is_empty() { + first_arg_pos = pos + } + arg_values.push(value.flatten()); + }) + })?; (arg_values, first_arg_pos).into() } @@ -2225,7 +2227,7 @@ impl Engine { let mut pos = *pos; let mut result: Dynamic = self.const_empty_string().into(); - for expr in x.iter() { + x.iter().try_for_each(|expr| { let item = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?; self.eval_op_assignment( @@ -2243,8 +2245,8 @@ impl Engine { pos = expr.position(); self.check_data_size(&result) - .map_err(|err| err.fill_position(pos))?; - } + .map_err(|err| err.fill_position(pos)) + })?; assert!( result.is::(), @@ -2257,24 +2259,21 @@ impl Engine { #[cfg(not(feature = "no_index"))] Expr::Array(x, _) => { let mut arr = Array::with_capacity(x.len()); - for item in x.as_ref() { - arr.push( - self.eval_expr(scope, mods, state, lib, this_ptr, item, level)? - .flatten(), - ); - } + x.iter().try_for_each(|item| { + self.eval_expr(scope, mods, state, lib, this_ptr, item, level) + .map(|value| arr.push(value.flatten())) + })?; Ok(arr.into()) } #[cfg(not(feature = "no_object"))] Expr::Map(x, _) => { let mut map = x.1.clone(); - for (Ident { name: key, .. }, expr) in &x.0 { + x.0.iter().try_for_each(|(Ident { name: key, .. }, expr)| { let value_ref = map.get_mut(key.as_str()).expect("contains all keys"); - *value_ref = self - .eval_expr(scope, mods, state, lib, this_ptr, expr, level)? - .flatten(); - } + self.eval_expr(scope, mods, state, lib, this_ptr, expr, level) + .map(|value| *value_ref = value.flatten()) + })?; Ok(map.into()) } @@ -3119,19 +3118,20 @@ impl Engine { // Export statement #[cfg(not(feature = "no_module"))] Stmt::Export(list, _) => { - for (Ident { name, pos, .. }, Ident { name: rename, .. }) in list.as_ref() { - // Mark scope variables as public - if let Some((index, _)) = scope.get_index(name) { - scope.add_entry_alias( - index, - if rename.is_empty() { name } else { rename }.clone(), - ); - } else { - return Err( - EvalAltResult::ErrorVariableNotFound(name.to_string(), *pos).into() - ); - } - } + list.iter().try_for_each( + |(Ident { name, pos, .. }, Ident { name: rename, .. })| { + // Mark scope variables as public + if let Some((index, _)) = scope.get_index(name) { + scope.add_entry_alias( + index, + if rename.is_empty() { name } else { rename }.clone(), + ); + Ok(()) as Result<_, Box> + } else { + Err(EvalAltResult::ErrorVariableNotFound(name.to_string(), *pos).into()) + } + }, + )?; Ok(Dynamic::UNIT) } diff --git a/src/func/call.rs b/src/func/call.rs index 703bd677..84214d92 100644 --- a/src/func/call.rs +++ b/src/func/call.rs @@ -471,6 +471,8 @@ impl Engine { /// Call a script-defined function. /// + /// If `rewind_scope` is `false`, arguments are removed from the scope but new variables are not. + /// /// # WARNING /// /// Function call arguments may be _consumed_ when the function requires them to be passed by value. @@ -548,7 +550,7 @@ impl Engine { // Merge in encapsulated environment, if any let mut lib_merged = StaticVec::with_capacity(lib.len() + 1); - let (unified_lib, unified) = if let Some(ref env_lib) = fn_def.lib { + let (unified, is_unified) = if let Some(ref env_lib) = fn_def.lib { state.push_fn_resolution_cache(); lib_merged.push(env_lib.as_ref()); lib_merged.extend(lib.iter().cloned()); @@ -572,7 +574,7 @@ impl Engine { scope, mods, state, - unified_lib, + unified, this_ptr, body, true, @@ -604,10 +606,14 @@ impl Engine { // Remove all local variables if rewind_scope { scope.rewind(prev_scope_len); + } else if !args.is_empty() { + // Remove arguments only, leaving new variables in the scope + scope.remove_range(prev_scope_len, args.len()) } + mods.truncate(prev_mods_len); - if unified { + if is_unified { state.pop_fn_resolution_cache(); } @@ -1052,12 +1058,11 @@ impl Engine { lib: &[&Module], this_ptr: &mut Option<&mut Dynamic>, level: usize, - args_expr: &[Expr], + arg_expr: &Expr, constants: &[Dynamic], - index: usize, ) -> Result<(Dynamic, Position), Box> { - match args_expr[index] { - Expr::Stack(slot, pos) => Ok((constants[slot].clone(), pos)), + match arg_expr { + Expr::Stack(slot, pos) => Ok((constants[*slot].clone(), *pos)), ref arg => self .eval_expr(scope, mods, state, lib, this_ptr, arg, level) .map(|v| (v, arg.position())), @@ -1075,23 +1080,23 @@ impl Engine { fn_name: &str, args_expr: &[Expr], constants: &[Dynamic], - mut hashes: FnCallHashes, + hashes: FnCallHashes, pos: Position, capture_scope: bool, level: usize, ) -> RhaiResult { - // Handle call() - Redirect function call - let redirected; - let mut args_expr = args_expr; - let mut total_args = args_expr.len(); + let mut a_expr = args_expr; + let mut total_args = a_expr.len(); let mut curry = StaticVec::new(); let mut name = fn_name; + let mut hashes = hashes; + let redirected; // Handle call() - Redirect function call match name { // Handle call() KEYWORD_FN_PTR_CALL if total_args >= 1 => { let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; if !arg.is::() { @@ -1109,7 +1114,7 @@ impl Engine { name = &redirected; // Skip the first argument - args_expr = &args_expr[1..]; + a_expr = &a_expr[1..]; total_args -= 1; // Recalculate hash @@ -1123,7 +1128,7 @@ impl Engine { // Handle Fn() KEYWORD_FN_PTR if total_args == 1 => { let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; // Fn - only in function call style @@ -1138,7 +1143,7 @@ impl Engine { // Handle curry() KEYWORD_FN_PTR_CURRY if total_args > 1 => { let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; if !arg.is::() { @@ -1151,12 +1156,10 @@ impl Engine { let (name, mut fn_curry) = arg.cast::().take_data(); // Append the new curried arguments to the existing list. - for index in 1..args_expr.len() { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, index, - )?; - fn_curry.push(value); - } + a_expr.iter().skip(1).try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| fn_curry.push(value)) + })?; return Ok(FnPtr::new_unchecked(name, fn_curry).into()); } @@ -1165,7 +1168,7 @@ impl Engine { #[cfg(not(feature = "no_closure"))] crate::engine::KEYWORD_IS_SHARED if total_args == 1 => { let (arg, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; return Ok(arg.is_shared().into()); } @@ -1174,7 +1177,7 @@ impl Engine { #[cfg(not(feature = "no_function"))] crate::engine::KEYWORD_IS_DEF_FN if total_args == 2 => { let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; let fn_name = arg @@ -1182,7 +1185,7 @@ impl Engine { .map_err(|typ| self.make_type_mismatch_err::(typ, arg_pos))?; let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 1, + scope, mods, state, lib, this_ptr, level, &a_expr[1], constants, )?; let num_params = arg @@ -1201,7 +1204,7 @@ impl Engine { // Handle is_def_var() KEYWORD_IS_DEF_VAR if total_args == 1 => { let (arg, arg_pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; let var_name = arg .into_immutable_string() @@ -1214,7 +1217,7 @@ impl Engine { // eval - only in function call style let prev_len = scope.len(); let (value, pos) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, 0, + scope, mods, state, lib, this_ptr, level, &a_expr[0], constants, )?; let script = &value .into_immutable_string() @@ -1246,19 +1249,19 @@ impl Engine { } // Normal function call - except for Fn, curry, call and eval (handled above) - let mut arg_values = StaticVec::with_capacity(args_expr.len()); - let mut args = StaticVec::with_capacity(args_expr.len() + curry.len()); + let mut arg_values = StaticVec::with_capacity(a_expr.len()); + let mut args = StaticVec::with_capacity(a_expr.len() + curry.len()); let mut is_ref_mut = false; // Capture parent scope? + // + // If so, do it separately because we cannot convert the first argument (if it is a simple + // variable access) to &mut because `scope` is needed. if capture_scope && !scope.is_empty() { - // func(..., ...) - for index in 0..args_expr.len() { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, index, - )?; - arg_values.push(value.flatten()); - } + a_expr.iter().try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| arg_values.push(value.flatten())) + })?; args.extend(curry.iter_mut()); args.extend(arg_values.iter_mut()); @@ -1273,22 +1276,20 @@ impl Engine { } // Call with blank scope - if args_expr.is_empty() && curry.is_empty() { + if a_expr.is_empty() && curry.is_empty() { // No arguments } else { // If the first argument is a variable, and there is no curried arguments, // convert to method-call style in order to leverage potential &mut first argument and // avoid cloning the value - if curry.is_empty() && !args_expr.is_empty() && args_expr[0].is_variable_access(false) { + if curry.is_empty() && !a_expr.is_empty() && a_expr[0].is_variable_access(false) { // func(x, ...) -> x.func(...) - let (first_expr, rest_expr) = args_expr.split_first().expect("not empty"); + let (first_expr, rest_expr) = a_expr.split_first().expect("not empty"); - for index in 0..rest_expr.len() { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, rest_expr, constants, index, - )?; - arg_values.push(value.flatten()); - } + rest_expr.iter().try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| arg_values.push(value.flatten())) + })?; let (mut target, _pos) = self.search_namespace(scope, mods, state, lib, this_ptr, first_expr)?; @@ -1317,12 +1318,10 @@ impl Engine { } } else { // func(..., ...) - for index in 0..args_expr.len() { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, index, - )?; - arg_values.push(value.flatten()); - } + a_expr.iter().try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| arg_values.push(value.flatten())) + })?; args.extend(curry.iter_mut()); args.extend(arg_values.iter_mut()); } @@ -1362,16 +1361,12 @@ impl Engine { // &mut first argument and avoid cloning the value if !args_expr.is_empty() && args_expr[0].is_variable_access(true) { // func(x, ...) -> x.func(...) - for index in 0..args_expr.len() { - if index == 0 { - arg_values.push(Dynamic::UNIT); - } else { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, index, - )?; - arg_values.push(value.flatten()); - } - } + arg_values.push(Dynamic::UNIT); + + args_expr.iter().skip(1).try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| arg_values.push(value.flatten())) + })?; // Get target reference to first argument let (target, _pos) = @@ -1398,12 +1393,10 @@ impl Engine { } } else { // func(..., ...) or func(mod::x, ...) - for index in 0..args_expr.len() { - let (value, _) = self.get_arg_value( - scope, mods, state, lib, this_ptr, level, args_expr, constants, index, - )?; - arg_values.push(value.flatten()); - } + args_expr.iter().try_for_each(|expr| { + self.get_arg_value(scope, mods, state, lib, this_ptr, level, expr, constants) + .map(|(value, _)| arg_values.push(value.flatten())) + })?; args.extend(arg_values.iter_mut()); } } diff --git a/src/optimizer.rs b/src/optimizer.rs index dc2ed085..1ac33788 100644 --- a/src/optimizer.rs +++ b/src/optimizer.rs @@ -999,13 +999,14 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { x.args.iter_mut().for_each(|a| optimize_expr(a, state, false)); // Move constant arguments - for arg in x.args.iter_mut() { + let constants = &mut x.constants; + x.args.iter_mut().for_each(|arg| { if let Some(value) = arg.get_literal_value() { state.set_dirty(); - x.constants.push(value); - *arg = Expr::Stack(x.constants.len()-1, arg.position()); + constants.push(value); + *arg = Expr::Stack(constants.len()-1, arg.position()); } - } + }); } // Eagerly call functions diff --git a/src/serde/serialize.rs b/src/serde/serialize.rs index d2bd511e..f8eeffd5 100644 --- a/src/serde/serialize.rs +++ b/src/serde/serialize.rs @@ -60,9 +60,8 @@ impl Serialize for Dynamic { #[cfg(not(feature = "no_object"))] Union::Map(ref m, _, _) => { let mut map = ser.serialize_map(Some(m.len()))?; - for (k, v) in m.iter() { - map.serialize_entry(k.as_str(), v)?; - } + m.iter() + .try_for_each(|(k, v)| map.serialize_entry(k.as_str(), v))?; map.end() } Union::FnPtr(ref f, _, _) => ser.serialize_str(f.fn_name()),