Merge pull request #726 from schungx/master

Fix static hashing key bug.

CHANGELOG.md

@@ -1,9 +1,29 @@
 Rhai Release Notes
 ==================
 
+Version 1.15.0
+==============
+
+Bug fixes
+---------
+
+* Fixes a concurrency error in static hashing keys (thanks [`garypen`](https://github.com/garypen)!).
+
+Enhancements
+------------
+
+* Expressions involving `this` should now run slightly faster due to a dedicated `AST` node `ThisPtr`.
+* A `take` function is added to the standard library to take ownership of any data (replacing with `()`) in order to avoid cloning.
+* `EvalAltResult::ErrorMismatchOutputType` now gives a better name for the requested generic type (e.g. `&str` is now `&str` and not `string`).
+
+
 Version 1.14.0
 ==============
 
+This new version contains a substantial number of bug fixes for edge cases.
+
+A new syntax is supported to facilitate writing object methods in script.
+
 The code hacks that attempt to optimize branch prediction performance are removed because benchmarks do not show any material speed improvements.
 
 Bug fixes

Cargo.toml

@@ -3,7 +3,7 @@ members = [".", "codegen"]
 
 [package]
 name = "rhai"
-version = "1.14.0"
+version = "1.15.0"
 rust-version = "1.61.0"
 edition = "2018"
 resolver = "2"
@@ -25,7 +25,7 @@ bitflags = { version = "1", default-features = false }
 smartstring = { version = "1", default-features = false }
 rhai_codegen = { version = "1.5.0", path = "codegen", default-features = false }
 
-no-std-compat = { git = "https://gitlab.com/jD91mZM2/no-std-compat", default-features = false, features = ["alloc"], optional = true }
+no-std-compat = { git = "https://gitlab.com/jD91mZM2/no-std-compat", version = "0.4.1", default-features = false, features = ["alloc"], optional = true }
 libm = { version = "0.2", default-features = false, optional = true }
 hashbrown = { version = "0.13", optional = true }
 core-error = { version = "0.0", default-features = false, features = ["alloc"], optional = true }

examples/event_handler_js/script.rhai

@@ -22,11 +22,12 @@ fn start(data) {
     if this.value <= 0 {
         throw "Conditions not yet ready to start!";
     }
-    this.bool_state = true;
-    this.value += parse_int(data);
 
     // Constant 'MY_CONSTANT' in custom scope is also visible!
     print(`MY_CONSTANT = ${MY_CONSTANT}`);
+
+    this.value += parse_int(data);
+    this.bool_state = true;
 }
 
 /// 'end' event handler
@@ -37,8 +38,8 @@ fn end(data) {
     if this.value > 0 {
         throw "Conditions not yet ready to end!";
     }
-    this.bool_state = false;
     this.value = parse_int(data);
+    this.bool_state = false;
 }
 
 /// 'update' event handler

examples/event_handler_main/script.rhai

@@ -23,14 +23,14 @@ fn start(data) {
     if value <= 0 {
         throw "Conditions not yet ready to start!";
     }
-    bool_state = true;
 
     // Constants 'MY_CONSTANT' and 'EXTRA_CONSTANT'
     // in custom scope are also visible!
     print(`MY_CONSTANT = ${MY_CONSTANT}`);
     print(`EXTRA_CONSTANT = ${EXTRA_CONSTANT}`);
 
     value += parse_int(data);
+    bool_state = true;
 }
 
 /// 'end' event handler
@@ -41,8 +41,8 @@ fn end(data) {
     if value > 0 {
         throw "Conditions not yet ready to end!";
     }
-    bool_state = false;
     value = parse_int(data);
+    bool_state = false;
 }
 
 /// 'update' event handler

examples/event_handler_map/script.rhai

@@ -28,11 +28,12 @@ fn start(data) {
     if state.value <= 0 {
         throw "Conditions not yet ready to start!";
     }
-    state.bool_state = true;
-    state.value = parse_int(data);
 
     // Constant 'MY_CONSTANT' in custom scope is also visible!
     print(`MY_CONSTANT = ${MY_CONSTANT}`);
+
+    state.value = parse_int(data);
+    state.bool_state = true;
 }
 
 /// 'end' event handler
@@ -43,8 +44,8 @@ fn end(data) {
     if state.value > 0 {
         throw "Conditions not yet ready to end!";
     }
-    state.bool_state = false;
     state.value = parse_int(data);
+    state.bool_state = false;
 }
 
 /// 'update' event handler

src/api/call_fn.rs

@@ -8,10 +8,7 @@ use crate::{
 };
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
-use std::{
-    any::{type_name, TypeId},
-    mem,
-};
+use std::{any::type_name, mem};
 
 /// Options for calling a script-defined function via [`Engine::call_fn_with_options`].
 #[derive(Debug, Hash)]
@@ -181,17 +178,14 @@ impl Engine {
             options,
         )
         .and_then(|result| {
-            // Bail out early if the return type needs no cast
-            if TypeId::of::<T>() == TypeId::of::<Dynamic>() {
-                return Ok(reify! { result => T });
-            }
-
-            // Cast return type
-            let typ = self.map_type_name(result.type_name());
-
-            result.try_cast().ok_or_else(|| {
-                let t = self.map_type_name(type_name::<T>()).into();
-                ERR::ErrorMismatchOutputType(t, typ.into(), Position::NONE).into()
+            result.try_cast_raw().map_err(|r| {
+                let result_type = self.map_type_name(r.type_name());
+                let cast_type = match type_name::<T>() {
+                    typ @ _ if typ.contains("::") => self.map_type_name(typ),
+                    typ @ _ => typ,
+                };
+                ERR::ErrorMismatchOutputType(cast_type.into(), result_type.into(), Position::NONE)
+                    .into()
             })
         })
     }

src/api/custom_syntax.rs

@@ -109,6 +109,8 @@ impl Expression<'_> {
             #[cfg(not(feature = "no_module"))]
             Expr::Variable(x, ..) if !x.1.is_empty() => None,
             Expr::Variable(x, ..) => Some(x.3.as_str()),
+            #[cfg(not(feature = "no_function"))]
+            Expr::ThisPtr(..) => Some(crate::engine::KEYWORD_THIS),
             Expr::StringConstant(x, ..) => Some(x.as_str()),
             _ => None,
         }

src/api/eval.rs

@@ -212,11 +212,18 @@ impl Engine {
             return Ok(reify! { result => T });
         }
 
-        let typ = self.map_type_name(result.type_name());
+        result.try_cast_raw::<T>().map_err(|v| {
+            let typename = match type_name::<T>() {
+                typ @ _ if typ.contains("::") => self.map_type_name(typ),
+                typ @ _ => typ,
+            };
 
-        result.try_cast::<T>().ok_or_else(|| {
-            let t = self.map_type_name(type_name::<T>()).into();
-            ERR::ErrorMismatchOutputType(t, typ.into(), Position::NONE).into()
+            ERR::ErrorMismatchOutputType(
+                typename.into(),
+                self.map_type_name(v.type_name()).into(),
+                Position::NONE,
+            )
+            .into()
         })
     }
     /// Evaluate an [`AST`] with own scope, returning the result value or an error.

src/ast/expr.rs

@@ -297,6 +297,8 @@ pub enum Expr {
         Option<NonZeroU8>,
         Position,
     ),
+    /// `this`.
+    ThisPtr(Position),
     /// Property access - ((getter, hash), (setter, hash), prop)
     Property(
         Box<(
@@ -375,6 +377,7 @@ impl fmt::Debug for Expr {
                     .entries(x.0.iter().map(|(k, v)| (k, v)))
                     .finish()
             }
+            Self::ThisPtr(..) => f.debug_struct("ThisPtr").finish(),
             Self::Variable(x, i, ..) => {
                 f.write_str("Variable(")?;
 
@@ -632,6 +635,7 @@ impl Expr {
             | Self::Array(..)
             | Self::Map(..)
             | Self::Variable(..)
+            | Self::ThisPtr(..)
             | Self::And(..)
             | Self::Or(..)
             | Self::Coalesce(..)
@@ -662,6 +666,7 @@ impl Expr {
             | Self::Array(.., pos)
             | Self::Map(.., pos)
             | Self::Variable(.., pos)
+            | Self::ThisPtr(pos)
             | Self::And(.., pos)
             | Self::Or(.., pos)
             | Self::Coalesce(.., pos)
@@ -725,6 +730,7 @@ impl Expr {
             | Self::Dot(.., pos)
             | Self::Index(.., pos)
             | Self::Variable(.., pos)
+            | Self::ThisPtr(pos)
             | Self::FnCall(.., pos)
             | Self::MethodCall(.., pos)
             | Self::InterpolatedString(.., pos)
@@ -816,6 +822,7 @@ impl Expr {
             | Self::StringConstant(..)
             | Self::InterpolatedString(..)
             | Self::FnCall(..)
+            | Self::ThisPtr(..)
             | Self::MethodCall(..)
             | Self::Stmt(..)
             | Self::Dot(..)

src/config/hashing.rs

@@ -51,9 +51,9 @@ impl HokmaLock {
     pub fn write(&'static self) -> WhenTheHokmaSuppression {
         loop {
             // We are only interested in error results
-            if let Err(previous) = self
-                .lock
-                .compare_exchange(1, 1, Ordering::SeqCst, Ordering::SeqCst)
+            if let Err(previous) =
+                self.lock
+                    .compare_exchange(1, 1, Ordering::SeqCst, Ordering::SeqCst)
             {
                 // If we failed, previous cannot be 1
                 return WhenTheHokmaSuppression {

src/engine.rs

@@ -29,6 +29,7 @@ pub const KEYWORD_IS_SHARED: &str = "is_shared";
 pub const KEYWORD_IS_DEF_VAR: &str = "is_def_var";
 #[cfg(not(feature = "no_function"))]
 pub const KEYWORD_IS_DEF_FN: &str = "is_def_fn";
+#[cfg(not(feature = "no_function"))]
 pub const KEYWORD_THIS: &str = "this";
 #[cfg(not(feature = "no_function"))]
 #[cfg(not(feature = "no_module"))]

src/eval/chaining.rs

@@ -388,6 +388,23 @@ impl Engine {
         let scope2 = ();
 
         match (lhs, new_val) {
+            // this.??? or this[???]
+            (Expr::ThisPtr(var_pos), new_val) => {
+                self.track_operation(global, *var_pos)?;
+
+                #[cfg(feature = "debugging")]
+                self.run_debugger(global, caches, scope, this_ptr.as_deref_mut(), lhs)?;
+
+                if let Some(this_ptr) = this_ptr {
+                    let target = &mut this_ptr.into();
+
+                    self.eval_dot_index_chain_raw(
+                        global, caches, scope2, None, lhs, expr, target, rhs, idx_values, new_val,
+                    )
+                } else {
+                    Err(ERR::ErrorUnboundThis(*var_pos).into())
+                }
+            }
             // id.??? or id[???]
             (Expr::Variable(.., var_pos), new_val) => {
                 self.track_operation(global, *var_pos)?;

src/eval/expr.rs

@@ -2,7 +2,6 @@
 
 use super::{Caches, EvalContext, GlobalRuntimeState, Target};
 use crate::ast::Expr;
-use crate::engine::KEYWORD_THIS;
 use crate::packages::string_basic::{print_with_func, FUNC_TO_STRING};
 use crate::types::dynamic::AccessMode;
 use crate::{Dynamic, Engine, RhaiResult, RhaiResultOf, Scope, SmartString, ERR};
@@ -140,15 +139,7 @@ impl Engine {
 
         let index = match expr {
             // Check if the variable is `this`
-            Expr::Variable(v, None, ..)
-                if v.0.is_none() && v.1.is_empty() && v.3 == KEYWORD_THIS =>
-            {
-                return if let Some(this_ptr) = this_ptr {
-                    Ok(this_ptr.into())
-                } else {
-                    Err(ERR::ErrorUnboundThis(expr.position()).into())
-                };
-            }
+            Expr::ThisPtr(..) => unreachable!("Expr::ThisPtr should have been handled outside"),
 
             _ if global.always_search_scope => 0,
 
@@ -259,13 +250,9 @@ impl Engine {
                 self.eval_fn_call_expr(global, caches, scope, this_ptr, x, *pos)
             }
 
-            Expr::Variable(x, index, var_pos)
-                if index.is_none() && x.0.is_none() && x.3 == KEYWORD_THIS =>
-            {
-                this_ptr
-                    .ok_or_else(|| ERR::ErrorUnboundThis(*var_pos).into())
-                    .cloned()
-            }
+            Expr::ThisPtr(var_pos) => this_ptr
+                .ok_or_else(|| ERR::ErrorUnboundThis(*var_pos).into())
+                .cloned(),
 
             Expr::Variable(..) => self
                 .search_namespace(global, caches, scope, this_ptr, expr)
@@ -413,13 +400,7 @@ impl Engine {
                     .and_then(|r| self.check_data_size(r, expr.start_position()))
             }
 
-            Expr::Stmt(x) => {
-                if x.is_empty() {
-                    Ok(Dynamic::UNIT)
-                } else {
-                    self.eval_stmt_block(global, caches, scope, this_ptr, x, true)
-                }
-            }
+            Expr::Stmt(x) => self.eval_stmt_block(global, caches, scope, this_ptr, x, true),
 
             #[cfg(not(feature = "no_index"))]
             Expr::Index(..) => {

src/eval/stmt.rs

@@ -302,7 +302,26 @@ impl Engine {
             Stmt::Assignment(x, ..) => {
                 let (op_info, BinaryExpr { lhs, rhs }) = &**x;
 
-                if let Expr::Variable(x, ..) = lhs {
+                if let Expr::ThisPtr(..) = lhs {
+                    if this_ptr.is_none() {
+                        return Err(ERR::ErrorUnboundThis(lhs.position()).into());
+                    }
+
+                    #[cfg(not(feature = "no_function"))]
+                    {
+                        let rhs_val = self
+                            .eval_expr(global, caches, scope, this_ptr.as_deref_mut(), rhs)?
+                            .flatten();
+
+                        self.track_operation(global, lhs.position())?;
+
+                        let target = &mut this_ptr.unwrap().into();
+
+                        self.eval_op_assignment(global, caches, op_info, lhs, target, rhs_val)?;
+                    }
+                    #[cfg(feature = "no_function")]
+                    unreachable!();
+                } else if let Expr::Variable(x, ..) = lhs {
                     let rhs_val = self
                         .eval_expr(global, caches, scope, this_ptr.as_deref_mut(), rhs)?
                         .flatten();
@@ -342,6 +361,10 @@ impl Engine {
                         // Must be either `var[index] op= val` or `var.prop op= val`.
                         // The return value of any op-assignment (should be `()`) is thrown away and not used.
                         let _ = match lhs {
+                            // this op= rhs (handled above)
+                            Expr::ThisPtr(..) => {
+                                unreachable!("Expr::ThisPtr case is already handled")
+                            }
                             // name op= rhs (handled above)
                             Expr::Variable(..) => {
                                 unreachable!("Expr::Variable case is already handled")
@@ -861,9 +884,12 @@ impl Engine {
                 }
 
                 let v = self.eval_expr(global, caches, scope, this_ptr, expr)?;
-                let typ = v.type_name();
-                let path = v.try_cast::<crate::ImmutableString>().ok_or_else(|| {
-                    self.make_type_mismatch_err::<crate::ImmutableString>(typ, expr.position())
+
+                let path = v.try_cast_raw::<crate::ImmutableString>().map_err(|v| {
+                    self.make_type_mismatch_err::<crate::ImmutableString>(
+                        v.type_name(),
+                        expr.position(),
+                    )
                 })?;
 
                 let path_pos = expr.start_position();