Reduce unnecessary data size checking.

src/engine.rs

@@ -361,6 +361,7 @@ impl<'a> Target<'a> {
         }
     }
     /// Get the source [`Dynamic`] of the [`Target`].
+    #[allow(dead_code)]
     #[inline]
     #[must_use]
     pub fn source(&self) -> &Dynamic {
@@ -1459,9 +1460,6 @@ impl Engine {
                             }
                         }
 
-                        #[cfg(not(feature = "unchecked"))]
-                        self.check_data_size(target.as_ref(), root.1)?;
-
                         Ok(result)
                     }
                     // xxx[rhs] op= new_val
@@ -1478,6 +1476,8 @@ impl Engine {
                                     global, state, lib, op_info, op_pos, obj_ptr, root, new_val,
                                 )
                                 .map_err(|err| err.fill_position(new_pos))?;
+                                #[cfg(not(feature = "unchecked"))]
+                                self.check_data_size(obj_ptr, new_pos)?;
                                 None
                             }
                             // Can't index - try to call an index setter
@@ -1501,9 +1501,6 @@ impl Engine {
                             )?;
                         }
 
-                        #[cfg(not(feature = "unchecked"))]
-                        self.check_data_size(target.as_ref(), root.1)?;
-
                         Ok((Dynamic::UNIT, true))
                     }
                     // xxx[rhs]
@@ -1549,7 +1546,7 @@ impl Engine {
                             .map_err(|err| err.fill_position(new_pos))?;
                         }
                         #[cfg(not(feature = "unchecked"))]
-                        self.check_data_size(target.as_ref(), root.1)?;
+                        self.check_data_size(target.source(), new_pos)?;
                         Ok((Dynamic::UNIT, true))
                     }
                     // {xxx:map}.id
@@ -1605,9 +1602,6 @@ impl Engine {
                             )
                             .map_err(|err| err.fill_position(new_pos))?;
 
-                            #[cfg(not(feature = "unchecked"))]
-                            self.check_data_size(target.as_ref(), root.1)?;
-
                             new_val = orig_val;
                         }
 
@@ -1799,9 +1793,6 @@ impl Engine {
                                             _ => Err(err),
                                         },
                                     )?;
-
-                                    #[cfg(not(feature = "unchecked"))]
-                                    self.check_data_size(target.as_ref(), root.1)?;
                                 }
 
                                 Ok((result, may_be_changed))
@@ -1867,7 +1858,7 @@ impl Engine {
 
         let is_assignment = new_val.is_some();
 
-        let result = match lhs {
+        match lhs {
             // id.??? or id[???]
             Expr::Variable(_, var_pos, x) => {
                 #[cfg(not(feature = "unchecked"))]
@@ -1900,12 +1891,6 @@ impl Engine {
                 .map(|(v, _)| if is_assignment { Dynamic::UNIT } else { v })
                 .map_err(|err| err.fill_position(op_pos))
             }
-        };
-
-        if is_assignment {
-            result.map(|_| Dynamic::UNIT)
-        } else {
-            self.check_return_value(result, expr.position())
         }
     }
 
@@ -2355,7 +2340,7 @@ impl Engine {
             ..
         } = expr;
 
-        let result = if let Some(namespace) = namespace.as_ref() {
+        if let Some(namespace) = namespace.as_ref() {
             // Qualified function call
             let hash = hashes.native;
 
@@ -2369,9 +2354,7 @@ impl Engine {
                 scope, global, state, lib, this_ptr, name, args, constants, *hashes, pos, *capture,
                 level,
             )
-        };
+        }
-
-        self.check_return_value(result, pos)
     }
 
     /// Evaluate an expression.
@@ -2694,7 +2677,6 @@ impl Engine {
             hash_op,
             op,
         }) = op_info
-        {
         {
             let mut lock_guard;
             let lhs_ptr_inner;
@@ -2729,15 +2711,12 @@ impl Engine {
                 }
                 err => return err.map(|_| ()),
             }
-            }
         } else {
             // Normal assignment
             *target.as_mut() = new_val;
         }
 
-        target.propagate_changed_value()?;
+        target.propagate_changed_value()
-
-        self.check_data_size(target.source(), Position::NONE)
     }
 
     /// Evaluate a statement.
@@ -2768,20 +2747,14 @@ impl Engine {
             return self.eval_fn_call_expr(scope, global, state, lib, this_ptr, x, *pos, level);
         }
 
