diff --git a/CHANGELOG.md b/CHANGELOG.md index b77d3dea..6523076d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,12 @@ Version 0.19.14 Bug fixes --------- +* Panic when passing a shared string into a registered function as `&str` argument is fixed. +* Panic when calling `switch` statements on custom types is fixed. +* Potential overflow panics in `range(from, to, step)` is fixed. +* Some expressions involving shared variables now work properly, for example `x in shared_value`, `return shared_value`, `obj.field = shared_value` etc. Previously, the resultant value is still shared which is counter-intuitive. * Errors in native Rust functions now contain the correct function call positions. * Fixed error types in `EvalAltResult::ErrorMismatchDataType` which were swapped. -* Some expressions involving shared variables now work properly, for example `x in shared_value`, `return shared_value`, `obj.field = shared_value` etc. Previously, the resultant value is still shared which is counter-intuitive. -* Potential overflow panics in `range(from, to, step)` is fixed. -* `switch` statements no longer panic on custom types. Breaking changes ---------------- diff --git a/src/dynamic.rs b/src/dynamic.rs index 020ed7e8..ae7ecd9a 100644 --- a/src/dynamic.rs +++ b/src/dynamic.rs @@ -284,11 +284,13 @@ impl Dynamic { /// Always [`false`] under the `no_closure` feature. #[inline(always)] pub fn is_shared(&self) -> bool { + #[cfg(not(feature = "no_closure"))] match self.0 { - #[cfg(not(feature = "no_closure"))] Union::Shared(_, _) => true, _ => false, } + #[cfg(feature = "no_closure")] + false } /// Is the value held by this [`Dynamic`] a particular type? /// @@ -1533,7 +1535,7 @@ impl Dynamic { /// /// Panics if the value is shared. #[inline(always)] - pub(crate) fn as_str(&self) -> Result<&str, &'static str> { + pub(crate) fn as_str_ref(&self) -> Result<&str, &'static str> { match &self.0 { Union::Str(s, _) => Ok(s), #[cfg(not(feature = "no_closure"))] diff --git a/src/fn_call.rs b/src/fn_call.rs index 2250b5df..585a8273 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -334,6 +334,12 @@ impl Engine { backup.as_mut().unwrap().change_first_arg_to_copy(args); } + // Flatten arguments passed by value + args.iter_mut() + .skip(if is_ref { 1 } else { 0 }) + .filter(|v| v.is_shared()) + .for_each(|v| **v = mem::take(*v).flatten()); + // Run external function let source = src.as_ref().or_else(|| source.as_ref()).map(|s| s.as_str()); let result = if func.is_plugin_fn() { @@ -1129,16 +1135,18 @@ impl Engine { // Handle is_def_fn() #[cfg(not(feature = "no_function"))] crate::engine::KEYWORD_IS_DEF_FN if args_expr.len() == 2 => { - let fn_name = - self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?; - let fn_name = fn_name.take_immutable_string().map_err(|err| { - self.make_type_mismatch_err::(err, args_expr[0].position()) - })?; - let num_params = - self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[1], level)?; - let num_params = num_params.as_int().map_err(|err| { - self.make_type_mismatch_err::(err, args_expr[0].position()) - })?; + let fn_name = self + .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)? + .take_immutable_string() + .map_err(|err| { + self.make_type_mismatch_err::(err, args_expr[0].position()) + })?; + let num_params = self + .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[1], level)? + .as_int() + .map_err(|err| { + self.make_type_mismatch_err::(err, args_expr[0].position()) + })?; return Ok(if num_params < 0 { Dynamic::FALSE @@ -1151,11 +1159,12 @@ impl Engine { // Handle is_def_var() KEYWORD_IS_DEF_VAR if args_expr.len() == 1 => { - let var_name = - self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?; - let var_name = var_name.take_immutable_string().map_err(|err| { - self.make_type_mismatch_err::(err, args_expr[0].position()) - })?; + let var_name = self + .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)? + .take_immutable_string() + .map_err(|err| { + self.make_type_mismatch_err::(err, args_expr[0].position()) + })?; return Ok(scope.contains(&var_name).into()); } @@ -1166,11 +1175,12 @@ impl Engine { // eval - only in function call style let prev_len = scope.len(); - let script = - self.eval_expr(scope, mods, state, lib, this_ptr, script_expr, level)?; - let script = script.take_immutable_string().map_err(|typ| { - self.make_type_mismatch_err::(typ, script_pos) - })?; + let script = self + .eval_expr(scope, mods, state, lib, this_ptr, script_expr, level)? + .take_immutable_string() + .map_err(|typ| { + self.make_type_mismatch_err::(typ, script_pos) + })?; let result = self.eval_script_expr_in_place( scope, mods, diff --git a/src/fn_register.rs b/src/fn_register.rs index 48ed4bab..74ca32be 100644 --- a/src/fn_register.rs +++ b/src/fn_register.rs @@ -100,13 +100,20 @@ pub fn by_ref(data: &mut Dynamic) -> DynamicWriteLock { #[inline(always)] pub fn by_value(data: &mut Dynamic) -> T { if TypeId::of::() == TypeId::of::<&str>() { - // If T is &str, data must be ImmutableString, so map directly to it - let ref_str = data.as_str().unwrap(); - let ref_T = unsafe { mem::transmute::<_, &T>(&ref_str) }; - ref_T.clone() + // If T is `&str`, data must be `ImmutableString`, so map directly to it + + // Beware - `Dynamic::as_str_ref` panics if `data` is shared, + // but this should not happen for argument which is passed by value + assert!(!data.is_shared()); + + let ref_str = data + .as_str_ref() + .expect("argument passed by value should not be shared"); + let ref_t = unsafe { mem::transmute::<_, &T>(&ref_str) }; + ref_t.clone() } else if TypeId::of::() == TypeId::of::() { - // If T is String, data must be ImmutableString, so map directly to it - *unsafe_cast_box(Box::new(data.clone().take_string().unwrap())).unwrap() + // If T is `String`, data must be `ImmutableString`, so map directly to it + *unsafe_cast_box(Box::new(mem::take(data).take_string().unwrap())).unwrap() } else { // We consume the argument and then replace it with () - the argument is not supposed to be used again. // This way, we avoid having to clone the argument again, because it is already a clone when passed here. diff --git a/tests/closures.rs b/tests/closures.rs index d64cccd7..25f81c8a 100644 --- a/tests/closures.rs +++ b/tests/closures.rs @@ -179,6 +179,51 @@ fn test_closures() -> Result<(), Box> { Ok(()) } +#[test] +#[cfg(not(feature = "no_closure"))] +fn test_closures_sharing() -> Result<(), Box> { + let mut engine = Engine::new(); + + engine.register_fn("foo", |x: INT, s: &str| s.len() as INT + x); + engine.register_fn("bar", |x: INT, s: String| s.len() as INT + x); + + assert_eq!( + engine.eval::( + r#" + let s = "hello"; + let f = || s; + foo(1, s) + "# + )?, + 6 + ); + + assert_eq!( + engine.eval::( + r#" + let s = "hello"; + let f = || s; + let n = foo(1, s); + s + "# + )?, + "hello" + ); + + assert_eq!( + engine.eval::( + r#" + let s = "hello"; + let f = || s; + bar(1, s) + "# + )?, + 6 + ); + + Ok(()) +} + #[test] #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "no_object"))]