From 26bdc8ba0875a508866aa0f31062572c9f0ead4f Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 14 Mar 2020 19:46:44 +0800 Subject: [PATCH] FIX - fixes panic when constant array is assigned to. Refine README section on constants. --- README.md | 93 ++++++++++++++++++++++++++++++----------- src/engine.rs | 101 ++++++++++++++++++++++++++++++--------------- src/parser.rs | 14 ++++--- tests/arrays.rs | 14 +++++-- tests/constants.rs | 8 +++- 5 files changed, 161 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 6cbb3280..db33e369 100644 --- a/README.md +++ b/README.md @@ -65,15 +65,15 @@ Examples A number of examples can be found in the `examples` folder: -| Example | Description | -| -------------------------- | ------------------------------------------------------------------------- | -| `arrays_and_structs` | demonstrates registering a new type to Rhai and the usage of arrays on it | -| `custom_types_and_methods` | shows how to register a type and methods for it | -| `hello` | simple example that evaluates an expression and prints the result | -| `reuse_scope` | evaluates two pieces of code in separate runs, but using a common scope | -| `rhai_runner` | runs each filename passed to it as a Rhai script | -| `simple_fn` | shows how to register a Rust function to a Rhai engine | -| `repl` | a simple REPL, interactively evaluate statements from stdin | +| Example | Description | +| -------------------------- | --------------------------------------------------------------------------- | +| `arrays_and_structs` | demonstrates registering a new type to Rhai and the usage of arrays on it | +| `custom_types_and_methods` | shows how to register a type and methods for it | +| `hello` | simple example that evaluates an expression and prints the result | +| `reuse_scope` | evaluates two pieces of code in separate runs, but using a common [`Scope`] | +| `rhai_runner` | runs each filename passed to it as a Rhai script | +| `simple_fn` | shows how to register a Rust function to a Rhai [`Engine`] | +| `repl` | a simple REPL, interactively evaluate statements from stdin | Examples can be run with the following command: @@ -294,7 +294,7 @@ fn main() -> Result<(), EvalAltResult> } ``` -To return a `Dynamic` value, simply `Box` it and return it. +To return a [`Dynamic`] value, simply `Box` it and return it. ```rust fn decide(yes_no: bool) -> Dynamic { @@ -421,7 +421,7 @@ fn main() -> Result<(), EvalAltResult> } ``` -First, for each type we use with the engine, we need to be able to Clone. This allows the engine to pass by value and still keep its own state. +All custom types must implement `Clone`. This allows the [`Engine`] to pass by value. ```rust #[derive(Clone)] @@ -430,7 +430,7 @@ struct TestStruct { } ``` -Next, we create a few methods that we'll later use in our scripts. Notice that we register our custom type with the engine. +Next, we create a few methods that we'll later use in our scripts. Notice that we register our custom type with the [`Engine`]. ```rust impl TestStruct { @@ -448,9 +448,9 @@ let mut engine = Engine::new(); engine.register_type::(); ``` -To use methods and functions with the engine, we need to register them. There are some convenience functions to help with this. Below I register update and new with the engine. +To use methods and functions with the [`Engine`], we need to register them. There are some convenience functions to help with this. Below I register update and new with the [`Engine`]. -*Note: the engine follows the convention that methods use a &mut first parameter so that invoking methods can update the value in memory.* +*Note: [`Engine`] follows the convention that methods use a `&mut` first parameter so that invoking methods can update the value in memory.* ```rust engine.register_fn("update", TestStruct::update); @@ -530,7 +530,7 @@ println!("result: {}", result); Initializing and maintaining state --------------------------------- -By default, Rhai treats each engine invocation as a fresh one, persisting only the functions that have been defined but no top-level state. This gives each one a fairly clean starting place. Sometimes, though, you want to continue using the same top-level state from one invocation to the next. +By default, Rhai treats each [`Engine`] invocation as a fresh one, persisting only the functions that have been defined but no top-level state. This gives each one a fairly clean starting place. Sometimes, though, you want to continue using the same top-level state from one invocation to the next. In this example, we first create a state with a few initialized variables, then thread the same state through multiple invocations: @@ -604,10 +604,17 @@ Constants can be defined and are immutable. Constants follow the same naming ru ```rust const x = 42; -print(x * 2); // prints 84 -x = 123; // <- syntax error - cannot assign to constant +print(x * 2); // prints 84 +x = 123; // <- syntax error - cannot assign to constant ``` +Constants must be assigned a _value_ not an expression. + +```rust +const x = 40 + 2; // <- syntax error - cannot assign expression to constant +``` + + Numbers ------- @@ -997,7 +1004,7 @@ fn add(x, y) { print(add(2, 3)); ``` -Functions defined in script always take `Dynamic` parameters (i.e. the parameter can be of any type). +Functions defined in script always take [`Dynamic`] parameters (i.e. the parameter can be of any type). It is important to remember that all parameters are passed by _value_, so all functions are _pure_ (i.e. they never modify their parameters). Any update to an argument will **not** be reflected back to the caller. This can introduce subtle bugs, if you are not careful. @@ -1029,7 +1036,7 @@ fn do_addition(x) { } ``` -Functions can be _overloaded_ based on the number of parameters (but not parameter types, since all parameters are `Dynamic`). +Functions can be _overloaded_ based on the number of parameters (but not parameter types, since all parameters are [`Dynamic`]). New definitions of the same name and number of parameters overwrite previous definitions. ```rust @@ -1122,24 +1129,56 @@ The above script optimizes to: Constants propagation is used to remove dead code: ```rust -const abc = true; -if abc || some_work() { print("done!"); } // 'abc' is constant so it is replaced by 'true'... -if true || some_work() { print("done!"); } // since '||' short-circuits, 'some_work' is never called because the LHS is 'true' +const ABC = true; +if ABC || some_work() { print("done!"); } // 'ABC' is constant so it is replaced by 'true'... +if true || some_work() { print("done!"); } // since '||' short-circuits, 'some_work' is never called if true { print("done!"); } // <-- the line above is equivalent to this print("done!"); // <-- the line above is further simplified to this // because the condition is always true ``` These are quite effective for template-based machine-generated scripts where certain constant values are spliced into the script text in order to turn on/off certain sections. +For fixed script texts, the constant values can be provided in a user-defined `Scope` object to the `Engine` for use in compilation and evaluation. Beware, however, that most operators are actually function calls, and those functions can be overridden, so they are not optimized away: ```rust -if 1 == 1 { ... } // '1==1' is NOT optimized away because you can define - // your own '==' function to override the built-in default! +const DECISION = 1; + +if DECISION == 1 { // NOT optimized away because you can define + : // your own '==' function to override the built-in default! + : +} else if DECISION == 2 { // same here, NOT optimized away + : +} else if DECISION == 3 { // same here, NOT optimized away + : +} else { + : +} ``` -### Here be dragons! +So, instead, do this: + +```rust +const DECISION_1 = true; +const DECISION_2 = false; +const DECISION_3 = false; + +if DECISION_1 { + : // this branch is kept +} else if DECISION_2 { + : // this branch is eliminated +} else if DECISION_3 { + : // this branch is eliminated +} else { + : // this branch is eliminated +} +``` + +In general, boolean constants are most effective if you want the optimizer to automatically prune large `if`-`else` branches because they do not depend on operators. + +Here be dragons! +---------------- Some optimizations can be quite aggressive and can alter subtle semantics of the script. For example: @@ -1191,3 +1230,7 @@ engine.set_optimization(false); // turn off the optimizer [`no_function`]: #optional-features [`only_i32`]: #optional-features [`only_i64`]: #optional-features + +[`Engine`]: #hello-world +[`Scope`]: #initializing-and-maintaining-state +[`Dynamic`]: #values-and-types diff --git a/src/engine.rs b/src/engine.rs index 86c9a0d9..36d7fc71 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -372,16 +372,26 @@ impl Engine<'_> { let val = self.get_dot_val_helper(scope, target.as_mut(), dot_rhs); // In case the expression mutated `target`, we need to update it back into the scope because it is cloned. - if let Some((id, src_idx)) = src { - Self::update_indexed_var_in_scope( - src_type, - scope, - id, - src_idx, - idx, - target, - idx_lhs.position(), - )?; + if let Some((id, var_type, src_idx)) = src { + match var_type { + VariableType::Constant => { + return Err(EvalAltResult::ErrorAssignmentToConstant( + id.to_string(), + idx_lhs.position(), + )); + } + VariableType::Normal => { + Self::update_indexed_var_in_scope( + src_type, + scope, + id, + src_idx, + idx, + target, + dot_rhs.position(), + )?; + } + } } val @@ -477,7 +487,15 @@ impl Engine<'_> { lhs: &'a Expr, idx_expr: &Expr, idx_pos: Position, - ) -> Result<(IndexSourceType, Option<(&'a str, usize)>, usize, Dynamic), EvalAltResult> { + ) -> Result< + ( + IndexSourceType, + Option<(&'a str, VariableType, usize)>, + usize, + Dynamic, + ), + EvalAltResult, + > { let idx = self.eval_index_value(scope, idx_expr)?; match lhs { @@ -488,8 +506,13 @@ impl Engine<'_> { |val| self.get_indexed_value(&val, idx, idx_expr.position(), idx_pos), lhs.position(), ) - .map(|(src_idx, _, (val, src_type))| { - (src_type, Some((id.as_str(), src_idx)), idx as usize, val) + .map(|(src_idx, var_type, (val, src_type))| { + ( + src_type, + Some((id.as_str(), var_type, src_idx)), + idx as usize, + val, + ) }), // (expr)[idx_expr] @@ -739,16 +762,20 @@ impl Engine<'_> { self.set_dot_val_helper(scope, target.as_mut(), dot_rhs, new_val, val_pos); // In case the expression mutated `target`, we need to update it back into the scope because it is cloned. - if let Some((id, src_idx)) = src { - Self::update_indexed_var_in_scope( - src_type, - scope, - id, - src_idx, - idx, - target, - lhs.position(), - )?; + if let Some((id, var_type, src_idx)) = src { + match var_type { + VariableType::Constant => { + return Err(EvalAltResult::ErrorAssignmentToConstant( + id.to_string(), + lhs.position(), + )); + } + VariableType::Normal => { + Self::update_indexed_var_in_scope( + src_type, scope, id, src_idx, idx, target, val_pos, + )?; + } + } } val @@ -811,16 +838,24 @@ impl Engine<'_> { let (src_type, src, idx, _) = self.eval_index_expr(scope, idx_lhs, idx_expr, *idx_pos)?; - if let Some((id, src_idx)) = src { - Ok(Self::update_indexed_var_in_scope( - src_type, - scope, - &id, - src_idx, - idx, - rhs_val, - rhs.position(), - )?) + if let Some((id, var_type, src_idx)) = src { + match var_type { + VariableType::Constant => { + return Err(EvalAltResult::ErrorAssignmentToConstant( + id.to_string(), + idx_lhs.position(), + )); + } + VariableType::Normal => Ok(Self::update_indexed_var_in_scope( + src_type, + scope, + &id, + src_idx, + idx, + rhs_val, + rhs.position(), + )?), + } } else { Err(EvalAltResult::ErrorAssignmentToUnknownLHS( idx_lhs.position(), diff --git a/src/parser.rs b/src/parser.rs index 6b48e3f5..e660d0c6 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -343,6 +343,8 @@ impl Expr { | Expr::False(_) | Expr::Unit(_) => true, + Expr::Array(expressions, _) => expressions.iter().all(Expr::is_constant), + #[cfg(not(feature = "no_float"))] Expr::FloatConstant(_, _) => true, @@ -1635,12 +1637,12 @@ fn parse_assignment(lhs: Expr, rhs: Expr, pos: Position) -> Result valid_assignment_chain(idx_lhs, true), - - #[cfg(not(feature = "no_index"))] - Expr::Index(idx_lhs, _, _) if !is_top => Some(ParseError::new( - ParseErrorType::AssignmentToInvalidLHS, - idx_lhs.position(), + Expr::Index(idx_lhs, _, pos) => Some(ParseError::new( + match idx_lhs.as_ref() { + Expr::Index(_, _, _) => ParseErrorType::AssignmentToCopy, + _ => ParseErrorType::AssignmentToInvalidLHS, + }, + *pos, )), Expr::Dot(dot_lhs, dot_rhs, _) => match dot_lhs.as_ref() { diff --git a/tests/arrays.rs b/tests/arrays.rs index 4bee090e..d8c6a38d 100644 --- a/tests/arrays.rs +++ b/tests/arrays.rs @@ -7,6 +7,10 @@ fn test_arrays() -> Result<(), EvalAltResult> { assert_eq!(engine.eval::("let x = [1, 2, 3]; x[1]")?, 2); assert_eq!(engine.eval::("let y = [1, 2, 3]; y[1] = 5; y[1]")?, 5); + assert_eq!( + engine.eval::(r#"let y = [1, [ 42, 88, "93" ], 3]; y[1][2][1]"#)?, + '3' + ); Ok(()) } @@ -48,10 +52,12 @@ fn test_array_with_structs() -> Result<(), EvalAltResult> { assert_eq!( engine.eval::( - "let a = [new_ts()]; \ - a[0].x = 100; \ - a[0].update(); \ - a[0].x", + r" + let a = [new_ts()]; + a[0].x = 100; + a[0].update(); + a[0].x + " )?, 1100 ); diff --git a/tests/constants.rs b/tests/constants.rs index e81f7ce4..ff668797 100644 --- a/tests/constants.rs +++ b/tests/constants.rs @@ -6,7 +6,13 @@ fn test_constant() -> Result<(), EvalAltResult> { assert_eq!(engine.eval::("const x = 123; x")?, 123); - match engine.eval::("const x = 123; x = 42; x") { + match engine.eval::("const x = 123; x = 42;") { + Err(EvalAltResult::ErrorAssignmentToConstant(var, _)) if var == "x" => (), + Err(err) => return Err(err), + Ok(_) => panic!("expecting compilation error"), + } + + match engine.eval::("const x = [1, 2, 3, 4, 5]; x[2] = 42;") { Err(EvalAltResult::ErrorAssignmentToConstant(var, _)) if var == "x" => (), Err(err) => return Err(err), Ok(_) => panic!("expecting compilation error"),