Better error messages.

RELEASES.md

@@ -11,6 +11,7 @@ Breaking changes
 * `Module::iter_script_fn_info` is removed and merged into `Module::iter_script_fn`.
 * The `merge_namespaces` parameter to `Module::eval_ast_as_new` is removed and now defaults to `true`.
 * `GlobalFileModuleResolver` is removed because its performance gain over the `FileModuleResolver` is no longer very significant.
+* `EvalAltResult::ErrorCharMismatch` is renamed to `EvalAltResult::ErrorMismatchDataType`.
 
 New features
 ------------

src/engine.rs

@@ -229,24 +229,32 @@ impl Target<'_> {
         }
     }
     /// Update the value of the `Target`.
-    /// Position in `EvalAltResult` is `None` and must be set afterwards.
+    pub fn set_value(
-    pub fn set_value(&mut self, new_val: Dynamic) -> Result<(), Box<EvalAltResult>> {
+        &mut self,
+        new_val: Dynamic,
+        target_pos: Position,
+        new_pos: Position,
+    ) -> Result<(), Box<EvalAltResult>> {
         match self {
             Self::Ref(r) => **r = new_val,
             #[cfg(not(feature = "no_closure"))]
             #[cfg(not(feature = "no_object"))]
             Self::LockGuard((r, _)) => **r = new_val,
             Self::Value(_) => {
-                return EvalAltResult::ErrorAssignmentToUnknownLHS(Position::none()).into();
+                return EvalAltResult::ErrorAssignmentToUnknownLHS(target_pos).into();
             }
             #[cfg(not(feature = "no_index"))]
             Self::StringChar(string, index, _) if string.is::<ImmutableString>() => {
                 let mut s = string.write_lock::<ImmutableString>().unwrap();
 
                 // Replace the character at the specified index position
-                let new_ch = new_val
+                let new_ch = new_val.as_char().map_err(|err| {
-                    .as_char()
+                    Box::new(EvalAltResult::ErrorMismatchDataType(
-                    .map_err(|_| EvalAltResult::ErrorCharMismatch(Position::none()))?;
+                        err.to_string(),
+                        "char".to_string(),
+                        new_pos,
+                    ))
+                })?;
 
                 let mut chars = s.chars().collect::<StaticVec<_>>();
                 let ch = chars[*index];
@@ -413,56 +421,7 @@ impl fmt::Debug for Engine {
 
 impl Default for Engine {
     fn default() -> Self {
-        // Create the new scripting Engine
+        Self::new()
-        let mut engine = Self {
-            id: None,
-
-            packages: Default::default(),
-            global_module: Default::default(),
-
-            #[cfg(not(feature = "no_module"))]
-            #[cfg(not(feature = "no_std"))]
-            #[cfg(not(target_arch = "wasm32"))]
-            module_resolver: Some(Box::new(resolvers::FileModuleResolver::new())),
-            #[cfg(not(feature = "no_module"))]
-            #[cfg(any(feature = "no_std", target_arch = "wasm32",))]
-            module_resolver: None,
-
-            type_names: None,
-            disabled_symbols: None,
-            custom_keywords: None,
-            custom_syntax: None,
-
-            // default print/debug implementations
-            print: Box::new(default_print),
-            debug: Box::new(default_print),
-
-            // progress callback
-            progress: None,
-
-            // optimization level
-            optimization_level: if cfg!(feature = "no_optimize") {
-                OptimizationLevel::None
-            } else {
-                OptimizationLevel::Simple
-            },
-
-            #[cfg(not(feature = "unchecked"))]
-            limits: Limits {
-                max_call_stack_depth: MAX_CALL_STACK_DEPTH,
-                max_expr_depth: MAX_EXPR_DEPTH,
-                max_function_expr_depth: MAX_FUNCTION_EXPR_DEPTH,
-                max_operations: 0,
-                max_modules: usize::MAX,
-                max_string_size: 0,
-                max_array_size: 0,
-                max_map_size: 0,
-            },
-        };
-
-        engine.load_package(StandardPackage::new().get());
-
-        engine
     }
 }
 
@@ -628,7 +587,56 @@ pub fn search_scope_only<'s, 'a>(
 impl Engine {
     /// Create a new `Engine`
     pub fn new() -> Self {
-        Default::default()
+        // Create the new scripting Engine
+        let mut engine = Self {
+            id: None,
+
+            packages: Default::default(),
+            global_module: Default::default(),
+
+            #[cfg(not(feature = "no_module"))]
+            #[cfg(not(feature = "no_std"))]
+            #[cfg(not(target_arch = "wasm32"))]
+            module_resolver: Some(Box::new(resolvers::FileModuleResolver::new())),
+            #[cfg(not(feature = "no_module"))]
+            #[cfg(any(feature = "no_std", target_arch = "wasm32",))]
+            module_resolver: None,
+
+            type_names: None,
+            disabled_symbols: None,
+            custom_keywords: None,
+            custom_syntax: None,
+
+            // default print/debug implementations
+            print: Box::new(default_print),
+            debug: Box::new(default_print),
+
+            // progress callback
+            progress: None,
+
+            // optimization level
+            optimization_level: if cfg!(feature = "no_optimize") {
+                OptimizationLevel::None
+            } else {
+                OptimizationLevel::Simple
+            },
+
+            #[cfg(not(feature = "unchecked"))]
+            limits: Limits {
+                max_call_stack_depth: MAX_CALL_STACK_DEPTH,
+                max_expr_depth: MAX_EXPR_DEPTH,
+                max_function_expr_depth: MAX_FUNCTION_EXPR_DEPTH,
+                max_operations: 0,
+                max_modules: usize::MAX,
+                max_string_size: 0,
+                max_array_size: 0,
+                max_map_size: 0,
+            },
+        };
+
+        engine.load_package(StandardPackage::new().get());
+
+        engine
     }
 
     /// Create a new `Engine` with minimal built-in functions.
@@ -686,6 +694,7 @@ impl Engine {
         chain_type: ChainType,
         level: usize,
         new_val: Option<Dynamic>,
+        new_pos: Position,
     ) -> Result<(Dynamic, bool), Box<EvalAltResult>> {
         if chain_type == ChainType::None {
             panic!();
@@ -718,7 +727,7 @@ impl Engine {
 
                         self.eval_dot_index_chain_helper(
                             state, lib, this_ptr, obj_ptr, expr, idx_values, next_chain, level,
-                            new_val,
+                            new_val, new_pos,
                         )
                         .map_err(|err| err.new_position(*pos))
                     }
@@ -732,10 +741,7 @@ impl Engine {
                         {
                             // Indexed value is a reference - update directly
                             Ok(ref mut obj_ptr) => {
-                                obj_ptr
+                                obj_ptr.set_value(new_val.unwrap(), rhs.position(), new_pos)?;
-                                    .set_value(new_val.unwrap())
-                                    .map_err(|err| err.new_position(rhs.position()))?;
-
                                 None
                             }
                             Err(err) => match *err {
@@ -799,8 +805,7 @@ impl Engine {
                         let mut val = self
                             .get_indexed_mut(state, lib, target, index, *pos, true, false, level)?;
 
-                        val.set_value(new_val.unwrap())
+                        val.set_value(new_val.unwrap(), rhs.position(), new_pos)?;
-                            .map_err(|err| err.new_position(rhs.position()))?;
                         Ok((Default::default(), true))
                     }
                     // {xxx:map}.id
@@ -868,7 +873,7 @@ impl Engine {
 
                         self.eval_dot_index_chain_helper(
                             state, lib, this_ptr, &mut val, expr, idx_values, next_chain, level,
-                            new_val,
+                            new_val, new_pos,
                         )
                         .map_err(|err| err.new_position(*pos))
                     }
@@ -902,6 +907,7 @@ impl Engine {
                                         next_chain,
                                         level,
                                         new_val,
+                                        new_pos,
                                     )
                                     .map_err(|err| err.new_position(*pos))?;
 
@@ -941,7 +947,7 @@ impl Engine {
 
                                 self.eval_dot_index_chain_helper(
                                     state, lib, this_ptr, target, expr, idx_values, next_chain,
-                                    level, new_val,
+                                    level, new_val, new_pos,
                                 )
                                 .map_err(|err| err.new_position(*pos))
                             }
@@ -972,6 +978,7 @@ impl Engine {
         expr: &Expr,
         level: usize,
         new_val: Option<Dynamic>,
+        new_pos: Position,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
         let ((dot_lhs, dot_rhs, op_pos), chain_type) = match expr {
             Expr::Index(x) => (x.as_ref(), ChainType::Index),
@@ -1007,7 +1014,8 @@ impl Engine {
 
                 let obj_ptr = &mut target.into();
                 self.eval_dot_index_chain_helper(
-                    state, lib, &mut None, obj_ptr, dot_rhs, idx_values, chain_type, level, new_val,
+                    state, lib, &mut None, obj_ptr, dot_rhs, idx_values, chain_type, level,
+                    new_val, new_pos,
                 )
                 .map(|(v, _)| v)
                 .map_err(|err| err.new_position(*op_pos))
@@ -1022,6 +1030,7 @@ impl Engine {
                 let obj_ptr = &mut val.into();
                 self.eval_dot_index_chain_helper(
                     state, lib, this_ptr, obj_ptr, dot_rhs, idx_values, chain_type, level, new_val,
+                    new_pos,
                 )
                 .map(|(v, _)| v)
                 .map_err(|err| err.new_position(*op_pos))
@@ -1409,9 +1418,9 @@ impl Engine {
                 let mut rhs_val =
                     self.eval_expr(scope, mods, state, lib, this_ptr, rhs_expr, level)?;
 
-                let _new_val = Some(if op.is_empty() {
+                let (_new_val, _new_pos) = if op.is_empty() {
                     // Normal assignment
-                    rhs_val
+                    (Some(rhs_val), rhs_expr.position())
                 } else {
                     // Op-assignment - always map to `lhs = lhs op rhs`
                     let op = &op[..op.len() - 1]; // extract operator without =
@@ -1419,12 +1428,16 @@ impl Engine {
                         &mut self.eval_expr(scope, mods, state, lib, this_ptr, lhs_expr, level)?,
                         &mut rhs_val,
                     ];
-                    self.exec_fn_call(
+
+                    let result = self
+                        .exec_fn_call(
                             state, lib, op, 0, args, false, false, false, None, &None, level,
                         )
                         .map(|(v, _)| v)
-                    .map_err(|err| err.new_position(*op_pos))?
+                        .map_err(|err| err.new_position(*op_pos))?;
-                });
+
+                    (Some(result), rhs_expr.position())
+                };
 
                 match lhs_expr {
                     // name op= rhs
@@ -1433,7 +1446,7 @@ impl Engine {
                     #[cfg(not(feature = "no_index"))]
                     Expr::Index(_) => {
                         self.eval_dot_index_chain(
-                            scope, mods, state, lib, this_ptr, lhs_expr, level, _new_val,
+                            scope, mods, state, lib, this_ptr, lhs_expr, level, _new_val, _new_pos,
                         )?;
                         Ok(Default::default())
                     }
@@ -1441,7 +1454,7 @@ impl Engine {
                     #[cfg(not(feature = "no_object"))]
                     Expr::Dot(_) => {
                         self.eval_dot_index_chain(
-                            scope, mods, state, lib, this_ptr, lhs_expr, level, _new_val,
+                            scope, mods, state, lib, this_ptr, lhs_expr, level, _new_val, _new_pos,
                         )?;
                         Ok(Default::default())
                     }
@@ -1458,15 +1471,31 @@ impl Engine {
 
             // lhs[idx_expr]
             #[cfg(not(feature = "no_index"))]
-            Expr::Index(_) => {
+            Expr::Index(_) => self.eval_dot_index_chain(
-                self.eval_dot_index_chain(scope, mods, state, lib, this_ptr, expr, level, None)
+                scope,
-            }
+                mods,
+                state,
+                lib,
+                this_ptr,
+                expr,
+                level,
+                None,
+                Position::none(),
+            ),
 
             // lhs.dot_rhs
             #[cfg(not(feature = "no_object"))]
-            Expr::Dot(_) => {
+            Expr::Dot(_) => self.eval_dot_index_chain(
-                self.eval_dot_index_chain(scope, mods, state, lib, this_ptr, expr, level, None)
+                scope,
-            }
+                mods,
+                state,
+                lib,
+                this_ptr,
+                expr,
+                level,
+                None,
+                Position::none(),
+            ),
 
             #[cfg(not(feature = "no_index"))]
             Expr::Array(x) => Ok(Dynamic(Union::Array(Box::new(

src/fn_call.rs

@@ -614,7 +614,7 @@ impl Engine {
         mods: &mut Imports,
         state: &mut State,
         lib: &Module,
-        script_expr: &Dynamic,
+        script: &str,
         _level: usize,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
         self.inc_operations(state)?;
@@ -628,14 +628,6 @@ impl Engine {
             ));
         }
 
-        let script = script_expr.as_str().map_err(|typ| {
-            EvalAltResult::ErrorMismatchOutputType(
-                self.map_type_name(type_name::<ImmutableString>()).into(),
-                typ.into(),
-                Position::none(),
-            )
-        })?;
-
         // Compile the script text
         // No optimizations because we only run it once
         let mut ast = self.compile_with_scope_and_optimization_level(
@@ -804,7 +796,7 @@ impl Engine {
         // Feed the changed temp value back
         if updated && !is_ref && !is_value {
             let new_val = target.as_mut().clone();
-            target.set_value(new_val)?;
+            target.set_value(new_val, Position::none(), Position::none())?;
         }
 
         Ok((result, updated))
@@ -828,6 +820,15 @@ impl Engine {
         capture: bool,
         level: usize,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
+        fn make_type_err<T>(engine: &Engine, typ: &str, pos: Position) -> Box<EvalAltResult> {
+            EvalAltResult::ErrorMismatchDataType(
+                typ.into(),
+                engine.map_type_name(type_name::<T>()).into(),
+                pos,
+            )
+            .into()
+        }
+
         // Handle Fn()
         if name == KEYWORD_FN_PTR && args_expr.len() == 1 {
             let hash_fn = calc_fn_hash(empty(), name, 1, once(TypeId::of::<ImmutableString>()));
@@ -839,14 +840,7 @@ impl Engine {
 
                 return arg_value
                     .take_immutable_string()
-                    .map_err(|typ| {
+                    .map_err(|typ| make_type_err::<ImmutableString>(self, typ, expr.position()))
-                        EvalAltResult::ErrorMismatchOutputType(
-                            self.map_type_name(type_name::<ImmutableString>()).into(),
-                            typ.into(),
-                            expr.position(),
-                        )
-                        .into()
-                    })
                     .and_then(|s| FnPtr::try_from(s))
                     .map(Into::<Dynamic>::into)
                     .map_err(|err| err.new_position(expr.position()));
@@ -856,18 +850,17 @@ impl Engine {
         // Handle curry()
         if name == KEYWORD_FN_PTR_CURRY && args_expr.len() > 1 {
             let expr = args_expr.get(0).unwrap();
-            let fn_ptr = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
+            let arg_value = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
 
-            if !fn_ptr.is::<FnPtr>() {
+            if !arg_value.is::<FnPtr>() {
-                return EvalAltResult::ErrorMismatchOutputType(
+                return Err(make_type_err::<FnPtr>(
-                    self.map_type_name(type_name::<FnPtr>()).into(),
+                    self,
-                    self.map_type_name(fn_ptr.type_name()).into(),
+                    self.map_type_name(arg_value.type_name()),
                     expr.position(),
-                )
+                ));
-                .into();
             }
 
-            let (fn_name, fn_curry) = fn_ptr.cast::<FnPtr>().take_data();
+            let (fn_name, fn_curry) = arg_value.cast::<FnPtr>().take_data();
 
             let curry: StaticVec<_> = args_expr
                 .iter()
@@ -902,10 +895,10 @@ impl Engine {
             && !self.has_override(lib, 0, hash_script, pub_only)
         {
             let expr = args_expr.get(0).unwrap();
-            let fn_name = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
+            let arg_value = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
 
-            if fn_name.is::<FnPtr>() {
+            if arg_value.is::<FnPtr>() {
-                let fn_ptr = fn_name.cast::<FnPtr>();
+                let fn_ptr = arg_value.cast::<FnPtr>();
                 curry = fn_ptr.curry().iter().cloned().collect();
                 // Redirect function name
                 redirected = fn_ptr.take_data().0;
@@ -915,12 +908,11 @@ impl Engine {
                 // Recalculate hash
                 hash_script = calc_fn_hash(empty(), name, curry.len() + args_expr.len(), empty());
             } else {
-                return EvalAltResult::ErrorMismatchOutputType(
+                return Err(make_type_err::<FnPtr>(
-                    self.map_type_name(type_name::<FnPtr>()).into(),
+                    self,
-                    fn_name.type_name().into(),
+                    self.map_type_name(arg_value.type_name()),
                     expr.position(),
-                )
+                ));
-                .into();
             }
         }
 
@@ -930,10 +922,13 @@ impl Engine {
 
             if !self.has_override(lib, hash_fn, hash_script, pub_only) {
                 let expr = args_expr.get(0).unwrap();
-                if let Ok(var_name) = self
+                let arg_value = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
-                    .eval_expr(scope, mods, state, lib, this_ptr, expr, level)?
+                let var_name = arg_value
                     .as_str()
-                {
+                    .map_err(|err| make_type_err::<ImmutableString>(self, err, expr.position()))?;
+                if var_name.is_empty() {
+                    return Ok(false.into());
+                } else {
                     return Ok(scope.contains(var_name).into());
                 }
             }
@@ -954,12 +949,21 @@ impl Engine {
                 let fn_name_expr = args_expr.get(0).unwrap();
                 let num_params_expr = args_expr.get(1).unwrap();
 
-                if let (Ok(fn_name), Ok(num_params)) = (
+                let arg0_value =
-                    self.eval_expr(scope, mods, state, lib, this_ptr, fn_name_expr, level)?
+                    self.eval_expr(scope, mods, state, lib, this_ptr, fn_name_expr, level)?;
-                        .as_str(),
+                let arg1_value =
-                    self.eval_expr(scope, mods, state, lib, this_ptr, num_params_expr, level)?
+                    self.eval_expr(scope, mods, state, lib, this_ptr, num_params_expr, level)?;
-                        .as_int(),
+
-                ) {
+                let fn_name = arg0_value.as_str().map_err(|err| {
+                    make_type_err::<ImmutableString>(self, err, fn_name_expr.position())
+                })?;
+                let num_params = arg1_value
+                    .as_int()
+                    .map_err(|err| make_type_err::<INT>(self, err, num_params_expr.position()))?;
+
+                if fn_name.is_empty() || num_params < 0 {
+                    return Ok(false.into());
+                } else {
                     let hash = calc_fn_hash(empty(), fn_name, num_params as usize, empty());
                     return Ok(lib.contains_fn(hash, false).into());
                 }
@@ -974,10 +978,16 @@ impl Engine {
                 // eval - only in function call style
                 let prev_len = scope.len();
                 let expr = args_expr.get(0).unwrap();
-                let script = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
+                let arg_value = self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?;
-                let result = self
+                let script = arg_value
-                    .eval_script_expr(scope, mods, state, lib, &script, level + 1)
+                    .as_str()
-                    .map_err(|err| err.new_position(expr.position()));
+                    .map_err(|typ| make_type_err::<ImmutableString>(self, typ, expr.position()))?;
+                let result = if !script.is_empty() {
+                    self.eval_script_expr(scope, mods, state, lib, script, level + 1)
+                        .map_err(|err| err.new_position(expr.position()))
+                } else {
+                    Ok(().into())
+                };
 
                 // IMPORTANT! If the eval defines new variables in the current scope,
                 //            all variable offsets from this point on will be mis-aligned.

src/result.rs

@@ -46,8 +46,9 @@ pub enum EvalAltResult {
     ErrorUnboundThis(Position),
     /// Non-boolean operand encountered for boolean operator. Wrapped value is the operator.
     ErrorBooleanArgMismatch(String, Position),
-    /// Non-character value encountered where a character is required.
+    /// Data is not of the required type.
-    ErrorCharMismatch(Position),
+    /// Wrapped values are the type requested and type of the actual result.
+    ErrorMismatchDataType(String, String, Position),
     /// Array access out-of-bounds.
     /// Wrapped values are the current number of elements in the array and the index number.
     ErrorArrayBounds(usize, INT, Position),
@@ -120,7 +121,7 @@ impl EvalAltResult {
             Self::ErrorFunctionNotFound(_, _) => "Function not found",
             Self::ErrorUnboundThis(_) => "'this' is not bound",
             Self::ErrorBooleanArgMismatch(_, _) => "Boolean operator expects boolean operands",
-            Self::ErrorCharMismatch(_) => "Character expected",
+            Self::ErrorMismatchDataType(_, _, _) => "Data type is incorrect",
             Self::ErrorNumericIndexExpr(_) => {
                 "Indexing into an array or string expects an integer index"
             }
@@ -215,7 +216,10 @@ impl fmt::Display for EvalAltResult {
 
             Self::ErrorAssignmentToConstant(s, _) => write!(f, "{}: '{}'", desc, s)?,
             Self::ErrorMismatchOutputType(r, s, _) => {
-                write!(f, "{} (expecting {}): {}", desc, s, r)?
+                write!(f, "Output type is incorrect: {} (expecting {})", r, s)?
+            }
+            Self::ErrorMismatchDataType(r, s, _) => {
+                write!(f, "Data type is incorrect: {} (expecting {})", r, s)?
             }
             Self::ErrorArithmetic(s, _) => f.write_str(s)?,
 
@@ -225,7 +229,6 @@ impl fmt::Display for EvalAltResult {
             Self::ErrorBooleanArgMismatch(op, _) => {
                 write!(f, "{} operator expects boolean operands", op)?
             }
-            Self::ErrorCharMismatch(_) => write!(f, "string indexing expects a character value")?,
             Self::ErrorArrayBounds(_, index, _) if *index < 0 => {
                 write!(f, "{}: {} < 0", desc, index)?
             }
@@ -291,7 +294,7 @@ impl EvalAltResult {
             | Self::ErrorInModule(_, _, pos)
             | Self::ErrorUnboundThis(pos)
             | Self::ErrorBooleanArgMismatch(_, pos)
-            | Self::ErrorCharMismatch(pos)
+            | Self::ErrorMismatchDataType(_, _, pos)
             | Self::ErrorArrayBounds(_, _, pos)
             | Self::ErrorStringBounds(_, _, pos)
             | Self::ErrorIndexingType(_, pos)
@@ -333,7 +336,7 @@ impl EvalAltResult {
             | Self::ErrorInModule(_, _, pos)
             | Self::ErrorUnboundThis(pos)
             | Self::ErrorBooleanArgMismatch(_, pos)
-            | Self::ErrorCharMismatch(pos)
+            | Self::ErrorMismatchDataType(_, _, pos)
             | Self::ErrorArrayBounds(_, _, pos)
             | Self::ErrorStringBounds(_, _, pos)
             | Self::ErrorIndexingType(_, pos)