From cabceb74982cf4ed332bd70698cc65797d8f82d7 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Wed, 13 May 2020 21:58:38 +0800 Subject: [PATCH] Better handling of errors during function calls. --- src/api.rs | 3 +- src/engine.rs | 45 ++++++++++++++++++++++++------ src/fn_native.rs | 14 ++++------ src/fn_register.rs | 16 ++++------- src/module.rs | 56 ++++++++++++++------------------------ src/optimize.rs | 3 +- src/packages/math_basic.rs | 1 - src/parser.rs | 2 +- src/result.rs | 25 +++++++++++++---- tests/functions.rs | 2 +- tests/method_call.rs | 2 +- tests/stack.rs | 3 +- 12 files changed, 97 insertions(+), 75 deletions(-) diff --git a/src/api.rs b/src/api.rs index 0af6954f..24b28474 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1017,7 +1017,8 @@ impl Engine { let state = State::new(fn_lib); - let result = self.call_script_fn(Some(scope), &state, fn_def, args.as_mut(), pos, 0)?; + let result = + self.call_script_fn(Some(scope), &state, name, fn_def, args.as_mut(), pos, 0)?; let return_type = self.map_type_name(result.type_name()); diff --git a/src/engine.rs b/src/engine.rs index eda78ae0..d63787d4 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -543,7 +543,7 @@ impl Engine { if hashes.1 > 0 { if let Some(fn_def) = state.get_function(hashes.1) { return self - .call_script_fn(scope, state, fn_def, args, pos, level) + .call_script_fn(scope, state, fn_name, fn_def, args, pos, level) .map(|v| (v, false)); } } @@ -569,7 +569,7 @@ impl Engine { }; // Run external function - let result = match func.call(args, pos) { + let result = match func.call(args) { Ok(r) => { // Restore the backup value for the first argument since it has been consumed! if restore { @@ -577,7 +577,9 @@ impl Engine { } r } - Err(err) => return Err(err), + Err(err) => { + return Err(err.new_position(pos)); + } }; // See if the function match print/debug (which requires special processing) @@ -658,6 +660,7 @@ impl Engine { &self, scope: Option<&mut Scope>, state: &State, + fn_name: &str, fn_def: &FnDef, args: &mut FnCallArgs, pos: Position, @@ -687,7 +690,18 @@ impl Engine { .or_else(|err| match *err { // Convert return statement to return value EvalAltResult::Return(x, _) => Ok(x), - _ => Err(EvalAltResult::set_position(err, pos)), + EvalAltResult::ErrorInFunctionCall(name, err, _) => { + Err(Box::new(EvalAltResult::ErrorInFunctionCall( + format!("{} > {}", fn_name, name), + err, + pos, + ))) + } + _ => Err(Box::new(EvalAltResult::ErrorInFunctionCall( + fn_name.to_string(), + err, + pos, + ))), }); scope.rewind(scope_len); @@ -717,7 +731,18 @@ impl Engine { .or_else(|err| match *err { // Convert return statement to return value EvalAltResult::Return(x, _) => Ok(x), - _ => Err(EvalAltResult::set_position(err, pos)), + EvalAltResult::ErrorInFunctionCall(name, err, _) => { + Err(Box::new(EvalAltResult::ErrorInFunctionCall( + format!("{} > {}", fn_name, name), + err, + pos, + ))) + } + _ => Err(Box::new(EvalAltResult::ErrorInFunctionCall( + fn_name.to_string(), + err, + pos, + ))), }); } } @@ -809,7 +834,7 @@ impl Engine { // Evaluate the AST self.eval_ast_with_scope_raw(scope, &ast) - .map_err(|err| EvalAltResult::set_position(err, pos)) + .map_err(|err| err.new_position(pos)) } /// Chain-evaluate a dot/index chain. @@ -1415,7 +1440,7 @@ impl Engine { // First search in script-defined functions (can override built-in) if let Some(fn_def) = module.get_qualified_scripted_fn(*hash_fn_def) { - self.call_script_fn(None, state, fn_def, args.as_mut(), *pos, level) + self.call_script_fn(None, state, name, fn_def, args.as_mut(), *pos, level) } else { // Then search in Rust functions @@ -1428,8 +1453,10 @@ impl Engine { // 3) The final hash is the XOR of the two hashes. let hash_fn_native = *hash_fn_def ^ hash_fn_args; - match module.get_qualified_fn(name, hash_fn_native, *pos) { - Ok(func) => func.call(args.as_mut(), *pos), + match module.get_qualified_fn(name, hash_fn_native) { + Ok(func) => func + .call(args.as_mut()) + .map_err(|err| err.new_position(*pos)), Err(_) if def_val.is_some() => Ok(def_val.clone().unwrap()), Err(err) => Err(err), } diff --git a/src/fn_native.rs b/src/fn_native.rs index b56d2644..a8daac5f 100644 --- a/src/fn_native.rs +++ b/src/fn_native.rs @@ -1,16 +1,14 @@ use crate::any::Dynamic; use crate::result::EvalAltResult; -use crate::token::Position; use crate::stdlib::{boxed::Box, rc::Rc, sync::Arc}; pub type FnCallArgs<'a> = [&'a mut Dynamic]; #[cfg(feature = "sync")] -pub type FnAny = - dyn Fn(&mut FnCallArgs, Position) -> Result> + Send + Sync; +pub type FnAny = dyn Fn(&mut FnCallArgs) -> Result> + Send + Sync; #[cfg(not(feature = "sync"))] -pub type FnAny = dyn Fn(&mut FnCallArgs, Position) -> Result>; +pub type FnAny = dyn Fn(&mut FnCallArgs) -> Result>; #[cfg(feature = "sync")] pub type IteratorFn = dyn Fn(Dynamic) -> Box> + Send + Sync; @@ -85,7 +83,7 @@ pub trait NativeCallable { /// Get the ABI type of a native Rust function. fn abi(&self) -> NativeFunctionABI; /// Call a native Rust function. - fn call(&self, args: &mut FnCallArgs, pos: Position) -> Result>; + fn call(&self, args: &mut FnCallArgs) -> Result>; } /// A trait implemented by all native Rust functions that are callable by Rhai. @@ -94,7 +92,7 @@ pub trait NativeCallable: Send + Sync { /// Get the ABI type of a native Rust function. fn abi(&self) -> NativeFunctionABI; /// Call a native Rust function. - fn call(&self, args: &mut FnCallArgs, pos: Position) -> Result>; + fn call(&self, args: &mut FnCallArgs) -> Result>; } /// A type encapsulating a native Rust function callable by Rhai. @@ -104,8 +102,8 @@ impl NativeCallable for NativeFunction { fn abi(&self) -> NativeFunctionABI { self.1 } - fn call(&self, args: &mut FnCallArgs, pos: Position) -> Result> { - (self.0)(args, pos) + fn call(&self, args: &mut FnCallArgs) -> Result> { + (self.0)(args) } } diff --git a/src/fn_register.rs b/src/fn_register.rs index 80e09cd5..8b91ea42 100644 --- a/src/fn_register.rs +++ b/src/fn_register.rs @@ -7,9 +7,8 @@ use crate::engine::Engine; use crate::fn_native::{FnCallArgs, NativeFunctionABI::*}; use crate::parser::FnAccess; use crate::result::EvalAltResult; -use crate::token::Position; -use crate::stdlib::{any::TypeId, boxed::Box, iter::empty, mem, string::ToString}; +use crate::stdlib::{any::TypeId, boxed::Box, mem, string::ToString}; /// A trait to register custom functions with the `Engine`. pub trait RegisterFn { @@ -140,7 +139,7 @@ macro_rules! make_func { // ^ function parameter generic type name (A, B, C etc.) // ^ dereferencing function - Box::new(move |args: &mut FnCallArgs, pos: Position| { + Box::new(move |args: &mut FnCallArgs| { // The arguments are assumed to be of the correct number and types! #[allow(unused_variables, unused_mut)] @@ -155,23 +154,20 @@ macro_rules! make_func { let r = $fn($($par),*); // Map the result - $map(r, pos) + $map(r) }) }; } /// To Dynamic mapping function. #[inline(always)] -pub fn map_dynamic( - data: T, - _pos: Position, -) -> Result> { +pub fn map_dynamic(data: T) -> Result> { Ok(data.into_dynamic()) } /// To Dynamic mapping function. #[inline(always)] -pub fn map_identity(data: Dynamic, _pos: Position) -> Result> { +pub fn map_identity(data: Dynamic) -> Result> { Ok(data) } @@ -179,10 +175,8 @@ pub fn map_identity(data: Dynamic, _pos: Position) -> Result( data: Result>, - pos: Position, ) -> Result> { data.map(|v| v.into_dynamic()) - .map_err(|err| EvalAltResult::set_position(err, pos)) } macro_rules! def_register { diff --git a/src/module.rs b/src/module.rs index 698004c7..6e56a128 100644 --- a/src/module.rs +++ b/src/module.rs @@ -317,11 +317,7 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn() -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn() -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |_: &mut FnCallArgs, pos| { - func() - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) - }; + let f = move |_: &mut FnCallArgs| func().map(Dynamic::from); let arg_types = []; self.set_fn(name.into(), Pure, DEF_ACCESS, &arg_types, Box::new(f)) } @@ -345,11 +341,8 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(A) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(A) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { - func(mem::take(args[0]).cast::()) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) - }; + let f = + move |args: &mut FnCallArgs| func(mem::take(args[0]).cast::()).map(Dynamic::from); let arg_types = [TypeId::of::()]; self.set_fn(name.into(), Pure, DEF_ACCESS, &arg_types, Box::new(f)) } @@ -373,10 +366,8 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(&mut A) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(&mut A) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { - func(args[0].downcast_mut::().unwrap()) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) + let f = move |args: &mut FnCallArgs| { + func(args[0].downcast_mut::().unwrap()).map(Dynamic::from) }; let arg_types = [TypeId::of::()]; self.set_fn(name.into(), Method, DEF_ACCESS, &arg_types, Box::new(f)) @@ -403,13 +394,11 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(A, B) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(A, B) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { + let f = move |args: &mut FnCallArgs| { let a = mem::take(args[0]).cast::(); let b = mem::take(args[1]).cast::(); - func(a, b) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) + func(a, b).map(Dynamic::from) }; let arg_types = [TypeId::of::(), TypeId::of::()]; self.set_fn(name.into(), Pure, DEF_ACCESS, &arg_types, Box::new(f)) @@ -440,13 +429,11 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(&mut A, B) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(&mut A, B) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { + let f = move |args: &mut FnCallArgs| { let b = mem::take(args[1]).cast::(); let a = args[0].downcast_mut::().unwrap(); - func(a, b) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) + func(a, b).map(Dynamic::from) }; let arg_types = [TypeId::of::(), TypeId::of::()]; self.set_fn(name.into(), Method, DEF_ACCESS, &arg_types, Box::new(f)) @@ -479,14 +466,12 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(A, B, C) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(A, B, C) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { + let f = move |args: &mut FnCallArgs| { let a = mem::take(args[0]).cast::(); let b = mem::take(args[1]).cast::(); let c = mem::take(args[2]).cast::(); - func(a, b, c) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) + func(a, b, c).map(Dynamic::from) }; let arg_types = [TypeId::of::(), TypeId::of::(), TypeId::of::()]; self.set_fn(name.into(), Pure, DEF_ACCESS, &arg_types, Box::new(f)) @@ -520,14 +505,12 @@ impl Module { #[cfg(not(feature = "sync"))] func: impl Fn(&mut A, B, C) -> FuncReturn + 'static, #[cfg(feature = "sync")] func: impl Fn(&mut A, B, C) -> FuncReturn + Send + Sync + 'static, ) -> u64 { - let f = move |args: &mut FnCallArgs, pos| { + let f = move |args: &mut FnCallArgs| { let b = mem::take(args[1]).cast::(); let c = mem::take(args[2]).cast::(); let a = args[0].downcast_mut::().unwrap(); - func(a, b, c) - .map(Dynamic::from) - .map_err(|err| EvalAltResult::set_position(err, pos)) + func(a, b, c).map(Dynamic::from) }; let arg_types = [TypeId::of::(), TypeId::of::(), TypeId::of::()]; self.set_fn(name.into(), Method, DEF_ACCESS, &arg_types, Box::new(f)) @@ -559,12 +542,16 @@ impl Module { &mut self, name: &str, hash_fn_native: u64, - pos: Position, ) -> Result<&Box, Box> { self.all_functions .get(&hash_fn_native) .map(|f| f.as_ref()) - .ok_or_else(|| Box::new(EvalAltResult::ErrorFunctionNotFound(name.to_string(), pos))) + .ok_or_else(|| { + Box::new(EvalAltResult::ErrorFunctionNotFound( + name.to_string(), + Position::none(), + )) + }) } /// Get a modules-qualified script-defined functions. @@ -950,10 +937,9 @@ mod file { // Compile it let ast = engine .compile_file(file_path) - .map_err(|err| EvalAltResult::set_position(err, pos))?; + .map_err(|err| err.new_position(pos))?; - Module::eval_ast_as_new(scope, &ast, engine) - .map_err(|err| EvalAltResult::set_position(err, pos)) + Module::eval_ast_as_new(scope, &ast, engine).map_err(|err| err.new_position(pos)) } } } diff --git a/src/optimize.rs b/src/optimize.rs index e03ea76e..df385b74 100644 --- a/src/optimize.rs +++ b/src/optimize.rs @@ -124,8 +124,9 @@ fn call_fn( global_module .get_fn(hash) .or_else(|| packages.get_fn(hash)) - .map(|func| func.call(args, pos)) + .map(|func| func.call(args)) .transpose() + .map_err(|err| err.new_position(pos)) } /// Optimize a statement. diff --git a/src/packages/math_basic.rs b/src/packages/math_basic.rs index 0698befb..9c7313ed 100644 --- a/src/packages/math_basic.rs +++ b/src/packages/math_basic.rs @@ -1,5 +1,4 @@ use crate::def_package; -use crate::module::FuncReturn; use crate::parser::INT; use crate::result::EvalAltResult; use crate::token::Position; diff --git a/src/parser.rs b/src/parser.rs index 42f8a5a5..0fefc84c 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -516,7 +516,7 @@ impl Expr { } } - /// Get the `Position` of the expression. + /// Override the `Position` of the expression. pub(crate) fn set_position(mut self, new_pos: Position) -> Self { match &mut self { #[cfg(not(feature = "no_float"))] diff --git a/src/result.rs b/src/result.rs index 5e38f8d9..c33c810b 100644 --- a/src/result.rs +++ b/src/result.rs @@ -33,6 +33,9 @@ pub enum EvalAltResult { /// Call to an unknown function. Wrapped value is the name of the function. ErrorFunctionNotFound(String, Position), + /// An error has occurred inside a called function. + /// Wrapped values re the name of the function and the interior error. + ErrorInFunctionCall(String, Box, Position), /// Function call has incorrect number of arguments. /// Wrapped values are the name of the function, the number of parameters required /// and the actual number of arguments passed. @@ -97,6 +100,7 @@ impl EvalAltResult { Self::ErrorReadingScriptFile(_, _, _) => "Cannot read from script file", Self::ErrorParsing(p) => p.desc(), + Self::ErrorInFunctionCall(_, _, _) => "Error in called function", Self::ErrorFunctionNotFound(_, _) => "Function not found", Self::ErrorFunctionArgsMismatch(_, _, _, _) => { "Function call with wrong number of arguments" @@ -160,6 +164,10 @@ impl fmt::Display for EvalAltResult { Self::ErrorParsing(p) => write!(f, "Syntax error: {}", p), + Self::ErrorInFunctionCall(s, err, pos) => { + write!(f, "Error in call to function '{}' ({}): {}", s, pos, err) + } + Self::ErrorFunctionNotFound(s, pos) | Self::ErrorVariableNotFound(s, pos) | Self::ErrorModuleNotFound(s, pos) => write!(f, "{}: '{}' ({})", desc, s, pos), @@ -262,6 +270,7 @@ impl> From for Box { } impl EvalAltResult { + /// Get the `Position` of this error. pub fn position(&self) -> Position { match self { #[cfg(not(feature = "no_std"))] @@ -270,6 +279,7 @@ impl EvalAltResult { Self::ErrorParsing(err) => err.position(), Self::ErrorFunctionNotFound(_, pos) + | Self::ErrorInFunctionCall(_, _, pos) | Self::ErrorFunctionArgsMismatch(_, _, _, pos) | Self::ErrorBooleanArgMismatch(_, pos) | Self::ErrorCharMismatch(pos) @@ -296,16 +306,16 @@ impl EvalAltResult { } } - /// Consume the current `EvalAltResult` and return a new one - /// with the specified `Position`. - pub(crate) fn set_position(mut err: Box, new_position: Position) -> Box { - match err.as_mut() { + /// Override the `Position` of this error. + pub fn set_position(&mut self, new_position: Position) { + match self { #[cfg(not(feature = "no_std"))] Self::ErrorReadingScriptFile(_, pos, _) => *pos = new_position, Self::ErrorParsing(err) => err.1 = new_position, Self::ErrorFunctionNotFound(_, pos) + | Self::ErrorInFunctionCall(_, _, pos) | Self::ErrorFunctionArgsMismatch(_, _, _, pos) | Self::ErrorBooleanArgMismatch(_, pos) | Self::ErrorCharMismatch(pos) @@ -330,7 +340,12 @@ impl EvalAltResult { | Self::ErrorLoopBreak(_, pos) | Self::Return(_, pos) => *pos = new_position, } + } - err + /// Consume the current `EvalAltResult` and return a new one + /// with the specified `Position`. + pub(crate) fn new_position(mut self: Box, new_position: Position) -> Box { + self.set_position(new_position); + self } } diff --git a/tests/functions.rs b/tests/functions.rs index 422394c1..26d9b387 100644 --- a/tests/functions.rs +++ b/tests/functions.rs @@ -1,5 +1,5 @@ #![cfg(not(feature = "no_function"))] -use rhai::{Engine, EvalAltResult, Func, ParseErrorType, Scope, INT}; +use rhai::{Engine, EvalAltResult, INT}; #[test] fn test_functions() -> Result<(), Box> { diff --git a/tests/method_call.rs b/tests/method_call.rs index 59857102..850cf247 100644 --- a/tests/method_call.rs +++ b/tests/method_call.rs @@ -41,7 +41,7 @@ fn test_method_call() -> Result<(), Box> { #[test] fn test_method_call_style() -> Result<(), Box> { - let mut engine = Engine::new(); + let engine = Engine::new(); assert_eq!(engine.eval::("let x = -123; x.abs(); x")?, -123); diff --git a/tests/stack.rs b/tests/stack.rs index 503bcbde..1eeb4250 100644 --- a/tests/stack.rs +++ b/tests/stack.rs @@ -23,7 +23,8 @@ fn test_stack_overflow() -> Result<(), Box> { ) { Ok(_) => panic!("should be stack overflow"), Err(err) => match *err { - EvalAltResult::ErrorStackOverflow(_) => (), + EvalAltResult::ErrorInFunctionCall(name, _, _) + if name.starts_with("foo > foo > foo") => {} _ => panic!("should be stack overflow"), }, }