+        // Then assignments.
+        // We shouldn't do this for too many variants because, soon or later, the added comparisons
+        // will cost more than the mis-predicted `match` branch.
+        if let Stmt::Assignment(x, op_pos) = stmt {
             #[cfg(not(feature = "unchecked"))]
             self.inc_operations(&mut global.num_operations, stmt.position())?;
 
-        match stmt {
+            return if x.0.is_variable_access(false) {
-            // No-op
-            Stmt::Noop(_) => Ok(Dynamic::UNIT),
-
-            // Expression as statement
-            Stmt::Expr(expr) => Ok(self
-                .eval_expr(scope, global, state, lib, this_ptr, expr, level)?
-                .flatten()),
-
-            // var op= rhs
-            Stmt::Assignment(x, op_pos) if x.0.is_variable_access(false) => {
                 let (lhs_expr, op_info, rhs_expr) = x.as_ref();
                 let rhs_val = self
                     .eval_expr(scope, global, state, lib, this_ptr, rhs_expr, level)?
@@ -2810,16 +2783,8 @@ impl Engine {
                 )
                 .map_err(|err| err.fill_position(rhs_expr.position()))?;
 
-                #[cfg(not(feature = "unchecked"))]
-                if op_info.is_some() {
-                    self.check_data_size(lhs_ptr.as_ref(), lhs_expr.position())?;
-                }
-
                 Ok(Dynamic::UNIT)
-            }
+            } else {
-
-            // lhs op= rhs
-            Stmt::Assignment(x, op_pos) => {
                 let (lhs_expr, op_info, rhs_expr) = x.as_ref();
                 let rhs_val = self
                     .eval_expr(scope, global, state, lib, this_ptr, rhs_expr, level)?
@@ -2850,8 +2815,21 @@ impl Engine {
                     }
                     _ => unreachable!("cannot assign to expression: {:?}", lhs_expr),
                 }
+            };
         }
 
+        #[cfg(not(feature = "unchecked"))]
+        self.inc_operations(&mut global.num_operations, stmt.position())?;
+
+        match stmt {
+            // No-op
+            Stmt::Noop(_) => Ok(Dynamic::UNIT),
+
+            // Expression as statement
+            Stmt::Expr(expr) => Ok(self
+                .eval_expr(scope, global, state, lib, this_ptr, expr, level)?
+                .flatten()),
+
             // Block scope
             Stmt::Block(statements, _) if statements.is_empty() => Ok(Dynamic::UNIT),
             Stmt::Block(statements, _) => {
@@ -3395,7 +3373,7 @@ impl Engine {
     }
 
     /// Check a result to ensure that the data size is within allowable limit.
-    fn check_return_value(&self, mut result: RhaiResult, pos: Position) -> RhaiResult {
+    pub(crate) fn check_return_value(&self, mut result: RhaiResult, pos: Position) -> RhaiResult {
         let _pos = pos;
 
         match result {
@@ -3470,7 +3448,11 @@ impl Engine {
             }
             Union::Str(ref s, _, _) => (0, 0, s.len()),
             #[cfg(not(feature = "no_closure"))]
-            Union::Shared(_, _, _) if !top => {
+            Union::Shared(_, _, _) if top => {
+                Self::calc_data_sizes(&*value.read_lock::<Dynamic>().unwrap(), true)
+            }
+            #[cfg(not(feature = "no_closure"))]
+            Union::Shared(_, _, _) => {
                 unreachable!("shared values discovered within data: {}", value)
             }
             _ => (0, 0, 0),
@@ -3536,7 +3518,7 @@ impl Engine {
 
     /// Check whether the size of a [`Dynamic`] is within limits.
     #[cfg(not(feature = "unchecked"))]
-    fn check_data_size(&self, value: &Dynamic, pos: Position) -> RhaiResultOf<()> {
+    pub(crate) fn check_data_size(&self, value: &Dynamic, pos: Position) -> RhaiResultOf<()> {
         // If no data size limits, just return
         if !self.has_data_size_limit() {
             return Ok(());

src/func/call.rs

@@ -388,7 +388,14 @@ impl Engine {
                 bk.restore_first_arg(args)
             }
 
-            let result = result.map_err(|err| err.fill_position(pos))?;
+            // Check the return value (including data sizes)
+            let result = self.check_return_value(result, pos)?;
+
+            // Check the data size of any `&mut` object, which may be changed.
+            #[cfg(not(feature = "unchecked"))]
+            if is_ref_mut && args.len() > 0 {
+                self.check_data_size(&args[0], pos)?;
+            }
 
             // See if the function match print/debug (which requires special processing)
             return Ok(match name {
@@ -1285,17 +1292,19 @@ impl Engine {
 
             Some(f) if f.is_plugin_fn() => {
                 let context = (self, fn_name, module.id(), &*global, lib, pos).into();
-                f.get_plugin_fn()
+                let result = f
+                    .get_plugin_fn()
                     .expect("plugin function")
                     .clone()
-                    .call(context, &mut args)
+                    .call(context, &mut args);
-                    .map_err(|err| err.fill_position(pos))
+                self.check_return_value(result, pos)
             }
 
             Some(f) if f.is_native() => {
                 let func = f.get_native_fn().expect("native function");
                 let context = (self, fn_name, module.id(), &*global, lib, pos).into();
-                func(context, &mut args).map_err(|err| err.fill_position(pos))
+                let result = func(context, &mut args);
+                self.check_return_value(result, pos)
             }
 
             Some(f) => unreachable!("unknown function type: {:?}", f),

src/packages/array_basic.rs

@@ -3,7 +3,6 @@
 
 use crate::engine::OP_EQUALS;
 use crate::plugin::*;
-use crate::types::dynamic::Union;
 use crate::{
     def_package, Array, Dynamic, ExclusiveRange, FnPtr, InclusiveRange, NativeCallContext,
     Position, RhaiResultOf, ERR, INT,
@@ -97,8 +96,9 @@ pub mod array_functions {
 
         #[cfg(not(feature = "unchecked"))]
         let check_sizes = match item.0 {
-            Union::Array(_, _, _) | Union::Str(_, _, _) => true,
+            crate::types::dynamic::Union::Array(_, _, _)
-            Union::Map(_, _, _) => true,
+            | crate::types::dynamic::Union::Str(_, _, _) => true,
+            crate::types::dynamic::Union::Map(_, _, _) => true,
             _ => false,
         };
         #[cfg(feature = "unchecked")]

tests/data_size.rs

@@ -30,7 +30,7 @@ fn test_max_string_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<String>(
+            .run(
                 r#"
                     let x = "hello, ";
                     let y = "world!";
@@ -44,7 +44,7 @@ fn test_max_string_size() -> Result<(), Box<EvalAltResult>> {
     #[cfg(not(feature = "no_object"))]
     assert!(matches!(
         *engine
-            .eval::<String>(
+            .run(
                 r#"
                     let x = "hello";
                     x.pad(100, '!');
@@ -90,7 +90,7 @@ fn test_max_array_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<Array>(
+            .run(
                 "
                     let x = [1,2,3,4,5,6];
                     let y = [7,8,9,10,11,12];
@@ -101,10 +101,22 @@ fn test_max_array_size() -> Result<(), Box<EvalAltResult>> {
         EvalAltResult::ErrorDataTooLarge(_, _)
     ));
 
+    assert!(matches!(
+        *engine
+            .run(
+                "
+                    let x = [ 42 ];
+                    loop { x[0] = x; }
+                "
+            )
+            .expect_err("should error"),
+        EvalAltResult::ErrorDataTooLarge(_, _)
+    ));
+
     #[cfg(not(feature = "no_object"))]
     assert!(matches!(
         *engine
-            .eval::<Array>(
+            .run(
                 "
                     let x = [1,2,3,4,5,6];
                     x.pad(100, 42);
@@ -117,7 +129,7 @@ fn test_max_array_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<Array>(
+            .run(
                 "
                     let x = [1,2,3];
                     [x, x, x, x]
@@ -130,7 +142,7 @@ fn test_max_array_size() -> Result<(), Box<EvalAltResult>> {
     #[cfg(not(feature = "no_object"))]
     assert!(matches!(
         *engine
-            .eval::<Array>(
+            .run(
                 "
                     let x = #{a:1, b:2, c:3};
                     [x, x, x, x]
@@ -142,7 +154,7 @@ fn test_max_array_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<Array>(
+            .run(
                 "
                     let x = [1];
                     let y = [x, x];
@@ -220,7 +232,7 @@ fn test_max_map_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<Map>(
+            .run(
                 "
                     let x = #{a:1,b:2,c:3,d:4,e:5,f:6};
                     let y = #{g:7,h:8,i:9,j:10,k:11,l:12};
@@ -233,7 +245,7 @@ fn test_max_map_size() -> Result<(), Box<EvalAltResult>> {
 
     assert!(matches!(
         *engine
-            .eval::<Map>(
+            .run(
                 "
                     let x = #{a:1,b:2,c:3};
                     #{u:x, v:x, w:x, z:x}
@@ -246,7 +258,7 @@ fn test_max_map_size() -> Result<(), Box<EvalAltResult>> {
     #[cfg(not(feature = "no_index"))]
     assert!(matches!(
         *engine
-            .eval::<Map>(
+            .run(
                 "
                     let x = [1, 2, 3];
                     #{u:x, v:x, w:x, z:x}