From 057f6435a45aa1160faf0cd3c62956b7d6505f11 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 27 Jul 2020 18:10:45 +0800 Subject: [PATCH] Add public_only parameter to module function methods. --- RELEASES.md | 1 + src/engine.rs | 30 ++++++++--------- src/fn_call.rs | 80 ++++++++++++++------------------------------- src/module.rs | 58 ++++++++++++++++++++------------ src/packages/mod.rs | 9 ++--- src/result.rs | 2 +- tests/modules.rs | 2 +- tests/time.rs | 5 ++- 8 files changed, 87 insertions(+), 100 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 520a2bf1..bd4623c0 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -27,6 +27,7 @@ Breaking changes * Language keywords are now _reserved_ (even when disabled) and they can no longer be used as variable names. * Function signature for defining custom syntax is simplified. * `Engine::register_raw_fn_XXX` API shortcuts are removed. +* `PackagesCollection::get_fn`, `PackagesCollection::contains_fn`, `Module::get_fn` and `Module::contains_fn` now take an additional `public_only` parameter indicating whether only public functions are accepted. Housekeeping ------------ diff --git a/src/engine.rs b/src/engine.rs index aea01500..5eb11702 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -19,7 +19,7 @@ use crate::utils::StaticVec; use crate::any::Variant; #[cfg(not(feature = "no_function"))] -use crate::parser::{FnAccess, ScriptFnDef}; +use crate::parser::ScriptFnDef; #[cfg(not(feature = "no_module"))] use crate::module::ModuleResolver; @@ -264,19 +264,15 @@ pub fn get_script_function_by_signature<'a>( module: &'a Module, name: &str, params: usize, - public_only: bool, + pub_only: bool, ) -> Option<&'a ScriptFnDef> { // Qualifiers (none) + function name + number of arguments. let hash_script = calc_fn_hash(empty(), name, params, empty()); - let func = module.get_fn(hash_script)?; - if !func.is_script() { - return None; - } - let fn_def = func.get_fn_def(); - - match fn_def.access { - FnAccess::Private if public_only => None, - FnAccess::Private | FnAccess::Public => Some(&fn_def), + let func = module.get_fn(hash_script, pub_only)?; + if func.is_script() { + Some(func.get_fn_def()) + } else { + None } } @@ -798,7 +794,7 @@ impl Engine { let mut val = match sub_lhs { Expr::Property(p) => { - let ((prop, _, _), _) = p.as_ref(); + let ((prop, _, _), pos) = p.as_ref(); let index = prop.clone().into(); self.get_indexed_mut(state, lib, target, index, *pos, false, level)? } @@ -827,12 +823,12 @@ impl Engine { } // xxx.sub_lhs[expr] | xxx.sub_lhs.expr Expr::Index(x) | Expr::Dot(x) => { - let (sub_lhs, expr, pos) = x.as_ref(); + let (sub_lhs, expr, _) = x.as_ref(); match sub_lhs { // xxx.prop[expr] | xxx.prop.expr Expr::Property(p) => { - let ((_, getter, setter), _) = p.as_ref(); + let ((_, getter, setter), pos) = p.as_ref(); let arg_values = &mut [target.as_mut(), &mut Default::default()]; let args = &mut arg_values[..1]; let (mut val, updated) = self @@ -1304,8 +1300,8 @@ impl Engine { if let Some(CallableFunction::Method(func)) = self .global_module - .get_fn(hash_fn) - .or_else(|| self.packages.get_fn(hash_fn)) + .get_fn(hash_fn, false) + .or_else(|| self.packages.get_fn(hash_fn, false)) { // Overriding exact implementation func(self, lib, &mut [lhs_ptr, &mut rhs_val])?; @@ -1431,7 +1427,7 @@ impl Engine { let ((name, _, pos), modules, hash, args_expr, def_val) = x.as_ref(); self.make_qualified_function_call( scope, mods, state, lib, this_ptr, modules, name, args_expr, *def_val, *hash, - true, level, + level, ) .map_err(|err| err.new_position(*pos)) } diff --git a/src/fn_call.rs b/src/fn_call.rs index d581268a..6bd95393 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -8,10 +8,10 @@ use crate::engine::{ KEYWORD_TYPE_OF, }; use crate::error::ParseErrorType; -use crate::fn_native::{CallableFunction, FnCallArgs, FnPtr}; +use crate::fn_native::{FnCallArgs, FnPtr}; use crate::module::{Module, ModuleRef}; use crate::optimize::OptimizationLevel; -use crate::parser::{Expr, FnAccess, ImmutableString, AST, INT}; +use crate::parser::{Expr, ImmutableString, AST, INT}; use crate::result::EvalAltResult; use crate::scope::Scope; use crate::token::Position; @@ -105,19 +105,6 @@ fn restore_first_arg<'a>(old_this_ptr: Option<&'a mut Dynamic>, args: &mut FnCal } } -#[inline] -fn check_public_access(func: &CallableFunction) -> Option<&CallableFunction> { - if func.access() == FnAccess::Private { - None - } else { - Some(func) - } -} -#[inline(always)] -fn no_check_access(func: &CallableFunction) -> Option<&CallableFunction> { - Some(func) -} - impl Engine { /// Universal method for calling functions either registered with the `Engine` or written in Rhai. /// Position in `EvalAltResult` is `None` and must be set afterwards. @@ -146,12 +133,6 @@ impl Engine { let native_only = hash_script == 0; - let check = if pub_only { - check_public_access - } else { - no_check_access - }; - // Check for stack overflow #[cfg(not(feature = "no_function"))] #[cfg(not(feature = "unchecked"))] @@ -170,16 +151,14 @@ impl Engine { // Then search packages // NOTE: We skip script functions for global_module and packages, and native functions for lib let func = if !native_only { - lib.get_fn(hash_script).and_then(check) //.or_else(|| lib.get_fn(hash_fn)).and_then(check) + lib.get_fn(hash_script, pub_only) //.or_else(|| lib.get_fn(hash_fn, pub_only)) } else { None } - //.or_else(|| self.global_module.get_fn(hash_script)).and_then(check) - .or_else(|| self.global_module.get_fn(hash_fn)) - .and_then(check) - //.or_else(|| self.packages.get_fn(hash_script)).and_then(check) - .or_else(|| self.packages.get_fn(hash_fn)) - .and_then(check); + //.or_else(|| self.global_module.get_fn(hash_script, pub_only)) + .or_else(|| self.global_module.get_fn(hash_fn, pub_only)) + //.or_else(|| self.packages.get_fn(hash_script, pub_only)) + .or_else(|| self.packages.get_fn(hash_fn, pub_only)); if let Some(func) = func { #[cfg(not(feature = "no_function"))] @@ -274,7 +253,11 @@ impl Engine { // Getter function not found? if let Some(prop) = extract_prop_from_getter(fn_name) { return Err(Box::new(EvalAltResult::ErrorDotExpr( - format!("- property '{}' unknown or write-only", prop), + format!( + "Unknown property '{}' for {}, or it is write-only", + prop, + self.map_type_name(args[0].type_name()) + ), Position::none(), ))); } @@ -282,7 +265,11 @@ impl Engine { // Setter function not found? if let Some(prop) = extract_prop_from_setter(fn_name) { return Err(Box::new(EvalAltResult::ErrorDotExpr( - format!("- property '{}' unknown or read-only", prop), + format!( + "Unknown property '{}' for {}, or it is read-only", + prop, + self.map_type_name(args[0].type_name()) + ), Position::none(), ))); } @@ -401,27 +388,17 @@ impl Engine { // Has a system function an override? fn has_override(&self, lib: &Module, hash_fn: u64, hash_script: u64, pub_only: bool) -> bool { - let check = if pub_only { - check_public_access - } else { - no_check_access - }; - // NOTE: We skip script functions for global_module and packages, and native functions for lib // First check script-defined functions - lib.get_fn(hash_script) - .and_then(check) - //.or_else(|| lib.get_fn(hash_fn)).and_then(check) + lib.contains_fn(hash_script, pub_only) + //|| lib.contains_fn(hash_fn, pub_only) // Then check registered functions - //.or_else(|| self.global_module.get_fn(hash_script)).and_then(check) - .or_else(|| self.global_module.get_fn(hash_fn)) - .and_then(check) + //|| self.global_module.contains_fn(hash_script, pub_only) + || self.global_module.contains_fn(hash_fn, pub_only) // Then check packages - //.or_else(|| self.packages.get_fn(hash_script)).and_then(check) - .or_else(|| self.packages.get_fn(hash_fn)) - .and_then(check) - .is_some() + //|| self.packages.contains_fn(hash_script, pub_only) + || self.packages.contains_fn(hash_fn, pub_only) } /// Perform an actual function call, taking care of special functions @@ -840,7 +817,6 @@ impl Engine { args_expr: &[Expr], def_val: Option, hash_script: u64, - pub_only: bool, level: usize, ) -> Result> { let modules = modules.as_ref().unwrap(); @@ -887,13 +863,7 @@ impl Engine { let module = search_imports(mods, state, modules)?; // First search in script-defined functions (can override built-in) - let check = if pub_only { - check_public_access - } else { - no_check_access - }; - - let func = match module.get_qualified_fn(hash_script).and_then(check) { + let func = match module.get_qualified_fn(hash_script) { // Then search in Rust functions None => { self.inc_operations(state)?; @@ -907,7 +877,7 @@ impl Engine { // 3) The final hash is the XOR of the two hashes. let hash_qualified_fn = hash_script ^ hash_fn_args; - module.get_qualified_fn(hash_qualified_fn).and_then(check) + module.get_qualified_fn(hash_qualified_fn) } r => r, }; diff --git a/src/module.rs b/src/module.rs index 9fad5571..b88cbc59 100644 --- a/src/module.rs +++ b/src/module.rs @@ -355,10 +355,20 @@ impl Module { /// /// let mut module = Module::new(); /// let hash = module.set_fn_0("calc", || Ok(42_i64)); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` - pub fn contains_fn(&self, hash_fn: u64) -> bool { - self.functions.contains_key(&hash_fn) + pub fn contains_fn(&self, hash_fn: u64, public_only: bool) -> bool { + if public_only { + self.functions + .get(&hash_fn) + .map(|(_, access, _, _)| match access { + FnAccess::Public => true, + FnAccess::Private => false, + }) + .unwrap_or(false) + } else { + self.functions.contains_key(&hash_fn) + } } /// Set a Rust function into the module, returning a hash key. @@ -443,7 +453,7 @@ impl Module { /// Ok(orig) // return Result> /// }); /// - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_raw_fn( &mut self, @@ -468,7 +478,7 @@ impl Module { /// /// let mut module = Module::new(); /// let hash = module.set_fn_0("calc", || Ok(42_i64)); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_0( &mut self, @@ -491,7 +501,7 @@ impl Module { /// /// let mut module = Module::new(); /// let hash = module.set_fn_1("calc", |x: i64| Ok(x + 1)); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_1( &mut self, @@ -516,7 +526,7 @@ impl Module { /// /// let mut module = Module::new(); /// let hash = module.set_fn_1_mut("calc", |x: &mut i64| { *x += 1; Ok(*x) }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_1_mut( &mut self, @@ -541,7 +551,7 @@ impl Module { /// /// let mut module = Module::new(); /// let hash = module.set_getter_fn("value", |x: &mut i64| { Ok(*x) }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` #[cfg(not(feature = "no_object"))] pub fn set_getter_fn( @@ -565,7 +575,7 @@ impl Module { /// let hash = module.set_fn_2("calc", |x: i64, y: ImmutableString| { /// Ok(x + y.len() as i64) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_2( &mut self, @@ -596,7 +606,7 @@ impl Module { /// let hash = module.set_fn_2_mut("calc", |x: &mut i64, y: ImmutableString| { /// *x += y.len() as i64; Ok(*x) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_2_mut( &mut self, @@ -628,7 +638,7 @@ impl Module { /// *x = y.len() as i64; /// Ok(()) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` #[cfg(not(feature = "no_object"))] pub fn set_setter_fn( @@ -653,7 +663,7 @@ impl Module { /// let hash = module.set_indexer_get_fn(|x: &mut i64, y: ImmutableString| { /// Ok(*x + y.len() as i64) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` #[cfg(not(feature = "no_object"))] #[cfg(not(feature = "no_index"))] @@ -677,7 +687,7 @@ impl Module { /// let hash = module.set_fn_3("calc", |x: i64, y: ImmutableString, z: i64| { /// Ok(x + y.len() as i64 + z) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_3< A: Variant + Clone, @@ -714,7 +724,7 @@ impl Module { /// let hash = module.set_fn_3_mut("calc", |x: &mut i64, y: ImmutableString, z: i64| { /// *x += y.len() as i64 + z; Ok(*x) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_3_mut< A: Variant + Clone, @@ -752,7 +762,7 @@ impl Module { /// *x = y.len() as i64 + value; /// Ok(()) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` #[cfg(not(feature = "no_object"))] #[cfg(not(feature = "no_index"))] @@ -796,8 +806,8 @@ impl Module { /// Ok(()) /// } /// ); - /// assert!(module.contains_fn(hash_get)); - /// assert!(module.contains_fn(hash_set)); + /// assert!(module.contains_fn(hash_get, true)); + /// assert!(module.contains_fn(hash_set, true)); /// ``` #[cfg(not(feature = "no_object"))] #[cfg(not(feature = "no_index"))] @@ -825,7 +835,7 @@ impl Module { /// let hash = module.set_fn_4("calc", |x: i64, y: ImmutableString, z: i64, _w: ()| { /// Ok(x + y.len() as i64 + z) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_4< A: Variant + Clone, @@ -869,7 +879,7 @@ impl Module { /// let hash = module.set_fn_4_mut("calc", |x: &mut i64, y: ImmutableString, z: i64, _w: ()| { /// *x += y.len() as i64 + z; Ok(*x) /// }); - /// assert!(module.contains_fn(hash)); + /// assert!(module.contains_fn(hash, true)); /// ``` pub fn set_fn_4_mut< A: Variant + Clone, @@ -903,8 +913,14 @@ impl Module { /// /// The `u64` hash is calculated by the function `crate::calc_fn_hash`. /// It is also returned by the `set_fn_XXX` calls. - pub(crate) fn get_fn(&self, hash_fn: u64) -> Option<&Func> { - self.functions.get(&hash_fn).map(|(_, _, _, v)| v) + pub(crate) fn get_fn(&self, hash_fn: u64, public_only: bool) -> Option<&Func> { + self.functions + .get(&hash_fn) + .and_then(|(_, access, _, f)| match access { + _ if !public_only => Some(f), + FnAccess::Public => Some(f), + FnAccess::Private => None, + }) } /// Get a modules-qualified function. diff --git a/src/packages/mod.rs b/src/packages/mod.rs index ccd78add..1c6dc3a9 100644 --- a/src/packages/mod.rs +++ b/src/packages/mod.rs @@ -61,14 +61,15 @@ impl PackagesCollection { self.0.insert(0, package); } /// Does the specified function hash key exist in the `PackagesCollection`? - pub fn contains_fn(&self, hash: u64) -> bool { - self.0.iter().any(|p| p.contains_fn(hash)) + #[allow(dead_code)] + pub fn contains_fn(&self, hash: u64, public_only: bool) -> bool { + self.0.iter().any(|p| p.contains_fn(hash, public_only)) } /// Get specified function via its hash key. - pub fn get_fn(&self, hash: u64) -> Option<&CallableFunction> { + pub fn get_fn(&self, hash: u64, public_only: bool) -> Option<&CallableFunction> { self.0 .iter() - .map(|p| p.get_fn(hash)) + .map(|p| p.get_fn(hash, public_only)) .find(|f| f.is_some()) .flatten() } diff --git a/src/result.rs b/src/result.rs index 66fd9cdf..550dcc37 100644 --- a/src/result.rs +++ b/src/result.rs @@ -182,7 +182,7 @@ impl fmt::Display for EvalAltResult { | Self::ErrorVariableNotFound(s, _) | Self::ErrorModuleNotFound(s, _) => write!(f, "{}: '{}'", desc, s)?, - Self::ErrorDotExpr(s, _) if !s.is_empty() => write!(f, "{} {}", desc, s)?, + Self::ErrorDotExpr(s, _) if !s.is_empty() => write!(f, "{}", s)?, Self::ErrorIndexingType(_, _) | Self::ErrorNumericIndexExpr(_) diff --git a/tests/modules.rs b/tests/modules.rs index fd049815..3b00ab17 100644 --- a/tests/modules.rs +++ b/tests/modules.rs @@ -35,7 +35,7 @@ fn test_module_sub_module() -> Result<(), Box> { let m2 = m.get_sub_module("universe").unwrap(); assert!(m2.contains_var("answer")); - assert!(m2.contains_fn(hash_inc)); + assert!(m2.contains_fn(hash_inc, false)); assert_eq!(m2.get_var_value::("answer").unwrap(), 41); diff --git a/tests/time.rs b/tests/time.rs index 9873decf..b3f6668f 100644 --- a/tests/time.rs +++ b/tests/time.rs @@ -1,11 +1,14 @@ #![cfg(not(feature = "no_std"))] #![cfg(not(target_arch = "wasm32"))] -use rhai::{Engine, EvalAltResult, INT}; +use rhai::{Engine, EvalAltResult}; #[cfg(not(feature = "no_float"))] use rhai::FLOAT; +#[cfg(feature = "no_float")] +use rhai::INT; + #[test] fn test_timestamp() -> Result<(), Box> { let engine = Engine::new();