From 2bcc51cc457b7f321ee25a4cfdcd892e9b96af9d Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 30 May 2020 10:27:48 +0800 Subject: [PATCH] Fix bug in index expressions. --- src/api.rs | 3 +- src/engine.rs | 151 ++++++++++++++++++------------------------------ src/optimize.rs | 8 ++- src/parser.rs | 36 +++++++++++- tests/string.rs | 7 +-- 5 files changed, 98 insertions(+), 107 deletions(-) diff --git a/src/api.rs b/src/api.rs index 89e045c1..8a30a480 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1051,8 +1051,7 @@ impl Engine { let mut state = State::new(); let args = args.as_mut(); - let result = - self.call_script_fn(Some(scope), &mut state, &lib, name, fn_def, args, pos, 0)?; + let result = self.call_script_fn(scope, &mut state, &lib, name, fn_def, args, pos, 0)?; let return_type = self.map_type_name(result.type_name()); diff --git a/src/engine.rs b/src/engine.rs index 433cd77e..2c761c7e 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -432,11 +432,11 @@ fn default_print(s: &str) { } /// Search for a variable within the scope -fn search_scope<'a>( - scope: &'a mut Scope, +fn search_scope<'s, 'a>( + scope: &'s mut Scope, state: &mut State, expr: &'a Expr, -) -> Result<(&'a mut Dynamic, &'a str, ScopeEntryType, Position), Box> { +) -> Result<(&'s mut Dynamic, &'a str, ScopeEntryType, Position), Box> { let ((name, pos), modules, hash_var, index) = match expr { Expr::Variable(x) => x.as_ref(), _ => unreachable!(), @@ -592,7 +592,7 @@ impl Engine { /// **DO NOT** reuse the argument values unless for the first `&mut` argument - all others are silently replaced by `()`! pub(crate) fn call_fn_raw( &self, - scope: Option<&mut Scope>, + scope: &mut Scope, state: &mut State, lib: &FunctionsLib, fn_name: &str, @@ -774,7 +774,7 @@ impl Engine { /// **DO NOT** reuse the argument values unless for the first `&mut` argument - all others are silently replaced by `()`! pub(crate) fn call_script_fn( &self, - scope: Option<&mut Scope>, + scope: &mut Scope, state: &mut State, lib: &FunctionsLib, fn_name: &str, @@ -786,88 +786,42 @@ impl Engine { let orig_scope_level = state.scope_level; state.scope_level += 1; - match scope { - // Extern scope passed in which is not empty - Some(scope) if !scope.is_empty() => { - let scope_len = scope.len(); + let scope_len = scope.len(); - // Put arguments into scope as variables - // Actually consume the arguments instead of cloning them - scope.extend( - fn_def - .params - .iter() - .zip(args.iter_mut().map(|v| mem::take(*v))) - .map(|(name, value)| { - let var_name = unsafe_cast_var_name_to_lifetime(name.as_str(), state); - (var_name, ScopeEntryType::Normal, value) - }), - ); + // Put arguments into scope as variables + // Actually consume the arguments instead of cloning them + scope.extend( + fn_def + .params + .iter() + .zip(args.iter_mut().map(|v| mem::take(*v))) + .map(|(name, value)| { + let var_name = unsafe_cast_var_name_to_lifetime(name.as_str(), state); + (var_name, ScopeEntryType::Normal, value) + }), + ); - // Evaluate the function at one higher level of call depth - let result = self - .eval_stmt(scope, state, lib, &fn_def.body, level + 1) - .or_else(|err| match *err { - // Convert return statement to return value - EvalAltResult::Return(x, _) => Ok(x), - EvalAltResult::ErrorInFunctionCall(name, err, _) => { - Err(Box::new(EvalAltResult::ErrorInFunctionCall( - format!("{} > {}", fn_name, name), - err, - pos, - ))) - } - _ => Err(Box::new(EvalAltResult::ErrorInFunctionCall( - fn_name.to_string(), - err, - pos, - ))), - }); + // Evaluate the function at one higher level of call depth + let result = self + .eval_stmt(scope, state, lib, &fn_def.body, level + 1) + .or_else(|err| match *err { + // Convert return statement to return value + EvalAltResult::Return(x, _) => Ok(x), + EvalAltResult::ErrorInFunctionCall(name, err, _) => Err(Box::new( + EvalAltResult::ErrorInFunctionCall(format!("{} > {}", fn_name, name), err, pos), + )), + _ => Err(Box::new(EvalAltResult::ErrorInFunctionCall( + fn_name.to_string(), + err, + pos, + ))), + }); - // Remove all local variables - scope.rewind(scope_len); - state.scope_level = orig_scope_level; + // Remove all local variables + scope.rewind(scope_len); + state.scope_level = orig_scope_level; - return result; - } - // No new scope - create internal scope - _ => { - let mut scope = Scope::new(); - - // Put arguments into scope as variables - // Actually consume the arguments instead of cloning them - scope.extend( - fn_def - .params - .iter() - .zip(args.iter_mut().map(|v| mem::take(*v))) - .map(|(name, value)| (name, ScopeEntryType::Normal, value)), - ); - - // Evaluate the function at one higher level of call depth - let result = self - .eval_stmt(&mut scope, state, lib, &fn_def.body, level + 1) - .or_else(|err| match *err { - // Convert return statement to return value - EvalAltResult::Return(x, _) => Ok(x), - EvalAltResult::ErrorInFunctionCall(name, err, _) => { - Err(Box::new(EvalAltResult::ErrorInFunctionCall( - format!("{} > {}", fn_name, name), - err, - pos, - ))) - } - _ => Err(Box::new(EvalAltResult::ErrorInFunctionCall( - fn_name.to_string(), - err, - pos, - ))), - }); - - state.scope_level = orig_scope_level; - return result; - } - } + result } // Has a system function an override? @@ -925,9 +879,12 @@ impl Engine { } // Normal function call - _ => self.call_fn_raw( - None, state, lib, fn_name, hashes, args, is_ref, def_val, pos, level, - ), + _ => { + let mut scope = Scope::new(); + self.call_fn_raw( + &mut scope, state, lib, fn_name, hashes, args, is_ref, def_val, pos, level, + ) + } } } @@ -1252,14 +1209,16 @@ impl Engine { Expr::FnCall(_) => unreachable!(), Expr::Property(_) => idx_values.push(()), // Store a placeholder - no need to copy the property name Expr::Index(x) | Expr::Dot(x) => { + let (lhs, rhs, _) = x.as_ref(); + // Evaluate in left-to-right order - let lhs_val = match x.0 { + let lhs_val = match lhs { Expr::Property(_) => Default::default(), // Store a placeholder in case of a property - _ => self.eval_expr(scope, state, lib, &x.0, level)?, + _ => self.eval_expr(scope, state, lib, lhs, level)?, }; // Push in reverse order - self.eval_indexed_chain(scope, state, lib, &x.1, idx_values, size, level)?; + self.eval_indexed_chain(scope, state, lib, rhs, idx_values, size, level)?; idx_values.push(lhs_val); } @@ -1380,6 +1339,7 @@ impl Engine { Dynamic(Union::Array(mut rhs_value)) => { let op = "=="; let def_value = false.into(); + let mut scope = Scope::new(); // Call the `==` operator to compare each value for value in rhs_value.iter_mut() { @@ -1394,7 +1354,7 @@ impl Engine { ); let (r, _) = self.call_fn_raw( - None, state, lib, op, hashes, args, false, def_value, pos, level, + &mut scope, state, lib, op, hashes, args, false, def_value, pos, level, )?; if r.as_bool().unwrap_or(false) { return Ok(true.into()); @@ -1432,6 +1392,8 @@ impl Engine { self.inc_operations(state, expr.position())?; match expr { + Expr::Expr(x) => self.eval_expr(scope, state, lib, x.as_ref(), level), + Expr::IntegerConstant(x) => Ok(x.0.into()), #[cfg(not(feature = "no_float"))] Expr::FloatConstant(x) => Ok(x.0.into()), @@ -1710,9 +1672,8 @@ impl Engine { Ok(x) if x.is_script() => { let args = args.as_mut(); let fn_def = x.get_fn_def(); - let result = - self.call_script_fn(None, state, lib, name, fn_def, args, *pos, level)?; - Ok(result) + let mut scope = Scope::new(); + self.call_script_fn(&mut scope, state, lib, name, fn_def, args, *pos, level) } Ok(x) => x.get_native_fn()(args.as_mut()).map_err(|err| err.new_position(*pos)), Err(err) @@ -1772,9 +1733,9 @@ impl Engine { } /// Evaluate a statement - pub(crate) fn eval_stmt<'s>( + pub(crate) fn eval_stmt( &self, - scope: &mut Scope<'s>, + scope: &mut Scope, state: &mut State, lib: &FunctionsLib, stmt: &Stmt, diff --git a/src/optimize.rs b/src/optimize.rs index dedd1135..f8b0d1bb 100644 --- a/src/optimize.rs +++ b/src/optimize.rs @@ -122,7 +122,7 @@ fn call_fn_with_constant_arguments( state .engine .call_fn_raw( - None, + &mut Scope::new(), &mut Default::default(), state.lib, fn_name, @@ -138,7 +138,7 @@ fn call_fn_with_constant_arguments( } /// Optimize a statement. -fn optimize_stmt<'a>(stmt: Stmt, state: &mut State<'a>, preserve_result: bool) -> Stmt { +fn optimize_stmt(stmt: Stmt, state: &mut State, preserve_result: bool) -> Stmt { match stmt { // if expr { Noop } Stmt::IfThenElse(x) if matches!(x.1, Stmt::Noop(_)) => { @@ -359,11 +359,13 @@ fn optimize_stmt<'a>(stmt: Stmt, state: &mut State<'a>, preserve_result: bool) - } /// Optimize an expression. -fn optimize_expr<'a>(expr: Expr, state: &mut State<'a>) -> Expr { +fn optimize_expr(expr: Expr, state: &mut State) -> Expr { // These keywords are handled specially const DONT_EVAL_KEYWORDS: [&str; 3] = [KEYWORD_PRINT, KEYWORD_DEBUG, KEYWORD_EVAL]; match expr { + // expr - do not promote because there is a reason it is wrapped in an `Expr::Expr` + Expr::Expr(x) => Expr::Expr(Box::new(optimize_expr(*x, state))), // ( stmt ) Expr::Stmt(x) => match optimize_stmt(x.0, state, true) { // ( Noop ) -> () diff --git a/src/parser.rs b/src/parser.rs index 24e6f76e..7a2ad059 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -23,6 +23,7 @@ use crate::stdlib::{ collections::HashMap, format, iter::{empty, Peekable}, + mem, num::NonZeroUsize, ops::{Add, Deref, DerefMut}, string::{String, ToString}, @@ -391,6 +392,8 @@ pub enum Expr { Property(Box<((String, String, String), Position)>), /// { stmt } Stmt(Box<(Stmt, Position)>), + /// Wrapped expression - should not be optimized away. + Expr(Box), /// func(expr, ... ) - ((function name, native_only, position), optional modules, hash, arguments, optional default value) /// Use `Cow<'static, str>` because a lot of operators (e.g. `==`, `>=`) are implemented as function calls /// and the function names are predictable, so no need to allocate a new `String`. @@ -441,6 +444,8 @@ impl Expr { /// Panics when the expression is not constant. pub fn get_constant_value(&self) -> Dynamic { match self { + Self::Expr(x) => x.get_constant_value(), + Self::IntegerConstant(x) => x.0.into(), #[cfg(not(feature = "no_float"))] Self::FloatConstant(x) => x.0.into(), @@ -475,6 +480,8 @@ impl Expr { /// Panics when the expression is not constant. pub fn get_constant_str(&self) -> String { match self { + Self::Expr(x) => x.get_constant_str(), + #[cfg(not(feature = "no_float"))] Self::FloatConstant(x) => x.0.to_string(), @@ -494,6 +501,8 @@ impl Expr { /// Get the `Position` of the expression. pub fn position(&self) -> Position { match self { + Self::Expr(x) => x.position(), + #[cfg(not(feature = "no_float"))] Self::FloatConstant(x) => x.1, @@ -519,6 +528,11 @@ impl Expr { /// Override the `Position` of the expression. pub(crate) fn set_position(mut self, new_pos: Position) -> Self { match &mut self { + Self::Expr(ref mut x) => { + let expr = mem::take(x); + *x = Box::new(expr.set_position(new_pos)); + } + #[cfg(not(feature = "no_float"))] Self::FloatConstant(x) => x.1 = new_pos, @@ -550,6 +564,8 @@ impl Expr { /// A pure expression has no side effects. pub fn is_pure(&self) -> bool { match self { + Self::Expr(x) => x.is_pure(), + Self::Array(x) => x.0.iter().all(Self::is_pure), Self::Index(x) | Self::And(x) | Self::Or(x) | Self::In(x) => { @@ -568,6 +584,8 @@ impl Expr { /// Is the expression a constant? pub fn is_constant(&self) -> bool { match self { + Self::Expr(x) => x.is_constant(), + #[cfg(not(feature = "no_float"))] Self::FloatConstant(_) => true, @@ -598,6 +616,8 @@ impl Expr { /// Is a particular token allowed as a postfix operator to this expression? pub fn is_valid_postfix(&self, token: &Token) -> bool { match self { + Self::Expr(x) => x.is_valid_postfix(token), + #[cfg(not(feature = "no_float"))] Self::FloatConstant(_) => false, @@ -996,7 +1016,7 @@ fn parse_index_chain<'a>( (Token::LeftBracket, _) => { let idx_pos = eat_token(input, Token::LeftBracket); // Recursively parse the indexing chain, right-binding each - let idx = parse_index_chain( + let idx_expr = parse_index_chain( input, state, idx_expr, @@ -1005,10 +1025,20 @@ fn parse_index_chain<'a>( allow_stmt_expr, )?; // Indexing binds to right - Ok(Expr::Index(Box::new((lhs, idx, pos)))) + Ok(Expr::Index(Box::new((lhs, idx_expr, pos)))) } // Otherwise terminate the indexing chain - _ => Ok(Expr::Index(Box::new((lhs, idx_expr, pos)))), + _ => { + match idx_expr { + // Terminate with an `Expr::Expr` wrapper to prevent the last index expression + // inside brackets to be mis-parsed as another level of indexing, or a + // dot expression/function call to be mis-parsed as following the indexing chain. + Expr::Index(_) | Expr::Dot(_) | Expr::FnCall(_) => Ok(Expr::Index( + Box::new((lhs, Expr::Expr(Box::new(idx_expr)), pos)), + )), + _ => Ok(Expr::Index(Box::new((lhs, idx_expr, pos)))), + } + } } } (Token::LexError(err), pos) => return Err(PERR::BadInput(err.to_string()).into_err(*pos)), diff --git a/tests/string.rs b/tests/string.rs index 981d19eb..b868f050 100644 --- a/tests/string.rs +++ b/tests/string.rs @@ -20,21 +20,20 @@ fn test_string() -> Result<(), Box> { assert!(engine.eval::(r#"let y = "hello, world!"; 'w' in y"#)?); assert!(!engine.eval::(r#"let y = "hello, world!"; "hey" in y"#)?); - #[cfg(not(feature = "no_stdlib"))] assert_eq!(engine.eval::(r#""foo" + 123"#)?, "foo123"); - #[cfg(not(feature = "no_stdlib"))] #[cfg(not(feature = "no_object"))] assert_eq!(engine.eval::("(42).to_string()")?, "42"); + #[cfg(not(feature = "no_object"))] + assert_eq!(engine.eval::(r#"let y = "hello"; y[y.len-1]"#)?, 'o'); + #[cfg(not(feature = "no_float"))] - #[cfg(not(feature = "no_stdlib"))] assert_eq!(engine.eval::(r#""foo" + 123.4556"#)?, "foo123.4556"); Ok(()) } -#[cfg(not(feature = "no_stdlib"))] #[cfg(not(feature = "no_object"))] #[test] fn test_string_substring() -> Result<(), Box> {