From 3d3b939ba68147918ae7b8add365613686e0862c Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Fri, 6 Mar 2020 01:05:02 +0800 Subject: [PATCH] Simplify code, document logic, refactor and better error messages. --- src/engine.rs | 264 ++++++++++++++++++++++++++------------------------ src/error.rs | 17 ++-- src/parser.rs | 82 ++++++++++++---- src/scope.rs | 7 ++ 4 files changed, 220 insertions(+), 150 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 74654cd5..2b9abf1c 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -105,21 +105,16 @@ impl Engine<'_> { if let Some(f) = fn_def { match *f { FnIntExt::Ext(ref f) => { - let r = f(args, pos); - - if r.is_err() { - return r; - } + let r = f(args, pos)?; let callback = match spec.name.as_ref() { KEYWORD_PRINT => self.on_print.as_mut(), KEYWORD_DEBUG => self.on_debug.as_mut(), - _ => return r, + _ => return Ok(r), }; Ok(callback( - &r.unwrap() - .downcast::() + &r.downcast::() .map(|s| *s) .unwrap_or("error: not a string".into()), ) @@ -172,6 +167,7 @@ impl Engine<'_> { } } + /// Chain-evaluate a dot setter fn get_dot_val_helper( &mut self, scope: &mut Scope, @@ -181,6 +177,7 @@ impl Engine<'_> { use std::iter::once; match dot_rhs { + // xxx.fn_name(args) Expr::FunctionCall(fn_name, args, def_value, pos) => { let mut args: Array = args .iter() @@ -194,17 +191,16 @@ impl Engine<'_> { self.call_fn_raw(fn_name, args, def_value.as_ref(), *pos) } + // xxx.id Expr::Identifier(id, pos) => { let get_fn_name = format!("get${}", id); self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos) } + // xxx.lhs[idx_expr] Expr::Index(lhs, idx_expr) => { - let idx = *self - .eval_expr(scope, idx_expr)? - .downcast::() - .map_err(|_| EvalAltResult::ErrorIndexExpr(idx_expr.position()))?; + let idx = self.eval_index_value(scope, idx_expr)?; let (lhs_value, pos) = match lhs.as_ref() { Expr::Identifier(id, pos) => { @@ -220,18 +216,18 @@ impl Engine<'_> { Self::get_indexed_value(lhs_value, idx, pos).map(|(v, _)| v) } - Expr::Dot(inner_lhs, inner_rhs) => match inner_lhs.as_ref() { + // xxx.lhs.rhs + Expr::Dot(lhs, rhs) => match lhs.as_ref() { + // xxx.id.rhs Expr::Identifier(id, pos) => { let get_fn_name = format!("get${}", id); self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos) - .and_then(|mut v| self.get_dot_val_helper(scope, v.as_mut(), inner_rhs)) + .and_then(|mut v| self.get_dot_val_helper(scope, v.as_mut(), rhs)) } + // xxx.lhs[idx_expr].rhs Expr::Index(lhs, idx_expr) => { - let idx = *self - .eval_expr(scope, idx_expr)? - .downcast::() - .map_err(|_| EvalAltResult::ErrorIndexExpr(idx_expr.position()))?; + let idx = self.eval_index_value(scope, idx_expr)?; let (lhs_value, pos) = match lhs.as_ref() { Expr::Identifier(id, pos) => { @@ -245,16 +241,19 @@ impl Engine<'_> { }; Self::get_indexed_value(lhs_value, idx, pos).and_then(|(mut value, _)| { - self.get_dot_val_helper(scope, value.as_mut(), inner_rhs) + self.get_dot_val_helper(scope, value.as_mut(), rhs) }) } - _ => Err(EvalAltResult::ErrorDotExpr(inner_lhs.position())), + // Syntax error + _ => Err(EvalAltResult::ErrorDotExpr(lhs.position())), }, + // Syntax error _ => Err(EvalAltResult::ErrorDotExpr(dot_rhs.position())), } } + /// Search for a variable within the scope, returning its value and index inside the Scope fn search_scope( scope: &Scope, id: &str, @@ -267,13 +266,26 @@ impl Engine<'_> { .and_then(move |(idx, _, val)| map(val).map(|v| (idx, v))) } + /// Evaluate the value of an index (must evaluate to i64) + fn eval_index_value( + &mut self, + scope: &mut Scope, + idx_expr: &Expr, + ) -> Result { + self.eval_expr(scope, idx_expr)? + .downcast::() + .map(|v| *v) + .map_err(|_| EvalAltResult::ErrorIndexExpr(idx_expr.position())) + } + + /// Get the value at the indexed position of a base type fn get_indexed_value( val: Dynamic, idx: i64, pos: Position, ) -> Result<(Dynamic, VariableType), EvalAltResult> { if val.is::() { - let arr = val.downcast::().unwrap(); + let arr = val.downcast::().expect("Array expected"); if idx >= 0 { arr.get(idx as usize) @@ -284,7 +296,7 @@ impl Engine<'_> { Err(EvalAltResult::ErrorArrayBounds(arr.len(), idx, pos)) } } else if val.is::() { - let s = val.downcast::().unwrap(); + let s = val.downcast::().expect("String expected"); if idx >= 0 { s.chars() @@ -303,16 +315,14 @@ impl Engine<'_> { } } + /// Evaluate an index expression fn eval_index_expr( &mut self, scope: &mut Scope, lhs: &Expr, idx_expr: &Expr, ) -> Result<(VariableType, Option<(String, usize)>, usize, Dynamic), EvalAltResult> { - let idx = *self - .eval_expr(scope, idx_expr)? - .downcast::() - .map_err(|_| EvalAltResult::ErrorIndexExpr(idx_expr.position()))?; + let idx = self.eval_index_value(scope, idx_expr)?; match lhs { Expr::Identifier(id, _) => Self::search_scope( @@ -330,9 +340,10 @@ impl Engine<'_> { } } + /// Replace a character at an index position in a mutable string fn str_replace_char(s: &mut String, idx: usize, new_ch: char) { // The new character - let ch = s.chars().nth(idx).unwrap(); + let ch = s.chars().nth(idx).expect("string index out of bounds"); // See if changed - if so, update the String if ch == new_ch { @@ -346,6 +357,35 @@ impl Engine<'_> { chars.iter().for_each(|&ch| s.push(ch)); } + /// Update the value at an index position in a variable inside the scope + fn update_indexed_variable_in_scope( + source_type: VariableType, + scope: &mut Scope, + id: &str, + src_idx: usize, + idx: usize, + val: Dynamic, + ) -> Option { + match source_type { + VariableType::Array => { + let arr = scope.get_mut_by_type::(id, src_idx); + Some((arr[idx as usize] = val).into_dynamic()) + } + + VariableType::String => { + let s = scope.get_mut_by_type::(id, src_idx); + // Value must be a character + let ch = *val + .downcast::() + .expect("value to update an index position in a string must be a char"); + Some(Self::str_replace_char(s, idx as usize, ch).into_dynamic()) + } + + _ => None, + } + } + + /// Evaluate a dot chain getter fn get_dot_val( &mut self, scope: &mut Scope, @@ -353,6 +393,7 @@ impl Engine<'_> { dot_rhs: &Expr, ) -> Result { match dot_lhs { + // xxx.??? Expr::Identifier(id, pos) => { let (sc_idx, mut target) = Self::search_scope(scope, id, Ok, *pos)?; let value = self.get_dot_val_helper(scope, target.as_mut(), dot_rhs); @@ -364,6 +405,7 @@ impl Engine<'_> { value } + // lhs[idx_expr].??? Expr::Index(lhs, idx_expr) => { let (source_type, src, idx, mut target) = self.eval_index_expr(scope, lhs, idx_expr)?; @@ -371,39 +413,27 @@ impl Engine<'_> { // In case the expression mutated `target`, we need to reassign it because // of the above `clone`. - - match source_type { - VariableType::Array => { - let src = src.unwrap(); - scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .unwrap()[idx] = target - } - - VariableType::String => { - let src = src.unwrap(); - - Self::str_replace_char( - scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .unwrap(), // Root is a string - idx, - *target.downcast::().unwrap(), // Target should be a char - ) - } - - _ => panic!("source_type must be either Array or String"), + if let Some((id, src_idx)) = src { + Self::update_indexed_variable_in_scope( + source_type, + scope, + &id, + src_idx, + idx, + target, + ) + .expect("source_type must be either Array or String"); } value } + // Syntax error _ => Err(EvalAltResult::ErrorDotExpr(dot_lhs.position())), } } + /// Chain-evaluate a dot setter fn set_dot_val_helper( &mut self, this_ptr: &mut Variant, @@ -411,6 +441,7 @@ impl Engine<'_> { mut source_val: Dynamic, ) -> Result { match dot_rhs { + // xxx.id Expr::Identifier(id, pos) => { let set_fn_name = format!("set${}", id); @@ -422,13 +453,14 @@ impl Engine<'_> { ) } - Expr::Dot(inner_lhs, inner_rhs) => match inner_lhs.as_ref() { + // xxx.lhs.rhs + Expr::Dot(lhs, rhs) => match lhs.as_ref() { Expr::Identifier(id, pos) => { let get_fn_name = format!("get${}", id); self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos) .and_then(|mut v| { - self.set_dot_val_helper(v.as_mut(), inner_rhs, source_val) + self.set_dot_val_helper(v.as_mut(), rhs, source_val) .map(|_| v) // Discard Ok return value }) .and_then(|mut v| { @@ -437,13 +469,15 @@ impl Engine<'_> { self.call_fn_raw(&set_fn_name, vec![this_ptr, v.as_mut()], None, *pos) }) } - _ => Err(EvalAltResult::ErrorDotExpr(inner_lhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr(lhs.position())), }, + // Syntax error _ => Err(EvalAltResult::ErrorDotExpr(dot_rhs.position())), } } + // Evaluate a dot chain setter fn set_dot_val( &mut self, scope: &mut Scope, @@ -452,6 +486,7 @@ impl Engine<'_> { source_val: Dynamic, ) -> Result { match dot_lhs { + // id.??? Expr::Identifier(id, pos) => { let (sc_idx, mut target) = Self::search_scope(scope, id, Ok, *pos)?; let value = self.set_dot_val_helper(target.as_mut(), dot_rhs, source_val); @@ -463,6 +498,7 @@ impl Engine<'_> { value } + // lhs[idx_expr].??? Expr::Index(lhs, idx_expr) => { let (source_type, src, idx, mut target) = self.eval_index_expr(scope, lhs, idx_expr)?; @@ -471,39 +507,27 @@ impl Engine<'_> { // In case the expression mutated `target`, we need to reassign it because // of the above `clone`. - match source_type { - VariableType::Array => { - let src = src.unwrap(); - let val = scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .unwrap(); - val[idx] = target - } - - VariableType::String => { - let src = src.unwrap(); - - Self::str_replace_char( - scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .unwrap(), // Root is a string - idx, - *target.downcast::().unwrap(), // Target should be a char - ) - } - - _ => panic!("source_type must be either Array or String"), + if let Some((id, src_idx)) = src { + Self::update_indexed_variable_in_scope( + source_type, + scope, + &id, + src_idx, + idx, + target, + ) + .expect("source_type must be either Array or String"); } value } + // Syntax error _ => Err(EvalAltResult::ErrorDotExpr(dot_lhs.position())), } } + /// Evaluate an expression fn eval_expr(&mut self, scope: &mut Scope, expr: &Expr) -> Result { match expr { Expr::IntegerConstant(i, _) => Ok((*i).into_dynamic()), @@ -517,10 +541,12 @@ impl Engine<'_> { .eval_index_expr(scope, lhs, idx_expr) .map(|(_, _, _, x)| x), + // lhs = rhs Expr::Assignment(lhs, rhs) => { let rhs_val = self.eval_expr(scope, rhs)?; match lhs.as_ref() { + // name = rhs Expr::Identifier(name, pos) => { if let Some((idx, _, _)) = scope.get(name) { *scope.get_mut(name, idx) = rhs_val; @@ -530,50 +556,34 @@ impl Engine<'_> { } } + // idx_lhs[idx_expr] = rhs Expr::Index(idx_lhs, idx_expr) => { let (source_type, src, idx, _) = self.eval_index_expr(scope, idx_lhs, idx_expr)?; - match source_type { - VariableType::Array => { - let src = src.unwrap(); - scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .map(|arr| (arr[idx as usize] = rhs_val).into_dynamic()) - .ok_or_else(|| { - EvalAltResult::ErrorIndexExpr(idx_lhs.position()) - }) - } - - VariableType::String => { - let src = src.unwrap(); - scope - .get_mut(&src.0, src.1) - .downcast_mut::() - .map(|s| { - Self::str_replace_char( - s, - idx as usize, - *rhs_val.downcast::().unwrap(), - ) - .into_dynamic() - }) - .ok_or_else(|| { - EvalAltResult::ErrorIndexExpr(idx_lhs.position()) - }) - } - - _ => Err(EvalAltResult::ErrorAssignmentToUnknownLHS( - idx_lhs.position(), - )), + if let Some((id, src_idx)) = src { + Self::update_indexed_variable_in_scope( + source_type, + scope, + &id, + src_idx, + idx, + rhs_val, + ) + } else { + None } + .ok_or_else(|| { + EvalAltResult::ErrorAssignmentToUnknownLHS(idx_lhs.position()) + }) } + // dot_lhs.dot_rhs = rhs Expr::Dot(dot_lhs, dot_rhs) => { self.set_dot_val(scope, dot_lhs, dot_rhs, rhs_val) } + // Syntax error _ => Err(EvalAltResult::ErrorAssignmentToUnknownLHS(lhs.position())), } } @@ -615,12 +625,13 @@ impl Engine<'_> { .map_err(|_| { EvalAltResult::ErrorBooleanArgMismatch("AND".into(), lhs.position()) })? - && *self - .eval_expr(scope, &*rhs)? - .downcast::() - .map_err(|_| { - EvalAltResult::ErrorBooleanArgMismatch("AND".into(), rhs.position()) - })?, + && // Short-circuit using && + *self + .eval_expr(scope, &*rhs)? + .downcast::() + .map_err(|_| { + EvalAltResult::ErrorBooleanArgMismatch("AND".into(), rhs.position()) + })?, )), Expr::Or(lhs, rhs) => Ok(Box::new( @@ -630,12 +641,13 @@ impl Engine<'_> { .map_err(|_| { EvalAltResult::ErrorBooleanArgMismatch("OR".into(), lhs.position()) })? - || *self - .eval_expr(scope, &*rhs)? - .downcast::() - .map_err(|_| { - EvalAltResult::ErrorBooleanArgMismatch("OR".into(), rhs.position()) - })?, + || // Short-circuit using || + *self + .eval_expr(scope, &*rhs)? + .downcast::() + .map_err(|_| { + EvalAltResult::ErrorBooleanArgMismatch("OR".into(), rhs.position()) + })?, )), Expr::True(_) => Ok(true.into_dynamic()), @@ -644,6 +656,7 @@ impl Engine<'_> { } } + /// Evaluate a statement pub(crate) fn eval_stmt( &mut self, scope: &mut Scope, @@ -679,8 +692,8 @@ impl Engine<'_> { .and_then(|guard_val| { if *guard_val { self.eval_stmt(scope, body) - } else if else_body.is_some() { - self.eval_stmt(scope, else_body.as_ref().unwrap()) + } else if let Some(stmt) = else_body { + self.eval_stmt(scope, stmt.as_ref()) } else { Ok(().into_dynamic()) } @@ -775,6 +788,7 @@ impl Engine<'_> { } } + /// Map a type_name into a pretty-print name pub(crate) fn map_type_name<'a>(&'a self, name: &'a str) -> &'a str { self.type_names .get(name) diff --git a/src/error.rs b/src/error.rs index 6314a031..d6410031 100644 --- a/src/error.rs +++ b/src/error.rs @@ -56,13 +56,13 @@ pub enum ParseErrorType { /// An unknown operator is encountered. Wrapped value is the operator. UnknownOperator(String), /// An open `(` is missing the corresponding closing `)`. - MissingRightParen, + MissingRightParen(String), /// Expecting `(` but not finding one. MissingLeftBrace, /// An open `{` is missing the corresponding closing `}`. - MissingRightBrace, + MissingRightBrace(String), /// An open `[` is missing the corresponding closing `]`. - MissingRightBracket, + MissingRightBracket(String), /// An expression in function call arguments `()` has syntax error. MalformedCallExpr, /// An expression in indexing brackets `[]` has syntax error. @@ -104,10 +104,10 @@ impl Error for ParseError { ParseErrorType::BadInput(ref p) => p, ParseErrorType::InputPastEndOfFile => "Script is incomplete", ParseErrorType::UnknownOperator(_) => "Unknown operator", - ParseErrorType::MissingRightParen => "Expecting ')'", + ParseErrorType::MissingRightParen(_) => "Expecting ')'", ParseErrorType::MissingLeftBrace => "Expecting '{'", - ParseErrorType::MissingRightBrace => "Expecting '}'", - ParseErrorType::MissingRightBracket => "Expecting ']'", + ParseErrorType::MissingRightBrace(_) => "Expecting '}'", + ParseErrorType::MissingRightBracket(_) => "Expecting ']'", ParseErrorType::MalformedCallExpr => "Invalid expression in function call arguments", ParseErrorType::MalformedIndexExpr => "Invalid index in indexing expression", ParseErrorType::VarExpectsIdentifier => "Expecting name of a variable", @@ -130,6 +130,11 @@ impl fmt::Display for ParseError { ParseErrorType::FnMissingParams(ref s) => { write!(f, "Missing parameters for function '{}'", s)? } + ParseErrorType::MissingRightParen(ref s) + | ParseErrorType::MissingRightBrace(ref s) + | ParseErrorType::MissingRightBracket(ref s) => { + write!(f, "{} for {}", self.description(), s)? + } _ => write!(f, "{}", self.description())?, } diff --git a/src/parser.rs b/src/parser.rs index 855c2bca..4a634d90 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1043,7 +1043,16 @@ fn parse_paren_expr<'a>( match input.next() { Some((Token::RightParen, _)) => Ok(expr), - _ => Err(ParseError::new(PERR::MissingRightParen, Position::eof())), + Some((_, pos)) => { + return Err(ParseError::new( + PERR::MissingRightParen("a matching ( in the expression".into()), + pos, + )) + } + None => Err(ParseError::new( + PERR::MissingRightParen("a matching ( in the expression".into()), + Position::eof(), + )), } } @@ -1068,8 +1077,24 @@ fn parse_call_expr<'a>( return Ok(Expr::FunctionCall(id, args, None, begin)); } Some(&(Token::Comma, _)) => (), - Some(&(_, pos)) => return Err(ParseError::new(PERR::MalformedCallExpr, pos)), - None => return Err(ParseError::new(PERR::MalformedCallExpr, Position::eof())), + Some(&(_, pos)) => { + return Err(ParseError::new( + PERR::MissingRightParen(format!( + "closing the arguments list to function call of '{}'", + id + )), + pos, + )) + } + None => { + return Err(ParseError::new( + PERR::MissingRightParen(format!( + "closing the arguments list to function call of '{}'", + id + )), + Position::eof(), + )) + } } input.next(); @@ -1080,17 +1105,24 @@ fn parse_index_expr<'a>( lhs: Box, input: &mut Peekable>, ) -> Result { - match parse_expr(input) { - Ok(idx_expr) => match input.peek() { - Some(&(Token::RightBracket, _)) => { - input.next(); - return Ok(Expr::Index(lhs, Box::new(idx_expr))); - } - Some(&(_, pos)) => return Err(ParseError::new(PERR::MalformedIndexExpr, pos)), - None => return Err(ParseError::new(PERR::MalformedIndexExpr, Position::eof())), - }, - Err(err) => return Err(ParseError::new(PERR::MalformedIndexExpr, err.position())), - } + parse_expr(input).and_then(|idx_expr| match input.peek() { + Some(&(Token::RightBracket, _)) => { + input.next(); + return Ok(Expr::Index(lhs, Box::new(idx_expr))); + } + Some(&(_, pos)) => { + return Err(ParseError::new( + PERR::MissingRightBracket("index expression".into()), + pos, + )) + } + None => { + return Err(ParseError::new( + PERR::MissingRightBracket("index expression".into()), + Position::eof(), + )) + } + }) } fn parse_ident_expr<'a>( @@ -1141,8 +1173,14 @@ fn parse_array_expr<'a>( input.next(); Ok(Expr::Array(arr, begin)) } - Some(&(_, pos)) => Err(ParseError::new(PERR::MissingRightBracket, pos)), - None => Err(ParseError::new(PERR::MissingRightBracket, Position::eof())), + Some(&(_, pos)) => Err(ParseError::new( + PERR::MissingRightBracket("the end of array literal".into()), + pos, + )), + None => Err(ParseError::new( + PERR::MissingRightBracket("the end of array literal".into()), + Position::eof(), + )), } } @@ -1462,7 +1500,7 @@ fn parse_binary_op<'a>( } token => { return Err(ParseError::new( - PERR::UnknownOperator(token.syntax().to_string()), + PERR::UnknownOperator(token.syntax().into()), pos, )) } @@ -1599,8 +1637,14 @@ fn parse_block<'a>(input: &mut Peekable>) -> Result Err(ParseError::new(PERR::MissingRightBrace, pos)), - None => Err(ParseError::new(PERR::MissingRightBrace, Position::eof())), + Some(&(_, pos)) => Err(ParseError::new( + PERR::MissingRightBrace("end of block".into()), + pos, + )), + None => Err(ParseError::new( + PERR::MissingRightBrace("end of block".into()), + Position::eof(), + )), } } diff --git a/src/scope.rs b/src/scope.rs index bafba6ed..9e7f63dc 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -85,6 +85,13 @@ impl Scope { &mut entry.1 } + /// Get a mutable reference to a variable in the Scope and downcast it to a specific type + pub(crate) fn get_mut_by_type(&mut self, key: &str, index: usize) -> &mut T { + self.get_mut(key, index) + .downcast_mut::() + .expect("wrong type cast") + } + /// Get an iterator to variables in the Scope. pub fn iter(&self) -> impl Iterator { self.0