Merge pull request #454 from schungx/bug-fixes

Bug fixes

CHANGELOG.md

@@ -4,6 +4,17 @@ Rhai Release Notes
 Version 1.0.6
 =============
 
+Bug fixes
+---------
+
+* Eliminate unnecessary property write-back when accessed via a getter since property getters are assumed to be _pure_.
+* Writing to a property of an indexed valued obtained via an indexer now works properly by writing back the changed value via an index setter.
+
+Enhancements
+------------
+
+* `MultiInputsStream`, `ParseState`, `TokenIterator`, `IdentifierBuilder` and `AccessMode` are exported under the `internals` feature.
+
 
 Version 1.0.5
 =============

README.md

@@ -76,7 +76,7 @@ Example
 
 The [`scripts`](https://github.com/rhaiscript/rhai/tree/master/scripts) subdirectory contains sample Rhai scripts.
 
-Below is a the standard _Fibonacci_ example for scripting languages:
+Below is the standard _Fibonacci_ example for scripting languages:
 
 ```js
 // This Rhai script calculates the n-th Fibonacci number using a really dumb algorithm
@@ -93,7 +93,7 @@ fn fib(n) {
     }
 }
 
-print(`Running Fibonacci(28) x ${REPEAT} times...`);
+print(`Running Fibonacci(${TARGET}) x ${REPEAT} times...`);
 print("Ready... Go!");
 
 let result;

codegen/ui_tests/rhai_fn_non_clonable_return.stderr

@@ -8,7 +8,7 @@ error[E0277]: the trait bound `NonClonable: Clone` is not satisfied
      |        the trait `Clone` is not implemented for `NonClonable`
      |
 note: required by a bound in `rhai::Dynamic::from`
-    --> $DIR/dynamic.rs:1043:30
+    --> $DIR/dynamic.rs:1044:30
      |
-1043 |     pub fn from<T: Variant + Clone>(mut value: T) -> Self {
+1044 |     pub fn from<T: Variant + Clone>(mut value: T) -> Self {
      |                              ^^^^^ required by this bound in `rhai::Dynamic::from`

codegen/ui_tests/rhai_mod_non_clonable_return.stderr

@@ -8,7 +8,7 @@ error[E0277]: the trait bound `NonClonable: Clone` is not satisfied
      |            the trait `Clone` is not implemented for `NonClonable`
      |
 note: required by a bound in `rhai::Dynamic::from`
-    --> $DIR/dynamic.rs:1043:30
+    --> $DIR/dynamic.rs:1044:30
      |
-1043 |     pub fn from<T: Variant + Clone>(mut value: T) -> Self {
+1044 |     pub fn from<T: Variant + Clone>(mut value: T) -> Self {
      |                              ^^^^^ required by this bound in `rhai::Dynamic::from`

codegen/ui_tests/rhai_mod_unknown_type.stderr

@@ -13,9 +13,9 @@ help: a struct with a similar name exists
    |                           ~~~~~
 help: consider importing one of these items
    |
-11 |     use core::fmt::Pointer;
-   |
 11 |     use std::fmt::Pointer;
    |
 11 |     use syn::__private::fmt::Pointer;
    |
+11 |     use core::fmt::Pointer;
+   |

scripts/fibonacci.rhai

@@ -12,7 +12,7 @@ fn fib(n) {
     }
 }
 
-print(`Running Fibonacci(28) x ${REPEAT} times...`);
+print(`Running Fibonacci(${TARGET}) x ${REPEAT} times...`);
 print("Ready... Go!");
 
 let result;

src/dynamic.rs

@@ -147,7 +147,8 @@ impl dyn Variant {
     }
 }
 
-/// Modes of access.
+/// _(internals)_ Modes of access.
+/// Exported under the `internals` feature only.
 #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)]
 pub enum AccessMode {
     /// Mutable.

src/engine.rs

@@ -335,6 +335,20 @@ impl ChainArgument {
             _ => None,
         }
     }
+    /// Return the [position][Position].
+    #[inline(always)]
+    #[must_use]
+    #[allow(dead_code)]
+    pub const fn position(&self) -> Position {
+        match self {
+            #[cfg(not(feature = "no_object"))]
+            ChainArgument::Property(pos) => *pos,
+            #[cfg(not(feature = "no_object"))]
+            ChainArgument::MethodCallArgs(_, pos) => *pos,
+            #[cfg(not(feature = "no_index"))]
+            ChainArgument::IndexValue(_, pos) => *pos,
+        }
+    }
 }
 
 #[cfg(not(feature = "no_object"))]
@@ -1253,6 +1267,7 @@ impl Engine {
             #[cfg(not(feature = "no_index"))]
             ChainType::Indexing => {
                 let pos = rhs.position();
+                let root_pos = idx_val.position();
                 let idx_val = idx_val
                     .into_index_value()
                     .expect("never fails because `chain_type` is `ChainType::Index`");
@@ -1262,17 +1277,50 @@ impl Engine {
                     Expr::Dot(x, term, x_pos) | Expr::Index(x, term, x_pos)
                         if !_terminate_chaining =>
                     {
+                        let mut idx_val_for_setter = idx_val.clone();
                         let idx_pos = x.lhs.position();
-                        let obj_ptr = &mut self.get_indexed_mut(
-                            mods, state, lib, target, idx_val, idx_pos, false, true, level,
-                        )?;
-
                         let rhs_chain = match_chaining_type(rhs);
-                        self.eval_dot_index_chain_helper(
-                            mods, state, lib, this_ptr, obj_ptr, root, &x.rhs, *term, idx_values,
-                            rhs_chain, level, new_val,
-                        )
-                        .map_err(|err| err.fill_position(*x_pos))
+
+                        let (try_setter, result) = {
+                            let mut obj = self.get_indexed_mut(
+                                mods, state, lib, target, idx_val, idx_pos, false, true, level,
+                            )?;
+                            let is_obj_temp_val = obj.is_temp_value();
+                            let obj_ptr = &mut obj;
+
+                            match self.eval_dot_index_chain_helper(
+                                mods, state, lib, this_ptr, obj_ptr, root, &x.rhs, *term,
+                                idx_values, rhs_chain, level, new_val,
+                            ) {
+                                Ok((result, true)) if is_obj_temp_val => {
+                                    (Some(obj.take_or_clone()), (result, true))
+                                }
+                                Ok(result) => (None, result),
+                                Err(err) => return Err(err.fill_position(*x_pos)),
+                            }
+                        };
+
+                        if let Some(mut new_val) = try_setter {
+                            // Try to call index setter if value is changed
+                            let hash_set =
+                                FnCallHashes::from_native(crate::calc_fn_hash(FN_IDX_SET, 3));
+                            let args = &mut [target, &mut idx_val_for_setter, &mut new_val];
+
+                            if let Err(err) = self.exec_fn_call(
+                                mods, state, lib, FN_IDX_SET, hash_set, args, is_ref_mut, true,
+                                root_pos, None, level,
+                            ) {
+                                // Just ignore if there is no index setter
+                                if !matches!(*err, EvalAltResult::ErrorFunctionNotFound(_, _)) {
+                                    return Err(err);
+                                }
+                            }
+                        }
+
+                        self.check_data_size(target.as_ref())
+                            .map_err(|err| err.fill_position(root.1))?;
+
+                        Ok(result)
                     }
                     // xxx[rhs] op= new_val
                     _ if new_val.is_some() => {
@@ -1305,11 +1353,10 @@ impl Engine {
                             let hash_set =
                                 FnCallHashes::from_native(crate::calc_fn_hash(FN_IDX_SET, 3));
                             let args = &mut [target, &mut idx_val_for_setter, &mut new_val];
-                            let pos = Position::NONE;
 
                             self.exec_fn_call(
                                 mods, state, lib, FN_IDX_SET, hash_set, args, is_ref_mut, true,
-                                pos, None, level,
+                                root_pos, None, level,
                             )?;
                         }
 
@@ -1480,6 +1527,7 @@ impl Engine {
                                 }
                                 _ => Err(err),
                             },
+                            // Assume getters are always pure
                             |(v, _)| Ok((v, false)),
                         )
                     }
@@ -1533,7 +1581,9 @@ impl Engine {
                                 let hash_set = FnCallHashes::from_native(*hash_set);
                                 let mut arg_values = [target.as_mut(), &mut Default::default()];
                                 let args = &mut arg_values[..1];
-                                let (mut val, updated) = self
+
+                                // Assume getters are always pure
+                                let (mut val, _) = self
                                     .exec_fn_call(
                                         mods, state, lib, getter, hash_get, args, is_ref_mut, true,
                                         *pos, None, level,
@@ -1577,7 +1627,7 @@ impl Engine {
                                     .map_err(|err| err.fill_position(*x_pos))?;
 
                                 // Feed the value back via a setter just in case it has been updated
-                                if updated || may_be_changed {
+                                if may_be_changed {
                                     // Re-use args because the first &mut parameter will not be consumed
                                     let mut arg_values = [target.as_mut(), val];
                                     let args = &mut arg_values;
@@ -2341,7 +2391,7 @@ impl Engine {
                 let args = &mut [lhs_ptr_inner, &mut new_val];
 
                 match self.call_native_fn(mods, state, lib, op, hash, args, true, true, op_pos) {
-                    Err(err) if matches!(err.as_ref(), EvalAltResult::ErrorFunctionNotFound(f, _) if f.starts_with(op)) =>
+                    Err(err) if matches!(*err, EvalAltResult::ErrorFunctionNotFound(ref f, _) if f.starts_with(op)) =>
                     {
                         // Expand to `var = var op rhs`
                         let op = &op[..op.len() - 1]; // extract operator without =

src/lib.rs

@@ -212,7 +212,7 @@ pub use optimize::OptimizationLevel;
 
 #[cfg(feature = "internals")]
 #[deprecated = "this type is volatile and may change"]
-pub use dynamic::{DynamicReadLock, DynamicWriteLock, Variant};
+pub use dynamic::{AccessMode, DynamicReadLock, DynamicWriteLock, Variant};
 
 // Expose internal data structures.
 #[cfg(feature = "internals")]
@@ -222,7 +222,14 @@ pub use token::{get_next_token, parse_string_literal};
 // Expose internal data structures.
 #[cfg(feature = "internals")]
 #[deprecated = "this type is volatile and may change"]
-pub use token::{InputStream, Token, TokenizeState, TokenizerControl, TokenizerControlBlock};
+pub use token::{
+    InputStream, MultiInputsStream, Token, TokenIterator, TokenizeState, TokenizerControl,
+    TokenizerControlBlock,
+};
+
+#[cfg(feature = "internals")]
+#[deprecated = "this type is volatile and may change"]
+pub use parse::{IdentifierBuilder, ParseState};
 
 #[cfg(feature = "internals")]
 #[deprecated = "this type is volatile and may change"]

src/parse.rs

@@ -44,7 +44,8 @@ const SCOPE_SEARCH_BARRIER_MARKER: &str = "$BARRIER$";
 /// The message: never fails because `TokenStream` never ends
 const NEVER_ENDS: &str = "never fails because `TokenStream` never ends";
 
-/// A factory of identifiers from text strings.
+/// _(internals)_ A factory of identifiers from text strings.
+/// Exported under the `internals` feature only.
 ///
 /// When [`SmartString`](https://crates.io/crates/smartstring) is used as [`Identifier`],
 /// this just returns a copy because most identifiers in Rhai are short and ASCII-based.
@@ -74,38 +75,39 @@ impl IdentifierBuilder {
     }
 }
 
-/// A type that encapsulates the current state of the parser.
+/// _(internals)_ A type that encapsulates the current state of the parser.
+/// Exported under the `internals` feature only.
 #[derive(Debug)]
 pub struct ParseState<'e> {
     /// Reference to the scripting [`Engine`].
-    engine: &'e Engine,
+    pub engine: &'e Engine,
     /// Input stream buffer containing the next character to read.
-    tokenizer_control: TokenizerControl,
+    pub tokenizer_control: TokenizerControl,
     /// Interned strings.
-    interned_strings: IdentifierBuilder,
+    pub interned_strings: IdentifierBuilder,
     /// Encapsulates a local stack with variable names to simulate an actual runtime scope.
-    stack: Vec<(Identifier, AccessMode)>,
+    pub stack: Vec<(Identifier, AccessMode)>,
     /// Size of the local variables stack upon entry of the current block scope.
-    entry_stack_len: usize,
+    pub entry_stack_len: usize,
     /// Tracks a list of external variables (variables that are not explicitly declared in the scope).
     #[cfg(not(feature = "no_closure"))]
-    external_vars: BTreeMap<Identifier, Position>,
+    pub external_vars: BTreeMap<Identifier, Position>,
     /// An indicator that disables variable capturing into externals one single time
     /// up until the nearest consumed Identifier token.
     /// If set to false the next call to [`access_var`][ParseState::access_var] will not capture the variable.
     /// All consequent calls to [`access_var`][ParseState::access_var] will not be affected
     #[cfg(not(feature = "no_closure"))]
-    allow_capture: bool,
+    pub allow_capture: bool,
     /// Encapsulates a local stack with imported [module][crate::Module] names.
     #[cfg(not(feature = "no_module"))]
-    modules: StaticVec<Identifier>,
+    pub modules: StaticVec<Identifier>,
     /// Maximum levels of expression nesting.
     #[cfg(not(feature = "unchecked"))]
-    max_expr_depth: Option<NonZeroUsize>,
+    pub max_expr_depth: Option<NonZeroUsize>,
     /// Maximum levels of expression nesting in functions.
     #[cfg(not(feature = "unchecked"))]
     #[cfg(not(feature = "no_function"))]
-    max_function_expr_depth: Option<NonZeroUsize>,
+    pub max_function_expr_depth: Option<NonZeroUsize>,
 }
 
 impl<'e> ParseState<'e> {

src/token.rs

@@ -2057,15 +2057,17 @@ pub fn is_id_continue(x: char) -> bool {
     x.is_ascii_alphanumeric() || x == '_'
 }
 
-/// A type that implements the [`InputStream`] trait.
+/// _(internals)_ A type that implements the [`InputStream`] trait.
+/// Exported under the `internals` feature only.
+///
 /// Multiple character streams are jointed together to form one single stream.
 pub struct MultiInputsStream<'a> {
     /// Buffered character, if any.
-    buf: Option<char>,
+    pub buf: Option<char>,
     /// The current stream index.
-    index: usize,
+    pub index: usize,
     /// The input character streams.
-    streams: StaticVec<Peekable<Chars<'a>>>,
+    pub streams: StaticVec<Peekable<Chars<'a>>>,
 }
 
 impl InputStream for MultiInputsStream<'_> {
@@ -2115,20 +2117,21 @@ impl InputStream for MultiInputsStream<'_> {
     }
 }
 
-/// An iterator on a [`Token`] stream.
+/// _(internals)_ An iterator on a [`Token`] stream.
+/// Exported under the `internals` feature only.
 pub struct TokenIterator<'a> {
     /// Reference to the scripting `Engine`.
-    engine: &'a Engine,
+    pub engine: &'a Engine,
     /// Current state.
-    state: TokenizeState,
+    pub state: TokenizeState,
     /// Current position.
-    pos: Position,
+    pub pos: Position,
     /// External buffer containing the next character to read, if any.
-    tokenizer_control: TokenizerControl,
+    pub tokenizer_control: TokenizerControl,
     /// Input character stream.
-    stream: MultiInputsStream<'a>,
+    pub stream: MultiInputsStream<'a>,
     /// A processor function that maps a token to another.
-    map: Option<fn(Token) -> Token>,
+    pub map: Option<fn(Token) -> Token>,
 }
 
 impl<'a> Iterator for TokenIterator<'a> {