From 60a933862e089c910482944a6728346bad8d91c2 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 18 Apr 2022 23:12:47 +0800 Subject: [PATCH] Streamline op-assignments. --- src/ast/stmt.rs | 48 +++++++++++++++++++++++++------- src/eval/chaining.rs | 66 ++++++++++++++++++-------------------------- src/eval/expr.rs | 26 ++++++++--------- src/eval/stmt.rs | 39 ++++++++++++++------------ src/eval/target.rs | 2 ++ src/optimizer.rs | 4 +-- src/parser.rs | 27 +++++++++--------- 7 files changed, 116 insertions(+), 96 deletions(-) diff --git a/src/ast/stmt.rs b/src/ast/stmt.rs index 33fbd75a..d855e32b 100644 --- a/src/ast/stmt.rs +++ b/src/ast/stmt.rs @@ -17,6 +17,8 @@ use std::{ /// _(internals)_ An op-assignment operator. /// Exported under the `internals` feature only. +/// +/// This type may hold a straight assignment (i.e. not an op-assignment). #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] pub struct OpAssignment<'a> { /// Hash of the op-assignment call. @@ -27,9 +29,29 @@ pub struct OpAssignment<'a> { pub op_assign: &'a str, /// Underlying operator. pub op: &'a str, + /// [Position] of the op-assignment operator. + pub pos: Position, } impl OpAssignment<'_> { + /// Create a new [`OpAssignment`] that is only a straight assignment. + #[must_use] + #[inline(always)] + pub const fn new_assignment(pos: Position) -> Self { + Self { + hash_op_assign: 0, + hash_op: 0, + op_assign: "=", + op: "=", + pos, + } + } + /// Is this an op-assignment? + #[must_use] + #[inline(always)] + pub const fn is_op_assignment(&self) -> bool { + self.hash_op_assign != 0 || self.hash_op != 0 + } /// Create a new [`OpAssignment`]. /// /// # Panics @@ -37,8 +59,8 @@ impl OpAssignment<'_> { /// Panics if the name is not an op-assignment operator. #[must_use] #[inline(always)] - pub fn new(name: &str) -> Self { - Self::new_from_token(Token::lookup_from_syntax(name).expect("operator")) + pub fn new_op_assignment(name: &str, pos: Position) -> Self { + Self::new_op_assignment_from_token(Token::lookup_from_syntax(name).expect("operator"), pos) } /// Create a new [`OpAssignment`] from a [`Token`]. /// @@ -46,7 +68,7 @@ impl OpAssignment<'_> { /// /// Panics if the token is not an op-assignment operator. #[must_use] - pub fn new_from_token(op: Token) -> Self { + pub fn new_op_assignment_from_token(op: Token, pos: Position) -> Self { let op_raw = op .get_base_op_from_assignment() .expect("op-assignment operator") @@ -56,6 +78,7 @@ impl OpAssignment<'_> { hash_op: calc_fn_hash(op_raw, 2), op_assign: op.literal_syntax(), op: op_raw, + pos, } } /// Create a new [`OpAssignment`] from a base operator. @@ -65,8 +88,11 @@ impl OpAssignment<'_> { /// Panics if the name is not an operator that can be converted into an op-operator. #[must_use] #[inline(always)] - pub fn new_from_base(name: &str) -> Self { - Self::new_from_base_token(Token::lookup_from_syntax(name).expect("operator")) + pub fn new_op_assignment_from_base(name: &str, pos: Position) -> Self { + Self::new_op_assignment_from_base_token( + Token::lookup_from_syntax(name).expect("operator"), + pos, + ) } /// Convert a [`Token`] into a new [`OpAssignment`]. /// @@ -75,8 +101,8 @@ impl OpAssignment<'_> { /// Panics if the token is cannot be converted into an op-assignment operator. #[inline(always)] #[must_use] - pub fn new_from_base_token(op: Token) -> Self { - Self::new_from_token(op.convert_to_op_assignment().expect("operator")) + pub fn new_op_assignment_from_base_token(op: Token, pos: Position) -> Self { + Self::new_op_assignment_from_token(op.convert_to_op_assignment().expect("operator"), pos) } } @@ -375,7 +401,7 @@ pub enum Stmt { /// * [`CONSTANT`][ASTFlags::CONSTANT] = `const` Var(Box<(Ident, Expr, Option)>, ASTFlags, Position), /// expr op`=` expr - Assignment(Box<(Option>, BinaryExpr)>, Position), + Assignment(Box<(OpAssignment<'static>, BinaryExpr)>), /// func `(` expr `,` ... `)` /// /// Note - this is a duplicate of [`Expr::FnCall`] to cover the very common pattern of a single @@ -464,7 +490,6 @@ impl Stmt { match self { Self::Noop(pos) | Self::BreakLoop(.., pos) - | Self::Assignment(.., pos) | Self::FnCall(.., pos) | Self::If(.., pos) | Self::Switch(.., pos) @@ -475,6 +500,8 @@ impl Stmt { | Self::Var(.., pos) | Self::TryCatch(.., pos) => *pos, + Self::Assignment(x) => x.0.pos, + Self::Block(x) => x.position(), Self::Expr(x) => x.start_position(), @@ -493,7 +520,6 @@ impl Stmt { match self { Self::Noop(pos) | Self::BreakLoop(.., pos) - | Self::Assignment(.., pos) | Self::FnCall(.., pos) | Self::If(.., pos) | Self::Switch(.., pos) @@ -504,6 +530,8 @@ impl Stmt { | Self::Var(.., pos) | Self::TryCatch(.., pos) => *pos = new_pos, + Self::Assignment(x) => x.0.pos = new_pos, + Self::Block(x) => x.set_position(new_pos, x.end_position()), Self::Expr(x) => { diff --git a/src/eval/chaining.rs b/src/eval/chaining.rs index ed9d8b9e..22e1d8b5 100644 --- a/src/eval/chaining.rs +++ b/src/eval/chaining.rs @@ -130,10 +130,10 @@ impl Engine { _parent: &Expr, rhs: &Expr, _parent_options: ASTFlags, - idx_values: &mut StaticVec, + idx_values: &mut StaticVec, chain_type: ChainType, level: usize, - new_val: Option<((Dynamic, Position), (Option, Position))>, + new_val: Option<(Dynamic, OpAssignment)>, ) -> RhaiResultOf<(Dynamic, bool)> { let is_ref_mut = target.is_ref(); @@ -200,7 +200,7 @@ impl Engine { #[cfg(feature = "debugging")] self.run_debugger(scope, global, lib, this_ptr, _parent, level)?; - let ((new_val, new_pos), (op_info, op_pos)) = new_val.expect("`Some`"); + let (new_val, op_info) = new_val.expect("`Some`"); let mut idx_val2 = idx_val.clone(); let try_setter = match self.get_indexed_mut( @@ -209,12 +209,10 @@ impl Engine { // Indexed value is not a temp value - update directly Ok(ref mut obj_ptr) => { self.eval_op_assignment( - global, caches, lib, op_info, op_pos, obj_ptr, root, new_val, - level, - ) - .map_err(|err| err.fill_position(new_pos))?; + global, caches, lib, op_info, obj_ptr, root, new_val, level, + )?; #[cfg(not(feature = "unchecked"))] - self.check_data_size(obj_ptr, new_pos)?; + self.check_data_size(obj_ptr, op_info.pos)?; None } // Indexed value cannot be referenced - use indexer @@ -228,7 +226,7 @@ impl Engine { let idx = &mut idx_val2; // Is this an op-assignment? - if op_info.is_some() { + if op_info.is_op_assignment() { let idx = &mut idx.clone(); // Call the index getter to get the current value if let Ok(val) = @@ -237,14 +235,13 @@ impl Engine { let mut res = val.into(); // Run the op-assignment self.eval_op_assignment( - global, caches, lib, op_info, op_pos, &mut res, root, - new_val, level, - ) - .map_err(|err| err.fill_position(new_pos))?; + global, caches, lib, op_info, &mut res, root, new_val, + level, + )?; // Replace new value new_val = res.take_or_clone(); #[cfg(not(feature = "unchecked"))] - self.check_data_size(&new_val, new_pos)?; + self.check_data_size(&new_val, op_info.pos)?; } } @@ -305,19 +302,17 @@ impl Engine { self.run_debugger(scope, global, lib, this_ptr, rhs, level)?; let index = x.2.clone().into(); - let ((new_val, new_pos), (op_info, op_pos)) = new_val.expect("`Some`"); + let (new_val, op_info) = new_val.expect("`Some`"); { let val_target = &mut self.get_indexed_mut( global, caches, lib, target, index, *pos, true, false, level, )?; self.eval_op_assignment( - global, caches, lib, op_info, op_pos, val_target, root, new_val, - level, - ) - .map_err(|err| err.fill_position(new_pos))?; + global, caches, lib, op_info, val_target, root, new_val, level, + )?; } #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.source(), new_pos)?; + self.check_data_size(target.source(), op_info.pos)?; Ok((Dynamic::UNIT, true)) } // {xxx:map}.id @@ -337,9 +332,9 @@ impl Engine { self.run_debugger(scope, global, lib, this_ptr, rhs, level)?; let ((getter, hash_get), (setter, hash_set), name) = x.as_ref(); - let ((mut new_val, new_pos), (op_info, op_pos)) = new_val.expect("`Some`"); + let (mut new_val, op_info) = new_val.expect("`Some`"); - if op_info.is_some() { + if op_info.is_op_assignment() { let hash = crate::ast::FnCallHashes::from_native(*hash_get); let args = &mut [target.as_mut()]; let (mut orig_val, ..) = self @@ -369,10 +364,8 @@ impl Engine { let orig_val = &mut (&mut orig_val).into(); self.eval_op_assignment( - global, caches, lib, op_info, op_pos, orig_val, root, new_val, - level, - ) - .map_err(|err| err.fill_position(new_pos))?; + global, caches, lib, op_info, orig_val, root, new_val, level, + )?; } new_val = orig_val; @@ -620,7 +613,7 @@ impl Engine { this_ptr: &mut Option<&mut Dynamic>, expr: &Expr, level: usize, - new_val: Option<((Dynamic, Position), (Option, Position))>, + new_val: Option<(Dynamic, OpAssignment)>, ) -> RhaiResult { let (crate::ast::BinaryExpr { lhs, rhs }, chain_type, options, op_pos) = match expr { #[cfg(not(feature = "no_index"))] @@ -690,7 +683,7 @@ impl Engine { expr: &Expr, parent_options: ASTFlags, _parent_chain_type: ChainType, - idx_values: &mut StaticVec, + idx_values: &mut StaticVec, size: usize, level: usize, ) -> RhaiResultOf<()> { @@ -717,7 +710,7 @@ impl Engine { }, )?; - idx_values.push(super::ChainArgument::from_fn_call_args(values, pos)); + idx_values.push(ChainArgument::from_fn_call_args(values, pos)); } #[cfg(not(feature = "no_object"))] Expr::MethodCall(..) if _parent_chain_type == ChainType::Dotting => { @@ -726,7 +719,7 @@ impl Engine { #[cfg(not(feature = "no_object"))] Expr::Property(.., pos) if _parent_chain_type == ChainType::Dotting => { - idx_values.push(super::ChainArgument::Property(*pos)) + idx_values.push(ChainArgument::Property(*pos)) } Expr::Property(..) => unreachable!("unexpected Expr::Property for indexing"), @@ -739,7 +732,7 @@ impl Engine { let lhs_arg_val = match lhs { #[cfg(not(feature = "no_object"))] Expr::Property(.., pos) if _parent_chain_type == ChainType::Dotting => { - super::ChainArgument::Property(*pos) + ChainArgument::Property(*pos) } Expr::Property(..) => unreachable!("unexpected Expr::Property for indexing"), @@ -762,7 +755,7 @@ impl Engine { Ok::<_, crate::RhaiError>((values, pos)) }, )?; - super::ChainArgument::from_fn_call_args(values, pos) + ChainArgument::from_fn_call_args(values, pos) } #[cfg(not(feature = "no_object"))] Expr::MethodCall(..) if _parent_chain_type == ChainType::Dotting => { @@ -776,10 +769,7 @@ impl Engine { _ if _parent_chain_type == ChainType::Indexing => self .eval_expr(scope, global, caches, lib, this_ptr, lhs, level) .map(|v| { - super::ChainArgument::from_index_value( - v.flatten(), - lhs.start_position(), - ) + ChainArgument::from_index_value(v.flatten(), lhs.start_position()) })?, expr => unreachable!("unknown chained expression: {:?}", expr), }; @@ -802,9 +792,7 @@ impl Engine { #[cfg(not(feature = "no_index"))] _ if _parent_chain_type == ChainType::Indexing => idx_values.push( self.eval_expr(scope, global, caches, lib, this_ptr, expr, level) - .map(|v| { - super::ChainArgument::from_index_value(v.flatten(), expr.start_position()) - })?, + .map(|v| ChainArgument::from_index_value(v.flatten(), expr.start_position()))?, ), _ => unreachable!("unknown chained expression: {:?}", expr), } diff --git a/src/eval/expr.rs b/src/eval/expr.rs index 61b18991..74ef43a4 100644 --- a/src/eval/expr.rs +++ b/src/eval/expr.rs @@ -322,9 +322,13 @@ impl Engine { // `... ${...} ...` Expr::InterpolatedString(x, _) => { - let mut concat: Dynamic = self.const_empty_string().into(); + let mut concat = self.const_empty_string().into(); + let target = &mut concat; let mut result = Ok(Dynamic::UNIT); + let mut op_info = OpAssignment::new_op_assignment(OP_CONCAT, Position::NONE); + let root = ("", Position::NONE); + for expr in x.iter() { let item = match self.eval_expr(scope, global, caches, lib, this_ptr, expr, level) { @@ -335,23 +339,17 @@ impl Engine { } }; - if let Err(err) = self.eval_op_assignment( - global, - caches, - lib, - Some(OpAssignment::new(OP_CONCAT)), - expr.start_position(), - &mut (&mut concat).into(), - ("", Position::NONE), - item, - level, - ) { - result = Err(err.fill_position(expr.start_position())); + op_info.pos = expr.start_position(); + + if let Err(err) = self + .eval_op_assignment(global, caches, lib, op_info, target, root, item, level) + { + result = Err(err); break; } } - result.map(|_| concat) + result.map(|_| concat.take_or_clone()) } #[cfg(not(feature = "no_index"))] diff --git a/src/eval/stmt.rs b/src/eval/stmt.rs index 4af2a105..5e8bc8f0 100644 --- a/src/eval/stmt.rs +++ b/src/eval/stmt.rs @@ -110,14 +110,12 @@ impl Engine { } /// Evaluate an op-assignment statement. - /// [`Position`] in [`EvalAltResult`] is [`NONE`][Position::NONE] and should be set afterwards. pub(crate) fn eval_op_assignment( &self, global: &mut GlobalRuntimeState, caches: &mut Caches, lib: &[&Module], - op_info: Option, - op_pos: Position, + op_info: OpAssignment, target: &mut Target, root: (&str, Position), new_val: Dynamic, @@ -130,13 +128,15 @@ impl Engine { let mut new_val = new_val; - if let Some(OpAssignment { - hash_op_assign, - hash_op, - op_assign, - op, - }) = op_info - { + if op_info.is_op_assignment() { + let OpAssignment { + hash_op_assign, + hash_op, + op_assign, + op, + pos: op_pos, + } = op_info; + let mut lock_guard; let lhs_ptr_inner; @@ -166,9 +166,11 @@ impl Engine { Err(err) if matches!(*err, ERR::ErrorFunctionNotFound(ref f, ..) if f.starts_with(op_assign)) => { // Expand to `var = var op rhs` - let (value, ..) = self.call_native_fn( - global, caches, lib, op, hash_op, args, true, false, op_pos, level, - )?; + let (value, ..) = self + .call_native_fn( + global, caches, lib, op, hash_op, args, true, false, op_pos, level, + ) + .map_err(|err| err.fill_position(op_info.pos))?; #[cfg(not(feature = "unchecked"))] self.check_data_size(&value, root.1)?; @@ -182,7 +184,9 @@ impl Engine { *target.as_mut() = new_val; } - target.propagate_changed_value() + target + .propagate_changed_value() + .map_err(|err| err.fill_position(op_info.pos)) } /// Evaluate a statement. @@ -228,7 +232,7 @@ impl Engine { // Then assignments. // We shouldn't do this for too many variants because, soon or later, the added comparisons // will cost more than the mis-predicted `match` branch. - if let Stmt::Assignment(x, op_pos) = stmt { + if let Stmt::Assignment(x, ..) = stmt { #[cfg(not(feature = "unchecked"))] self.inc_operations(&mut global.num_operations, stmt.position())?; @@ -261,9 +265,8 @@ impl Engine { let lhs_ptr = &mut lhs_ptr; self.eval_op_assignment( - global, caches, lib, *op_info, *op_pos, lhs_ptr, root, rhs_val, level, + global, caches, lib, *op_info, lhs_ptr, root, rhs_val, level, ) - .map_err(|err| err.fill_position(rhs.start_position())) .map(|_| Dynamic::UNIT) } else { search_result.map(|_| Dynamic::UNIT) @@ -279,7 +282,7 @@ impl Engine { .map(Dynamic::flatten); if let Ok(rhs_val) = rhs_result { - let _new_val = Some(((rhs_val, rhs.start_position()), (*op_info, *op_pos))); + let _new_val = Some((rhs_val, *op_info)); // Must be either `var[index] op= val` or `var.prop op= val` match lhs { diff --git a/src/eval/target.rs b/src/eval/target.rs index eb046be2..69d67440 100644 --- a/src/eval/target.rs +++ b/src/eval/target.rs @@ -272,6 +272,8 @@ impl<'a> Target<'a> { } /// Propagate a changed value back to the original source. /// This has no effect for direct references. + /// + /// [`Position`] in [`EvalAltResult`] is [`NONE`][Position::NONE] and should be set afterwards. #[inline] pub fn propagate_changed_value(&mut self) -> RhaiResultOf<()> { match self { diff --git a/src/optimizer.rs b/src/optimizer.rs index f68d84d5..3936b91c 100644 --- a/src/optimizer.rs +++ b/src/optimizer.rs @@ -426,7 +426,7 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b match stmt { // var = var op expr => var op= expr Stmt::Assignment(x, ..) - if x.0.is_none() + if !x.0.is_op_assignment() && x.1.lhs.is_variable_access(true) && matches!(&x.1.rhs, Expr::FnCall(x2, ..) if Token::lookup_from_syntax(&x2.name).map(|t| t.has_op_assignment()).unwrap_or(false) @@ -437,7 +437,7 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b match x.1.rhs { Expr::FnCall(ref mut x2, ..) => { state.set_dirty(); - x.0 = Some(OpAssignment::new_from_base(&x2.name)); + x.0 = OpAssignment::new_op_assignment_from_base(&x2.name, x2.pos); x.1.rhs = mem::take(&mut x2.args[1]); } ref expr => unreachable!("Expr::FnCall expected but gets {:?}", expr), diff --git a/src/parser.rs b/src/parser.rs index eacfc13e..1d684960 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1808,7 +1808,11 @@ impl Engine { } } - let op_info = op.map(OpAssignment::new_from_token); + let op_info = if let Some(op) = op { + OpAssignment::new_op_assignment_from_token(op, op_pos) + } else { + OpAssignment::new_assignment(op_pos) + }; match lhs { // const_expr = rhs @@ -1816,10 +1820,9 @@ impl Engine { Err(PERR::AssignmentToConstant("".into()).into_err(lhs.start_position())) } // var (non-indexed) = rhs - Expr::Variable(ref x, None, _) if x.0.is_none() => Ok(Stmt::Assignment( - (op_info, (lhs, rhs).into()).into(), - op_pos, - )), + Expr::Variable(ref x, None, _) if x.0.is_none() => { + Ok(Stmt::Assignment((op_info, (lhs, rhs).into()).into())) + } // var (indexed) = rhs Expr::Variable(ref x, i, var_pos) => { let (index, .., name) = x.as_ref(); @@ -1832,10 +1835,9 @@ impl Engine { .get_mut_by_index(state.stack.len() - index) .access_mode() { - AccessMode::ReadWrite => Ok(Stmt::Assignment( - (op_info, (lhs, rhs).into()).into(), - op_pos, - )), + AccessMode::ReadWrite => { + Ok(Stmt::Assignment((op_info, (lhs, rhs).into()).into())) + } // Constant values cannot be assigned to AccessMode::ReadOnly => { Err(PERR::AssignmentToConstant(name.to_string()).into_err(var_pos)) @@ -1854,10 +1856,9 @@ impl Engine { None => { match x.lhs { // var[???] = rhs, var.??? = rhs - Expr::Variable(..) => Ok(Stmt::Assignment( - (op_info, (lhs, rhs).into()).into(), - op_pos, - )), + Expr::Variable(..) => { + Ok(Stmt::Assignment((op_info, (lhs, rhs).into()).into())) + } // expr[???] = rhs, expr.??? = rhs ref expr => Err(PERR::AssignmentToInvalidLHS("".to_string()) .into_err(expr.position())),