From 00b189d0c6e12dc1d9e6e3a41c40789fdea5268e Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 15 Jan 2022 11:26:43 +0800 Subject: [PATCH] Replace Cow in Scope with SmartString. --- CHANGELOG.md | 1 + src/eval/stmt.rs | 56 +++++++++++++++------------------ src/func/script.rs | 21 +++---------- src/tests.rs | 4 +-- src/types/scope.rs | 77 +++++++++++++++++++++++++--------------------- src/unsafe.rs | 17 ---------- tests/serde.rs | 2 +- 7 files changed, 75 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 994b36c9..79d1bf39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ Enhancements ------------ * Formatting of return types in functions metadata info is improved. +* Use `SmartString` for `Scope` variable names and remove `unsafe` lifetime casting. Version 1.4.0 diff --git a/src/eval/stmt.rs b/src/eval/stmt.rs index aff46fe6..aef4a313 100644 --- a/src/eval/stmt.rs +++ b/src/eval/stmt.rs @@ -3,15 +3,11 @@ use super::{EvalState, GlobalRuntimeState, Target}; use crate::ast::{Expr, Ident, OpAssignment, Stmt, AST_OPTION_FLAGS::*}; use crate::func::get_hasher; -use crate::r#unsafe::unsafe_cast_var_name_to_lifetime; use crate::types::dynamic::{AccessMode, Union}; use crate::{Dynamic, Engine, Module, Position, RhaiResult, RhaiResultOf, Scope, ERR, INT}; +use std::hash::{Hash, Hasher}; #[cfg(feature = "no_std")] use std::prelude::v1::*; -use std::{ - borrow::Cow, - hash::{Hash, Hasher}, -}; impl Engine { /// Evaluate a statements block. @@ -492,19 +488,13 @@ impl Engine { // Add the loop variables let orig_scope_len = scope.len(); let counter_index = if let Some(counter) = counter { - // Loop variables are always removed at the end of the statement - // so this cast is safe. - let counter_name = unsafe_cast_var_name_to_lifetime(&counter.name); - scope.push(counter_name, 0 as INT); + scope.push(counter.name.clone(), 0 as INT); scope.len() - 1 } else { usize::MAX }; - // Loop variables are always removed at the end of the statement - // so this cast is safe. - let var_name = unsafe_cast_var_name_to_lifetime(var_name); - scope.push(var_name, ()); + scope.push(var_name.clone(), ()); let index = scope.len() - 1; let mut loop_result = Ok(Dynamic::UNIT); @@ -514,11 +504,12 @@ impl Engine { if counter_index < usize::MAX { #[cfg(not(feature = "unchecked"))] if x > INT::MAX as usize { - return Err(ERR::ErrorArithmetic( + loop_result = Err(ERR::ErrorArithmetic( format!("for-loop counter overflow: {}", x), counter.as_ref().expect("`Some`").pos, ) .into()); + break; } let index_value = (x as INT).into(); @@ -575,7 +566,10 @@ impl Engine { Err(err) => match *err { ERR::LoopBreak(false, _) => (), ERR::LoopBreak(true, _) => break, - _ => return Err(err), + _ => { + loop_result = Err(err); + break; + } }, } } @@ -645,12 +639,9 @@ impl Engine { let orig_scope_len = scope.len(); - err_var_name.as_ref().map(|Ident { name, .. }| { - // Catch error variables are always removed from after the block - // so this cast is safe. - let var_name = unsafe_cast_var_name_to_lifetime(name); - scope.push(var_name, err_value) - }); + err_var_name + .as_ref() + .map(|Ident { name, .. }| scope.push(name.clone(), err_value)); let result = self.eval_stmt_block( scope, global, state, lib, this_ptr, catch_stmt, true, level, @@ -701,7 +692,7 @@ impl Engine { // Let/const statement Stmt::Var(expr, x, options, _) => { - let name = &x.name; + let var_name = &x.name; let entry_type = if options.contains(AST_OPTION_CONSTANT) { AccessMode::ReadOnly } else { @@ -713,7 +704,7 @@ impl Engine { .eval_expr(scope, global, state, lib, this_ptr, expr, level)? .flatten(); - let (var_name, _alias): (Cow<'_, str>, _) = if !rewind_scope { + let _alias = if !rewind_scope { #[cfg(not(feature = "no_function"))] #[cfg(not(feature = "no_module"))] if state.scope_level == 0 @@ -721,23 +712,26 @@ impl Engine { && lib.iter().any(|&m| !m.is_empty()) { // Add a global constant if at top level and there are functions - global.set_constant(name.clone(), value.clone()); + global.set_constant(var_name.clone(), value.clone()); } - ( - name.to_string().into(), - if export { Some(name.clone()) } else { None }, - ) + if export { + Some(var_name) + } else { + None + } } else if export { unreachable!("exported variable not on global level"); } else { - (unsafe_cast_var_name_to_lifetime(name).into(), None) + None }; - scope.push_dynamic_value(var_name, entry_type, value); + scope.push_dynamic_value(var_name.clone(), entry_type, value); #[cfg(not(feature = "no_module"))] - _alias.map(|alias| scope.add_entry_alias(scope.len() - 1, alias)); + if let Some(alias) = _alias { + scope.add_entry_alias(scope.len() - 1, alias.clone()); + } Ok(Dynamic::UNIT) } diff --git a/src/func/script.rs b/src/func/script.rs index 51d7305e..baf4a66c 100644 --- a/src/func/script.rs +++ b/src/func/script.rs @@ -4,7 +4,6 @@ use super::call::FnCallArgs; use crate::ast::ScriptFnDef; use crate::eval::{EvalState, GlobalRuntimeState}; -use crate::r#unsafe::unsafe_cast_var_name_to_lifetime; use crate::{Dynamic, Engine, Module, Position, RhaiError, RhaiResult, Scope, StaticVec, ERR}; use std::mem; #[cfg(feature = "no_std")] @@ -74,22 +73,10 @@ impl Engine { let orig_mods_len = global.num_imported_modules(); // Put arguments into scope as variables - scope.extend( - fn_def - .params - .iter() - .zip(args.into_iter().map(|v| { - // Actually consume the arguments instead of cloning them - mem::take(*v) - })) - .map(|(name, value)| { - // Arguments are always removed at the end of the call, - // so this cast is safe. - let var_name: std::borrow::Cow<_> = - unsafe_cast_var_name_to_lifetime(name).into(); - (var_name, value) - }), - ); + scope.extend(fn_def.params.iter().cloned().zip(args.into_iter().map(|v| { + // Actually consume the arguments instead of cloning them + mem::take(*v) + }))); // Merge in encapsulated environment, if any let mut lib_merged = StaticVec::with_capacity(lib.len() + 1); diff --git a/src/tests.rs b/src/tests.rs index 322e6cdb..f4d32675 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -23,11 +23,11 @@ fn check_struct_sizes() { assert_eq!(size_of::>(), if PACKED { 12 } else { 16 }); assert_eq!(size_of::(), if PACKED { 24 } else { 32 }); assert_eq!(size_of::>(), if PACKED { 24 } else { 32 }); - assert_eq!(size_of::(), if PACKED { 40 } else { 80 }); - assert_eq!(size_of::(), if PACKED { 232 } else { 464 }); #[cfg(target_pointer_width = "64")] { + assert_eq!(size_of::(), 400); + assert_eq!(size_of::(), 80); assert_eq!(size_of::(), 56); assert_eq!( size_of::(), diff --git a/src/types/scope.rs b/src/types/scope.rs index 84c4c446..56f21748 100644 --- a/src/types/scope.rs +++ b/src/types/scope.rs @@ -3,10 +3,12 @@ use super::dynamic::{AccessMode, Variant}; use crate::{Dynamic, Identifier, StaticVec}; use smallvec::SmallVec; -use std::iter::FromIterator; #[cfg(feature = "no_std")] use std::prelude::v1::*; -use std::{borrow::Cow, iter::Extend}; +use std::{ + iter::{Extend, FromIterator}, + marker::PhantomData, +}; /// Keep a number of entries inline (since [`Dynamic`] is usually small enough). const SCOPE_ENTRIES_INLINED: usize = 8; @@ -14,6 +16,11 @@ const SCOPE_ENTRIES_INLINED: usize = 8; /// Type containing information about the current scope. Useful for keeping state between /// [`Engine`][crate::Engine] evaluation runs. /// +/// # Lifetime +/// +/// Currently the lifetime parameter is not used, but it is not guaranteed to remain unused for +/// future versions. Until then, `'static` can be used. +/// /// # Thread Safety /// /// Currently, [`Scope`] is neither [`Send`] nor [`Sync`]. Turn on the `sync` feature to make it @@ -47,7 +54,7 @@ const SCOPE_ENTRIES_INLINED: usize = 8; // // [`Scope`] is implemented as two [`Vec`]'s of exactly the same length. Variables data (name, // type, etc.) is manually split into two equal-length arrays. That's because variable names take -// up the most space, with [`Cow`][Cow] being four words long, but in the vast majority of +// up the most space, with [`Identifier`] being four words long, but in the vast majority of // cases the name is NOT used to look up a variable. Variable lookup is usually via direct // indexing, by-passing the name altogether. // @@ -58,25 +65,30 @@ pub struct Scope<'a> { /// Current value of the entry. values: SmallVec<[Dynamic; SCOPE_ENTRIES_INLINED]>, /// (Name, aliases) of the entry. - names: SmallVec<[(Cow<'a, str>, Option>>); SCOPE_ENTRIES_INLINED]>, + names: SmallVec<[(Identifier, Option>>); SCOPE_ENTRIES_INLINED]>, + /// Phantom to keep the lifetime parameter in order not to break existing code. + phantom: PhantomData<&'a ()>, } -impl<'a> IntoIterator for Scope<'a> { - type Item = (Cow<'a, str>, Dynamic); - type IntoIter = Box + 'a>; +impl IntoIterator for Scope<'_> { + type Item = (String, Dynamic, Vec); + type IntoIter = Box>; #[inline] fn into_iter(self) -> Self::IntoIter { - Box::new( - self.values - .into_iter() - .zip(self.names.into_iter()) - .map(|(value, (name, _))| (name, value)), - ) + Box::new(self.values.into_iter().zip(self.names.into_iter()).map( + |(value, (name, alias))| { + ( + name.into(), + value, + alias.map(|a| a.to_vec()).unwrap_or_default(), + ) + }, + )) } } -impl<'a> Scope<'a> { +impl Scope<'_> { /// Create a new [`Scope`]. /// /// # Example @@ -95,6 +107,7 @@ impl<'a> Scope<'a> { Self { values: SmallVec::new_const(), names: SmallVec::new_const(), + phantom: PhantomData, } } /// Empty the [`Scope`]. @@ -171,11 +184,7 @@ impl<'a> Scope<'a> { /// assert_eq!(my_scope.get_value::("x").expect("x should exist"), 42); /// ``` #[inline(always)] - pub fn push( - &mut self, - name: impl Into>, - value: impl Variant + Clone, - ) -> &mut Self { + pub fn push(&mut self, name: impl Into, value: impl Variant + Clone) -> &mut Self { self.push_dynamic_value(name, AccessMode::ReadWrite, Dynamic::from(value)) } /// Add (push) a new [`Dynamic`] entry to the [`Scope`]. @@ -191,7 +200,7 @@ impl<'a> Scope<'a> { /// assert_eq!(my_scope.get_value::("x").expect("x should exist"), 42); /// ``` #[inline(always)] - pub fn push_dynamic(&mut self, name: impl Into>, value: Dynamic) -> &mut Self { + pub fn push_dynamic(&mut self, name: impl Into, value: Dynamic) -> &mut Self { self.push_dynamic_value(name, value.access_mode(), value) } /// Add (push) a new constant to the [`Scope`]. @@ -212,7 +221,7 @@ impl<'a> Scope<'a> { #[inline(always)] pub fn push_constant( &mut self, - name: impl Into>, + name: impl Into, value: impl Variant + Clone, ) -> &mut Self { self.push_dynamic_value(name, AccessMode::ReadOnly, Dynamic::from(value)) @@ -235,7 +244,7 @@ impl<'a> Scope<'a> { #[inline(always)] pub fn push_constant_dynamic( &mut self, - name: impl Into>, + name: impl Into, value: Dynamic, ) -> &mut Self { self.push_dynamic_value(name, AccessMode::ReadOnly, value) @@ -244,7 +253,7 @@ impl<'a> Scope<'a> { #[inline] pub(crate) fn push_dynamic_value( &mut self, - name: impl Into>, + name: impl Into, access: AccessMode, mut value: Dynamic, ) -> &mut Self { @@ -301,7 +310,7 @@ impl<'a> Scope<'a> { #[inline] #[must_use] pub fn contains(&self, name: &str) -> bool { - self.names.iter().any(|(key, _)| name == key.as_ref()) + self.names.iter().any(|(key, _)| name == key) } /// Find an entry in the [`Scope`], starting from the last. #[inline] @@ -314,7 +323,7 @@ impl<'a> Scope<'a> { .rev() // Always search a Scope in reverse order .enumerate() .find_map(|(i, (key, _))| { - if name == key.as_ref() { + if name == key { let index = len - 1 - i; Some((index, self.values[index].access_mode())) } else { @@ -343,7 +352,7 @@ impl<'a> Scope<'a> { .iter() .rev() .enumerate() - .find(|(_, (key, _))| name == key.as_ref()) + .find(|(_, (key, _))| name == key) .and_then(|(index, _)| self.values[len - 1 - index].flatten_clone().try_cast()) } /// Check if the named entry in the [`Scope`] is constant. @@ -398,7 +407,7 @@ impl<'a> Scope<'a> { #[inline] pub fn set_or_push( &mut self, - name: impl AsRef + Into>, + name: impl AsRef + Into, value: impl Variant + Clone, ) -> &mut Self { match self.get_index(name.as_ref()) { @@ -437,7 +446,7 @@ impl<'a> Scope<'a> { #[inline] pub fn set_value( &mut self, - name: impl AsRef + Into>, + name: impl AsRef + Into, value: impl Variant + Clone, ) -> &mut Self { match self.get_index(name.as_ref()) { @@ -535,9 +544,7 @@ impl<'a> Scope<'a> { /// Get an iterator to entries in the [`Scope`]. #[inline] #[allow(dead_code)] - pub(crate) fn into_iter( - self, - ) -> impl Iterator, Dynamic, Vec)> { + pub(crate) fn into_iter(self) -> impl Iterator)> { self.names .into_iter() .zip(self.values.into_iter()) @@ -597,7 +604,7 @@ impl<'a> Scope<'a> { } } -impl<'a, K: Into>> Extend<(K, Dynamic)> for Scope<'a> { +impl> Extend<(K, Dynamic)> for Scope<'_> { #[inline] fn extend>(&mut self, iter: T) { iter.into_iter().for_each(|(name, value)| { @@ -606,7 +613,7 @@ impl<'a, K: Into>> Extend<(K, Dynamic)> for Scope<'a> { } } -impl<'a, K: Into>> FromIterator<(K, Dynamic)> for Scope<'a> { +impl> FromIterator<(K, Dynamic)> for Scope<'_> { #[inline] fn from_iter>(iter: T) -> Self { let mut scope = Self::new(); @@ -615,7 +622,7 @@ impl<'a, K: Into>> FromIterator<(K, Dynamic)> for Scope<'a> { } } -impl<'a, K: Into>> Extend<(K, bool, Dynamic)> for Scope<'a> { +impl> Extend<(K, bool, Dynamic)> for Scope<'_> { #[inline] fn extend>(&mut self, iter: T) { iter.into_iter().for_each(|(name, is_constant, value)| { @@ -632,7 +639,7 @@ impl<'a, K: Into>> Extend<(K, bool, Dynamic)> for Scope<'a> { } } -impl<'a, K: Into>> FromIterator<(K, bool, Dynamic)> for Scope<'a> { +impl> FromIterator<(K, bool, Dynamic)> for Scope<'_> { #[inline] fn from_iter>(iter: T) -> Self { let mut scope = Self::new(); diff --git a/src/unsafe.rs b/src/unsafe.rs index ab272fa8..9c1eb467 100644 --- a/src/unsafe.rs +++ b/src/unsafe.rs @@ -52,20 +52,3 @@ pub fn unsafe_cast_box(item: Box) -> Option { None } } - -/// # DANGEROUS!!! -/// -/// A dangerous function that blindly casts a `&str` from one lifetime to a `&str` of -/// another lifetime. This is mainly used to let us push a block-local variable into the -/// current [`Scope`][crate::Scope] without cloning the variable name. Doing this is safe because all local -/// variables in the [`Scope`][crate::Scope] are cleared out before existing the block. -/// -/// Force-casting a local variable's lifetime to the current [`Scope`][crate::Scope]'s larger lifetime saves -/// on allocations and string cloning, thus avoids us having to maintain a chain of [`Scope`][crate::Scope]'s. -#[inline(always)] -#[must_use] -pub fn unsafe_cast_var_name_to_lifetime<'s>(name: &str) -> &'s str { - // 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(name) } -} diff --git a/tests/serde.rs b/tests/serde.rs index 08bbeccc..b6541d3d 100644 --- a/tests/serde.rs +++ b/tests/serde.rs @@ -12,7 +12,7 @@ use rhai::Array; use rhai::Map; #[cfg(not(feature = "no_float"))] use rhai::FLOAT; -#[cfg(not(feature = "no_float"))] +#[cfg(feature = "no_float")] #[cfg(feature = "decimal")] use rust_decimal::Decimal;