From 5c61827c7ce58b4f28f6f335829fb4278abc35d1 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Thu, 14 May 2020 11:21:56 +0800 Subject: [PATCH] Force-cast local variable names when pushing into scope. --- README.md | 1 + src/any.rs | 52 +++++++++++++++++++------------------- src/engine.rs | 69 +++++++++++++++++++++++++++++++++++++++------------ src/utils.rs | 4 +-- 4 files changed, 82 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 04e5ebd1..0b359294 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ Rhai's current features set: * Freely pass variables/constants into a script via an external [`Scope`] * Fairly efficient (1 million iterations in 0.75 sec on my 5 year old laptop) * Low compile-time overhead (~0.6 sec debug/~3 sec release for script runner app) +* Relatively little `unsafe` code (yes there are some for performance reasons) * [`no-std`](#optional-features) support * [Function overloading](#function-overloading) * [Operator overloading](#operator-overloading) diff --git a/src/any.rs b/src/any.rs index 2682115b..7ce26170 100644 --- a/src/any.rs +++ b/src/any.rs @@ -299,7 +299,7 @@ impl Default for Dynamic { } /// Cast a type into another type. -fn try_cast(a: A) -> Option { +fn unsafe_try_cast(a: A) -> Option { if TypeId::of::() == a.type_id() { // SAFETY: Just checked we have the right type. We explicitly forget the // value immediately after moving out, removing any chance of a destructor @@ -315,7 +315,7 @@ fn try_cast(a: A) -> Option { } /// Cast a Boxed type into another type. -fn cast_box(item: Box) -> Result, Box> { +fn unsafe_cast_box(item: Box) -> Result, Box> { // Only allow casting to the exact same type if TypeId::of::() == TypeId::of::() { // SAFETY: just checked whether we are pointing to the correct type @@ -383,17 +383,17 @@ impl Dynamic { let mut var = Box::new(value); - var = match cast_box::<_, Dynamic>(var) { + var = match unsafe_cast_box::<_, Dynamic>(var) { Ok(d) => return *d, Err(var) => var, }; - var = match cast_box::<_, String>(var) { + var = match unsafe_cast_box::<_, String>(var) { Ok(s) => return Self(Union::Str(s)), Err(var) => var, }; #[cfg(not(feature = "no_index"))] { - var = match cast_box::<_, Array>(var) { + var = match unsafe_cast_box::<_, Array>(var) { Ok(array) => return Self(Union::Array(array)), Err(var) => var, }; @@ -401,7 +401,7 @@ impl Dynamic { #[cfg(not(feature = "no_object"))] { - var = match cast_box::<_, Map>(var) { + var = match unsafe_cast_box::<_, Map>(var) { Ok(map) => return Self(Union::Map(map)), Err(var) => var, } @@ -426,23 +426,23 @@ impl Dynamic { /// ``` pub fn try_cast(self) -> Option { if TypeId::of::() == TypeId::of::() { - return cast_box::<_, T>(Box::new(self)).ok().map(|v| *v); + return unsafe_cast_box::<_, T>(Box::new(self)).ok().map(|v| *v); } match self.0 { - Union::Unit(value) => try_cast(value), - Union::Bool(value) => try_cast(value), - Union::Str(value) => cast_box::<_, T>(value).ok().map(|v| *v), - Union::Char(value) => try_cast(value), - Union::Int(value) => try_cast(value), + Union::Unit(value) => unsafe_try_cast(value), + Union::Bool(value) => unsafe_try_cast(value), + Union::Str(value) => unsafe_cast_box::<_, T>(value).ok().map(|v| *v), + Union::Char(value) => unsafe_try_cast(value), + Union::Int(value) => unsafe_try_cast(value), #[cfg(not(feature = "no_float"))] - Union::Float(value) => try_cast(value), + Union::Float(value) => unsafe_try_cast(value), #[cfg(not(feature = "no_index"))] - Union::Array(value) => cast_box::<_, T>(value).ok().map(|v| *v), + Union::Array(value) => unsafe_cast_box::<_, T>(value).ok().map(|v| *v), #[cfg(not(feature = "no_object"))] - Union::Map(value) => cast_box::<_, T>(value).ok().map(|v| *v), + Union::Map(value) => unsafe_cast_box::<_, T>(value).ok().map(|v| *v), #[cfg(not(feature = "no_module"))] - Union::Module(value) => cast_box::<_, T>(value).ok().map(|v| *v), + Union::Module(value) => unsafe_cast_box::<_, T>(value).ok().map(|v| *v), Union::Variant(value) => (*value).as_box_any().downcast().map(|x| *x).ok(), } } @@ -467,23 +467,23 @@ impl Dynamic { //self.try_cast::().unwrap() if TypeId::of::() == TypeId::of::() { - return *cast_box::<_, T>(Box::new(self)).unwrap(); + return *unsafe_cast_box::<_, T>(Box::new(self)).unwrap(); } match self.0 { - Union::Unit(value) => try_cast(value).unwrap(), - Union::Bool(value) => try_cast(value).unwrap(), - Union::Str(value) => *cast_box::<_, T>(value).unwrap(), - Union::Char(value) => try_cast(value).unwrap(), - Union::Int(value) => try_cast(value).unwrap(), + Union::Unit(value) => unsafe_try_cast(value).unwrap(), + Union::Bool(value) => unsafe_try_cast(value).unwrap(), + Union::Str(value) => *unsafe_cast_box::<_, T>(value).unwrap(), + Union::Char(value) => unsafe_try_cast(value).unwrap(), + Union::Int(value) => unsafe_try_cast(value).unwrap(), #[cfg(not(feature = "no_float"))] - Union::Float(value) => try_cast(value).unwrap(), + Union::Float(value) => unsafe_try_cast(value).unwrap(), #[cfg(not(feature = "no_index"))] - Union::Array(value) => *cast_box::<_, T>(value).unwrap(), + Union::Array(value) => *unsafe_cast_box::<_, T>(value).unwrap(), #[cfg(not(feature = "no_object"))] - Union::Map(value) => *cast_box::<_, T>(value).unwrap(), + Union::Map(value) => *unsafe_cast_box::<_, T>(value).unwrap(), #[cfg(not(feature = "no_module"))] - Union::Module(value) => *cast_box::<_, T>(value).unwrap(), + Union::Module(value) => *unsafe_cast_box::<_, T>(value).unwrap(), Union::Variant(value) => (*value).as_box_any().downcast().map(|x| *x).unwrap(), } } diff --git a/src/engine.rs b/src/engine.rs index d63787d4..5f730730 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -21,6 +21,7 @@ use crate::parser::ModuleRef; use crate::stdlib::{ any::TypeId, + borrow::Cow, boxed::Box, collections::HashMap, format, @@ -135,6 +136,10 @@ pub struct State<'a> { /// In some situation, e.g. after running an `eval` statement, subsequent offsets may become mis-aligned. /// When that happens, this flag is turned on to force a scope lookup by name. pub always_search: bool, + + /// Level of the current scope. The global (root) level is zero, a new block (or function call) + /// is one level higher, and so on. + pub scope_level: usize, } impl<'a> State<'a> { @@ -143,6 +148,7 @@ impl<'a> State<'a> { Self { always_search: false, fn_lib, + scope_level: 0, } } /// Does a certain script-defined function exist in the `State`? @@ -259,6 +265,24 @@ impl DerefMut for FunctionsLib { } } +/// A dangerous function that blindly casts a `&str` from one lifetime to a `Cow` of +/// another lifetime. This is mainly used to let us push a block-local variable into the +/// current `Scope` without cloning the variable name. Doing this is safe because all local +/// variables in the `Scope` are cleared out before existing the block. +/// +/// Force-casting a local variable lifetime to the current `Scope`'s larger lifetime saves +/// on allocations and string cloning, thus avoids us having to maintain a chain of `Scope`'s. +fn unsafe_cast_var_name<'s>(name: &str, state: &State) -> Cow<'s, str> { + // If not at global level, we can force-cast + if state.scope_level > 0 { + // WARNING - force-cast the variable name into the scope's lifetime to avoid cloning it + // this is safe because all local variables are cleared at the end of the block + unsafe { mem::transmute::<_, &'s str>(name) }.into() + } else { + name.to_string().into() + } +} + /// Rhai main scripting engine. /// /// ``` @@ -656,9 +680,9 @@ impl Engine { /// Function call arguments may be _consumed_ when the function requires them to be passed by value. /// All function arguments not in the first position are always passed by value and thus consumed. /// **DO NOT** reuse the argument values unless for the first `&mut` argument - all others are silently replaced by `()`! - pub(crate) fn call_script_fn<'a>( + pub(crate) fn call_script_fn<'s>( &self, - scope: Option<&mut Scope>, + scope: Option<&mut Scope<'s>>, state: &State, fn_name: &str, fn_def: &FnDef, @@ -672,7 +696,9 @@ impl Engine { let scope_len = scope.len(); let mut state = State::new(state.fn_lib); - // Put arguments into scope as variables - variable name is copied + state.scope_level += 1; + + // Put arguments into scope as variables scope.extend( fn_def .params @@ -681,7 +707,10 @@ impl Engine { // Actually consume the arguments instead of cloning them args.into_iter().map(|v| mem::take(*v)), ) - .map(|(name, value)| (name.clone(), ScopeEntryType::Normal, value)), + .map(|(name, value)| { + let var_name = unsafe_cast_var_name(name.as_str(), &state); + (var_name, ScopeEntryType::Normal, value) + }), ); // Evaluate the function at one higher level of call depth @@ -704,6 +733,8 @@ impl Engine { ))), }); + // Remove all local variables + // No need to reset `state.scope_level` because it is thrown away scope.rewind(scope_len); return result; @@ -712,6 +743,7 @@ impl Engine { _ => { let mut scope = Scope::new(); let mut state = State::new(state.fn_lib); + state.scope_level += 1; // Put arguments into scope as variables scope.extend( @@ -726,6 +758,7 @@ impl Engine { ); // Evaluate the function at one higher level of call depth + // No need to reset `state.scope_level` because it is thrown away return self .eval_stmt(&mut scope, &mut state, &fn_def.body, level + 1) .or_else(|err| match *err { @@ -1510,9 +1543,9 @@ impl Engine { } /// Evaluate a statement - pub(crate) fn eval_stmt( + pub(crate) fn eval_stmt<'s>( &self, - scope: &mut Scope, + scope: &mut Scope<'s>, state: &mut State, stmt: &Stmt, level: usize, @@ -1536,12 +1569,14 @@ impl Engine { // Block scope Stmt::Block(x) => { let prev_len = scope.len(); + state.scope_level += 1; let result = x.0.iter().try_fold(Default::default(), |_, stmt| { self.eval_stmt(scope, state, stmt, level) }); scope.rewind(prev_len); + state.scope_level -= 1; // The impact of an eval statement goes away at the end of a block // because any new variables introduced will go out of scope @@ -1604,9 +1639,10 @@ impl Engine { .or_else(|| self.packages.get_iter(tid)) { // Add the loop variable - let name = x.0.clone(); - scope.push(name, ()); + let var_name = unsafe_cast_var_name(&x.0, &state); + scope.push(var_name, ()); let index = scope.len() - 1; + state.scope_level += 1; for loop_var in iter_fn(iter_type) { *scope.get_mut(index).0 = loop_var; @@ -1622,6 +1658,7 @@ impl Engine { } scope.rewind(scope.len() - 1); + state.scope_level -= 1; Ok(Default::default()) } else { Err(Box::new(EvalAltResult::ErrorFor(x.1.position()))) @@ -1667,15 +1704,15 @@ impl Engine { Stmt::Let(x) if x.1.is_some() => { let ((var_name, _), expr) = x.as_ref(); let val = self.eval_expr(scope, state, expr.as_ref().unwrap(), level)?; - // TODO - avoid copying variable name in inner block? - scope.push_dynamic_value(var_name.clone(), ScopeEntryType::Normal, val, false); + let var_name = unsafe_cast_var_name(var_name, &state); + scope.push_dynamic_value(var_name, ScopeEntryType::Normal, val, false); Ok(Default::default()) } Stmt::Let(x) => { let ((var_name, _), _) = x.as_ref(); - // TODO - avoid copying variable name in inner block? - scope.push(var_name.clone(), ()); + let var_name = unsafe_cast_var_name(var_name, &state); + scope.push(var_name, ()); Ok(Default::default()) } @@ -1683,8 +1720,8 @@ impl Engine { Stmt::Const(x) if x.1.is_constant() => { let ((var_name, _), expr) = x.as_ref(); let val = self.eval_expr(scope, state, &expr, level)?; - // TODO - avoid copying variable name in inner block? - scope.push_dynamic_value(var_name.clone(), ScopeEntryType::Constant, val, true); + let var_name = unsafe_cast_var_name(var_name, &state); + scope.push_dynamic_value(var_name, ScopeEntryType::Constant, val, true); Ok(Default::default()) } @@ -1709,8 +1746,8 @@ impl Engine { let module = resolver.resolve(self, Scope::new(), &path, expr.position())?; - // TODO - avoid copying module name in inner block? - scope.push_module(name.clone(), module); + let mod_name = unsafe_cast_var_name(name, &state); + scope.push_module(mod_name, module); Ok(Default::default()) } else { Err(Box::new(EvalAltResult::ErrorModuleNotFound( diff --git a/src/utils.rs b/src/utils.rs index 381ab1a0..4e53878d 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,6 +1,4 @@ //! Module containing various utility types and functions. -// -// TODO - remove unsafe code use crate::stdlib::{ any::TypeId, @@ -55,6 +53,8 @@ pub fn calc_fn_spec<'a>( /// # Safety /// /// This type uses some unsafe code (mainly to zero out unused array slots) for efficiency. +// +// TODO - remove unsafe code #[derive(Clone, Hash)] pub struct StaticVec { /// Total number of values held.