From 85dcd6e7541e916088b7b2868fab0eadc260bf83 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 28 Sep 2021 15:59:46 +0800 Subject: [PATCH 1/5] Fix bug with changing property of value obtained via index getter. --- CHANGELOG.md | 1 + src/engine.rs | 70 +++++++++++++++++++++++++++++++++++++++--------- tests/get_set.rs | 35 ++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dc0644a..af79c972 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ 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 ------------ diff --git a/src/engine.rs b/src/engine.rs index c052f98d..d0847bac 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -335,6 +335,19 @@ impl ChainArgument { _ => None, } } + /// Return the [position][Position]. + #[inline(always)] + #[must_use] + 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 +1266,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 +1276,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 +1352,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, )?; } @@ -2344,7 +2390,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 = diff --git a/tests/get_set.rs b/tests/get_set.rs index cdaa1e68..5febe240 100644 --- a/tests/get_set.rs +++ b/tests/get_set.rs @@ -119,7 +119,17 @@ fn test_get_set_chain_with_write_back() -> Result<(), Box> { engine.register_get_set("x", TestChild::get_x, TestChild::set_x); engine.register_get_set("child", TestParent::get_child, TestParent::set_child); + #[cfg(not(feature = "no_index"))] + engine.register_indexer_get_set( + |parent: &mut TestParent, _: INT| parent.child.clone(), + |parent: &mut TestParent, n: INT, mut new_child: TestChild| { + new_child.x *= n; + parent.child = new_child; + }, + ); + engine.register_fn("new_tp", TestParent::new); + engine.register_fn("new_tc", TestChild::new); assert_eq!(engine.eval::("let a = new_tp(); a.child.x")?, 1); assert_eq!( @@ -132,6 +142,18 @@ fn test_get_set_chain_with_write_back() -> Result<(), Box> { "TestParent" ); + #[cfg(not(feature = "no_index"))] + assert_eq!( + engine.eval::("let a = new_tp(); let c = new_tc(); c.x = 123; a[2] = c; a.child.x")?, + 246 + ); + + #[cfg(not(feature = "no_index"))] + assert_eq!( + engine.eval::("let a = new_tp(); a[2].x = 42; a.child.x")?, + 84 + ); + Ok(()) } @@ -202,13 +224,26 @@ fn test_get_set_chain_without_write_back() -> Result<(), Box> { "inner", |t: &mut Outer| t.inner.clone(), |_: &mut Outer, new: Inner| panic!("Outer::inner setter called with {:?}", new), + ) + .register_indexer_get_set( + |t: &mut Outer, n: INT| Inner { + value: t.inner.value * n, + }, + |_: &mut Outer, n: INT, new: Inner| { + panic!("Outer::inner index setter called with {} and {:?}", n, new) + }, ); assert_eq!( engine.eval_with_scope::(&mut scope, "outer.inner.value")?, 42 ); + assert_eq!( + engine.eval_with_scope::(&mut scope, "outer[2].value")?, + 84 + ); engine.consume_with_scope(&mut scope, "print(outer.inner.value)")?; + engine.consume_with_scope(&mut scope, "print(outer[0].value)")?; Ok(()) } From 07acc6e1250dca8466e5ed82e8654d7165f9906e Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 27 Sep 2021 11:24:48 +0800 Subject: [PATCH 2/5] Fix test output. --- codegen/ui_tests/rhai_mod_unknown_type.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codegen/ui_tests/rhai_mod_unknown_type.stderr b/codegen/ui_tests/rhai_mod_unknown_type.stderr index 9ea2728f..5853808f 100644 --- a/codegen/ui_tests/rhai_mod_unknown_type.stderr +++ b/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; + | From b90776911d7ea0f6b17ee37b0b77cb94c7d0e916 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 28 Sep 2021 16:32:31 +0800 Subject: [PATCH 3/5] Fix tests output. --- codegen/ui_tests/rhai_fn_non_clonable_return.stderr | 4 ++-- codegen/ui_tests/rhai_mod_non_clonable_return.stderr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/codegen/ui_tests/rhai_fn_non_clonable_return.stderr b/codegen/ui_tests/rhai_fn_non_clonable_return.stderr index 3dea1ebc..8005ce5b 100644 --- a/codegen/ui_tests/rhai_fn_non_clonable_return.stderr +++ b/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(mut value: T) -> Self { +1044 | pub fn from(mut value: T) -> Self { | ^^^^^ required by this bound in `rhai::Dynamic::from` diff --git a/codegen/ui_tests/rhai_mod_non_clonable_return.stderr b/codegen/ui_tests/rhai_mod_non_clonable_return.stderr index aa542503..dbbe2e0b 100644 --- a/codegen/ui_tests/rhai_mod_non_clonable_return.stderr +++ b/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(mut value: T) -> Self { +1044 | pub fn from(mut value: T) -> Self { | ^^^^^ required by this bound in `rhai::Dynamic::from` From 7ce88873432a1ac4e39afaed082e9eaa70153126 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 28 Sep 2021 16:36:31 +0800 Subject: [PATCH 4/5] Fix no_index build. --- src/engine.rs | 1 + tests/get_set.rs | 23 +++++++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index d0847bac..cd28772f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -338,6 +338,7 @@ impl ChainArgument { /// Return the [position][Position]. #[inline(always)] #[must_use] + #[allow(dead_code)] pub const fn position(&self) -> Position { match self { #[cfg(not(feature = "no_object"))] diff --git a/tests/get_set.rs b/tests/get_set.rs index 5febe240..83dacc0b 100644 --- a/tests/get_set.rs +++ b/tests/get_set.rs @@ -224,25 +224,32 @@ fn test_get_set_chain_without_write_back() -> Result<(), Box> { "inner", |t: &mut Outer| t.inner.clone(), |_: &mut Outer, new: Inner| panic!("Outer::inner setter called with {:?}", new), - ) - .register_indexer_get_set( - |t: &mut Outer, n: INT| Inner { - value: t.inner.value * n, - }, - |_: &mut Outer, n: INT, new: Inner| { - panic!("Outer::inner index setter called with {} and {:?}", n, new) - }, ); + #[cfg(not(feature = "no_index"))] + engine.register_indexer_get_set( + |t: &mut Outer, n: INT| Inner { + value: t.inner.value * n, + }, + |_: &mut Outer, n: INT, new: Inner| { + panic!("Outer::inner index setter called with {} and {:?}", n, new) + }, + ); + assert_eq!( engine.eval_with_scope::(&mut scope, "outer.inner.value")?, 42 ); + + #[cfg(not(feature = "no_index"))] assert_eq!( engine.eval_with_scope::(&mut scope, "outer[2].value")?, 84 ); + engine.consume_with_scope(&mut scope, "print(outer.inner.value)")?; + + #[cfg(not(feature = "no_index"))] engine.consume_with_scope(&mut scope, "print(outer[0].value)")?; Ok(()) From 0cde18d3b56b87568513aac50a033b835d92e8e0 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 28 Sep 2021 16:51:23 +0800 Subject: [PATCH 5/5] Bump version. --- CHANGELOG.md | 4 ++++ Cargo.toml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af79c972..3333861b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ Rhai Release Notes ================== +Version 1.0.7 +============= + + Version 1.0.6 ============= diff --git a/Cargo.toml b/Cargo.toml index d4e7675d..e2e0f151 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = [".", "codegen"] [package] name = "rhai" -version = "1.0.6" +version = "1.0.7" edition = "2018" authors = ["Jonathan Turner", "Lukáš Hozda", "Stephen Chung", "jhwgh1968"] description = "Embedded scripting for Rust"