VM: Fix chain assignment evaluation order

src/eval/chaining.rs

@@ -11,10 +11,7 @@ use crate::{
 };
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
-use std::{
-    convert::{TryFrom, TryInto},
-    hash::Hash,
-};
+use std::{convert::TryInto, hash::Hash};
 
 /// Function call hashes to index getters and setters.
 static INDEXER_HASHES: OnceCell<(u64, u64)> = OnceCell::new();
@@ -121,6 +118,9 @@ impl Engine {
         _add_if_not_found: bool,
         use_indexers: bool,
     ) -> RhaiResultOf<Target<'t>> {
+        #[cfg(not(feature = "no_index"))]
+        use std::convert::TryFrom;
+
         self.track_operation(global, Position::NONE)?;
 
         match target {

src/func/call.rs

@@ -30,7 +30,7 @@
 
 #[cfg(not(feature = "no_float"))]
 #[cfg(feature = "no_std")]
 use num_traits::Float;
 
 /// Arguments to a function call, which is a list of [`&mut Dynamic`][Dynamic].
 pub type FnCallArgs<'a> = [&'a mut Dynamic];
@@ -479,64 +479,95 @@
         }
 
         // Error handling
+        //
+        // Note: we cannot use `assert!` here (e.g. to check for number of arguments) because
+        //       this function may be called from the VM running a corrupted bytecodes stream!
 
         match name {
             // index getter function not found?
             #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))]
-            crate::engine::FN_IDX_GET => {
-                debug_assert_eq!(args.len(), 2);
-
+            crate::engine::FN_IDX_GET => Err(if args.len() != 2 {
+                ERR::ErrorParsing(
+                    crate::ParseErrorType::MalformedIndexExpr(format!(
+                        "System error: {} argument(s) found for indexer (should be 2)",
+                        args.len()
+                    )),
+                    pos,
+                )
+                .into()
+            } else {
                 let t0 = self.map_type_name(args[0].type_name());
                 let t1 = self.map_type_name(args[1].type_name());
-
-                Err(ERR::ErrorIndexingType(format!("{t0} [{t1}]"), pos).into())
-            }
+                ERR::ErrorIndexingType(format!("{t0} [{t1}]"), pos).into()
+            }),
 
             // index setter function not found?
             #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))]
-            crate::engine::FN_IDX_SET => {
-                debug_assert_eq!(args.len(), 3);
-
+            crate::engine::FN_IDX_SET => Err(if args.len() != 3 {
+                ERR::ErrorParsing(
+                    crate::ParseErrorType::MalformedIndexExpr(format!(
+                        "System error: {} argument(s) found for index setter (should be 3)",
+                        args.len()
+                    )),
+                    pos,
+                )
+                .into()
+            } else {
                 let t0 = self.map_type_name(args[0].type_name());
                 let t1 = self.map_type_name(args[1].type_name());
                 let t2 = self.map_type_name(args[2].type_name());
-
-                Err(ERR::ErrorIndexingType(format!("{t0} [{t1}] = {t2}"), pos).into())
-            }
+                ERR::ErrorIndexingType(format!("{t0} [{t1}] = {t2}"), pos).into()
+            }),
 
             // Getter function not found?
             #[cfg(not(feature = "no_object"))]
             _ if name.starts_with(crate::engine::FN_GET) => {
-                debug_assert_eq!(args.len(), 1);
-
-                let prop = &name[crate::engine::FN_GET.len()..];
-                let t0 = self.map_type_name(args[0].type_name());
-
-                Err(ERR::ErrorDotExpr(
-                    format!(
-                        "Unknown property '{prop}' - a getter is not registered for type '{t0}'"
-                    ),
+                let prop = &name[crate::engine::FN_SET.len()..];
+                Err(if args.len() != 1 {
+                    ERR::ErrorParsing(
+                    crate::ParseErrorType::MalformedIndexExpr(format!(
+                        "System error: {} argument(s) found for property getter '{prop}' (should be 1)",
+                        args.len()
+                    )),
                     pos,
                 )
-                .into())
+                .into()
+                } else {
+                    let t0 = self.map_type_name(args[0].type_name());
+                    ERR::ErrorDotExpr(
+                        format!(
+                        "Unknown property '{prop}' - a getter is not registered for type '{t0}'"
+                    ),
+                        pos,
+                    )
+                    .into()
+                })
             }
 
             // Setter function not found?
             #[cfg(not(feature = "no_object"))]
             _ if name.starts_with(crate::engine::FN_SET) => {
-                debug_assert_eq!(args.len(), 2);
-
                 let prop = &name[crate::engine::FN_SET.len()..];
-                let t0 = self.map_type_name(args[0].type_name());
-                let t1 = self.map_type_name(args[1].type_name());
-
-                Err(ERR::ErrorDotExpr(
+                Err(if args.len() != 2 {
+                    ERR::ErrorParsing(
+                    crate::ParseErrorType::MalformedIndexExpr(format!(
+                        "System error: {} argument(s) found for property setter '{prop}' (should be 2)",
+                        args.len()
+                    )),
+                    pos,
+                )
+                .into()
+                } else {
+                    let t0 = self.map_type_name(args[0].type_name());
+                    let t1 = self.map_type_name(args[1].type_name());
+                    ERR::ErrorDotExpr(
                     format!(
                         "No writable property '{prop}' - a setter is not registered for type '{t0}' to handle '{t1}'"
                     ),
                     pos,
                 )
-                .into())
+                .into()
+                })
             }
 
             // Raise error

src/grain/compile/mod.rs

@@ -405,6 +405,39 @@ impl Lowering {
             _ => return false,
         };
 
+        // Evaluate the assignment value first, so the chain can read it back
+        // after the lvalue steps have been resolved.
+
+        // The chain is a single expression, so the value is evaluated before
+        // the root and steps.
+
+        let rewind_mark = self.mark();
+        let unwind_depth = self.slots.depth();
+
+        let value_slot = if let Some(value) = value {
+            if self.slots.is_full() {
+                return false;
+            }
+
+            let value_name = ImmutableString::from("$CHAIN_SET_VALUE$");
+            let value_name_index = self.push_name(value_name.clone());
+            let value_slot = self.slots.declare(value_name);
+
+            // First evaluate the assigned value first, stash it so the chain
+            // can read it back after the lvalue steps have been resolved.
+            self.emit(Op::Unit);
+            self.emit(Op::DeclareLocal {
+                name: value_name_index,
+                is_const: false,
+            });
+            self.expression(value);
+            self.emit(Op::StoreLocal(value_slot));
+
+            Some(value_slot)
+        } else {
+            None
+        };
+
         // Index values and method arguments are evaluated first, in step
         // order, exactly as Rhai collects them before walking
         // (`eval/chaining.rs:568`). Evaluating one partway down would need the
@@ -434,9 +467,17 @@ impl Lowering {
                 }
                 ChainStep::Method(call, pos) => {
                     if !self.is_lowerable_call(call) {
+                        if value_slot.is_some() {
+                            self.rewind(rewind_mark);
+                            self.slots.unwind_to(unwind_depth);
+                        }
                         return false;
                     }
                     let Ok(argc) = u8::try_from(call.args.len()) else {
+                        if value_slot.is_some() {
+                            self.rewind(rewind_mark);
+                            self.slots.unwind_to(unwind_depth);
+                        }
                         return false;
                     };
                     let first = operands;
@@ -461,10 +502,8 @@ impl Lowering {
             self.expression(root);
         }
 
-        // The value being assigned goes on last, above everything, so the
-        // walk can take what it needs before it borrows the container.
-        if let Some(value) = value {
-            self.expression(value);
+        if let Some(value_slot) = value_slot {
+            self.emit(Op::LoadLocal(value_slot));
         }
 
         let index = self.push_chain(Chain {
@@ -474,6 +513,7 @@ impl Lowering {
             operands,
         });
         self.emit_at(Op::Chain(index), expr.position());
+        self.unwind_to(unwind_depth);
         true
     }