Fix bug when passing shared string variable to &str parameter.

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
 ----------------

src/dynamic.rs

@@ -284,11 +284,13 @@ impl Dynamic {
     /// Always [`false`] under the `no_closure` feature.
     #[inline(always)]
     pub fn is_shared(&self) -> bool {
-        match self.0 {
         #[cfg(not(feature = "no_closure"))]
+        match self.0 {
             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"))]

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,14 +1135,16 @@ 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 =
+                let fn_name = self
-                    self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?;
+                    .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?
-                let fn_name = fn_name.take_immutable_string().map_err(|err| {
+                    .take_immutable_string()
+                    .map_err(|err| {
                         self.make_type_mismatch_err::<ImmutableString>(err, args_expr[0].position())
                     })?;
-                let num_params =
+                let num_params = self
-                    self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[1], level)?;
+                    .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[1], level)?
-                let num_params = num_params.as_int().map_err(|err| {
+                    .as_int()
+                    .map_err(|err| {
                         self.make_type_mismatch_err::<INT>(err, args_expr[0].position())
                     })?;
 
@@ -1151,9 +1159,10 @@ impl Engine {
 
             // Handle is_def_var()
             KEYWORD_IS_DEF_VAR if args_expr.len() == 1 => {
-                let var_name =
+                let var_name = self
-                    self.eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?;
+                    .eval_expr(scope, mods, state, lib, this_ptr, &args_expr[0], level)?
-                let var_name = var_name.take_immutable_string().map_err(|err| {
+                    .take_immutable_string()
+                    .map_err(|err| {
                         self.make_type_mismatch_err::<ImmutableString>(err, args_expr[0].position())
                     })?;
                 return Ok(scope.contains(&var_name).into());
@@ -1166,9 +1175,10 @@ impl Engine {
 
                 // eval - only in function call style
                 let prev_len = scope.len();
-                let script =
+                let script = self
-                    self.eval_expr(scope, mods, state, lib, this_ptr, script_expr, level)?;
+                    .eval_expr(scope, mods, state, lib, this_ptr, script_expr, level)?
-                let script = script.take_immutable_string().map_err(|typ| {
+                    .take_immutable_string()
+                    .map_err(|typ| {
                         self.make_type_mismatch_err::<ImmutableString>(typ, script_pos)
                     })?;
                 let result = self.eval_script_expr_in_place(

src/fn_register.rs

@@ -100,13 +100,20 @@ pub fn by_ref<T: Variant + Clone>(data: &mut Dynamic) -> DynamicWriteLock<T> {
 #[inline(always)]
 pub fn by_value<T: Variant + Clone>(data: &mut Dynamic) -> T {
     if TypeId::of::<T>() == TypeId::of::<&str>() {
-        // If T is &str, data must be ImmutableString, so map directly to it
+        // 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) };
+        // Beware - `Dynamic::as_str_ref` panics if `data` is shared,
-        ref_T.clone()
+        //          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::<T>() == TypeId::of::<String>() {
-        // If T is String, data must be ImmutableString, so map directly to it
+        // If T is `String`, data must be `ImmutableString`, so map directly to it
-        *unsafe_cast_box(Box::new(data.clone().take_string().unwrap())).unwrap()
+        *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.

tests/closures.rs

@@ -179,6 +179,51 @@ fn test_closures() -> Result<(), Box<EvalAltResult>> {
     Ok(())
 }
 
+#[test]
+#[cfg(not(feature = "no_closure"))]
+fn test_closures_sharing() -> Result<(), Box<EvalAltResult>> {
+    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::<INT>(
+            r#"
+                let s = "hello";
+                let f = || s;
+                foo(1, s)
+            "#
+        )?,
+        6
+    );
+
+    assert_eq!(
+        engine.eval::<String>(
+            r#"
+                let s = "hello";
+                let f = || s;
+                let n = foo(1, s);
+                s
+            "#
+        )?,
+        "hello"
+    );
+
+    assert_eq!(
+        engine.eval::<INT>(
+            r#"
+                let s = "hello";
+                let f = || s;
+                bar(1, s)
+            "#
+        )?,
+        6
+    );
+
+    Ok(())
+}
+
 #[test]
 #[cfg(not(feature = "no_closure"))]
 #[cfg(not(feature = "no_object"))]