From d055638e83b2dd49a96dbe9f5bead0265d644005 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 7 Mar 2020 17:32:15 +0800 Subject: [PATCH] Properly detect invalid assignment LHS at compile time. --- src/engine.rs | 85 +++++++++++++++++++++++++++++++++++---------------- src/error.rs | 2 +- src/parser.rs | 47 +++++++++++++--------------- src/result.rs | 15 +++++---- 4 files changed, 90 insertions(+), 59 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 5418a382..bacd8982 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -17,6 +17,8 @@ pub type FnCallArgs<'a> = Vec<&'a mut Variant>; const KEYWORD_PRINT: &'static str = "print"; const KEYWORD_DEBUG: &'static str = "debug"; const KEYWORD_TYPE_OF: &'static str = "type_of"; +const FUNC_GETTER: &'static str = "get$"; +const FUNC_SETTER: &'static str = "set$"; #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] enum IndexSourceType { @@ -153,6 +155,12 @@ impl Engine<'_> { } else if let Some(val) = def_value { // Return default value Ok(val.clone()) + } else if spec.name.starts_with(FUNC_GETTER) || spec.name.starts_with(FUNC_SETTER) { + // Getter or setter + Err(EvalAltResult::ErrorDotExpr( + "- invalid property access".to_string(), + pos, + )) } else { let types_list = args .iter() @@ -193,7 +201,7 @@ impl Engine<'_> { // xxx.id Expr::Identifier(id, pos) => { - let get_fn_name = format!("get${}", id); + let get_fn_name = format!("{}{}", FUNC_GETTER, id); self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos) } @@ -204,16 +212,18 @@ impl Engine<'_> { let (lhs_value, _) = match lhs.as_ref() { Expr::Identifier(id, pos) => { - let get_fn_name = format!("get${}", id); + let get_fn_name = format!("{}{}", FUNC_GETTER, id); ( self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos)?, *pos, ) } - expr => return Err(EvalAltResult::ErrorDotExpr(expr.position())), + expr => { + return Err(EvalAltResult::ErrorDotExpr("".to_string(), expr.position())) + } }; - Self::get_indexed_value(lhs_value, idx, idx_expr.position(), *idx_pos) + self.get_indexed_value(lhs_value, idx, idx_expr.position(), *idx_pos) .map(|(v, _)| v) } @@ -221,7 +231,7 @@ impl Engine<'_> { Expr::Dot(lhs, rhs, _) => match lhs.as_ref() { // xxx.id.rhs Expr::Identifier(id, pos) => { - let get_fn_name = format!("get${}", id); + let get_fn_name = format!("{}{}", FUNC_GETTER, 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(), rhs)) @@ -232,25 +242,34 @@ impl Engine<'_> { let (lhs_value, _) = match lhs.as_ref() { Expr::Identifier(id, pos) => { - let get_fn_name = format!("get${}", id); + let get_fn_name = format!("{}{}", FUNC_GETTER, id); ( self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos)?, *pos, ) } - expr => return Err(EvalAltResult::ErrorDotExpr(expr.position())), + expr => { + return Err(EvalAltResult::ErrorDotExpr( + "".to_string(), + expr.position(), + )) + } }; - Self::get_indexed_value(lhs_value, idx, idx_expr.position(), *idx_pos).and_then( - |(mut value, _)| self.get_dot_val_helper(scope, value.as_mut(), rhs), - ) + self.get_indexed_value(lhs_value, idx, idx_expr.position(), *idx_pos) + .and_then(|(mut value, _)| { + self.get_dot_val_helper(scope, value.as_mut(), rhs) + }) } // Syntax error - _ => Err(EvalAltResult::ErrorDotExpr(lhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr("".to_string(), lhs.position())), }, // Syntax error - _ => Err(EvalAltResult::ErrorDotExpr(dot_rhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr( + "".to_string(), + dot_rhs.position(), + )), } } @@ -281,6 +300,7 @@ impl Engine<'_> { /// Get the value at the indexed position of a base type fn get_indexed_value( + &self, val: Dynamic, idx: i64, val_pos: Position, @@ -318,7 +338,10 @@ impl Engine<'_> { } } else { // Error - cannot be indexed - Err(EvalAltResult::ErrorIndexingType(idx_pos)) + Err(EvalAltResult::ErrorIndexingType( + self.map_type_name(val.type_name()).to_string(), + idx_pos, + )) } } @@ -337,7 +360,7 @@ impl Engine<'_> { Expr::Identifier(id, _) => Self::search_scope( scope, &id, - |val| Self::get_indexed_value(val, idx, idx_expr.position(), idx_pos), + |val| self.get_indexed_value(val, idx, idx_expr.position(), idx_pos), lhs.position(), ) .map(|(src_idx, (val, src_type))| { @@ -345,13 +368,12 @@ impl Engine<'_> { }), // (expr)[idx_expr] - expr => Self::get_indexed_value( - self.eval_expr(scope, expr)?, - idx, - idx_expr.position(), - idx_pos, - ) - .map(|(val, _)| (IndexSourceType::Expression, None, idx as usize, val)), + expr => { + let val = self.eval_expr(scope, expr)?; + + self.get_indexed_value(val, idx, idx_expr.position(), idx_pos) + .map(|(value, _)| (IndexSourceType::Expression, None, idx as usize, value)) + } } } @@ -456,7 +478,7 @@ impl Engine<'_> { match dot_rhs { // xxx.id Expr::Identifier(id, pos) => { - let set_fn_name = format!("set${}", id); + let set_fn_name = format!("{}{}", FUNC_SETTER, id); self.call_fn_raw( &set_fn_name, @@ -469,7 +491,7 @@ impl Engine<'_> { // xxx.lhs.rhs Expr::Dot(lhs, rhs, _) => match lhs.as_ref() { Expr::Identifier(id, pos) => { - let get_fn_name = format!("get${}", id); + let get_fn_name = format!("{}{}", FUNC_GETTER, id); self.call_fn_raw(&get_fn_name, vec![this_ptr], None, *pos) .and_then(|mut v| { @@ -477,16 +499,22 @@ impl Engine<'_> { .map(|_| v) // Discard Ok return value }) .and_then(|mut v| { - let set_fn_name = format!("set${}", id); + let set_fn_name = format!("{}{}", FUNC_SETTER, id); self.call_fn_raw(&set_fn_name, vec![this_ptr, v.as_mut()], None, *pos) }) } - _ => Err(EvalAltResult::ErrorDotExpr(lhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr( + "for assignment".to_string(), + lhs.position(), + )), }, // Syntax error - _ => Err(EvalAltResult::ErrorDotExpr(dot_rhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr( + "for assignment".to_string(), + dot_rhs.position(), + )), } } @@ -530,7 +558,10 @@ impl Engine<'_> { } // Syntax error - _ => Err(EvalAltResult::ErrorDotExpr(dot_lhs.position())), + _ => Err(EvalAltResult::ErrorDotExpr( + "for assignment".to_string(), + dot_lhs.position(), + )), } } diff --git a/src/error.rs b/src/error.rs index 52ca89fc..d4096cb1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -116,7 +116,7 @@ impl Error for ParseError { ParseErrorType::FnMissingName => "Expecting name in function declaration", ParseErrorType::FnMissingParams(_) => "Expecting parameters in function declaration", ParseErrorType::WrongFnDefinition => "Function definitions must be at top level and cannot be inside a block or another function", - ParseErrorType::AssignmentToInvalidLHS => "Assignment to an unsupported left-hand side expression" + ParseErrorType::AssignmentToInvalidLHS => "Cannot assign to this expression because it will only be changing a copy of the value" } } diff --git a/src/parser.rs b/src/parser.rs index f6fa43df..5989b601 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1275,39 +1275,36 @@ fn parse_unary<'a>(input: &mut Peekable>) -> Result Result { - fn all_identifiers(expr: &Expr) -> (bool, Position) { + fn all_dots(expr: &Expr) -> (bool, Position) { match expr { - // variable Expr::Identifier(_, pos) => (true, *pos), - // indexing - Expr::Index(lhs, _, idx_pos) => match lhs.as_ref() { - // variable[x] - &Expr::Identifier(_, pos) => (true, pos), - // all other indexing is invalid - _ => (false, *idx_pos), + Expr::Dot(dot_lhs, dot_rhs, _) => match dot_lhs.as_ref() { + Expr::Identifier(_, _) => all_dots(dot_rhs), + _ => (false, dot_lhs.position()), }, - // variable.prop.prop.prop... - Expr::Dot(lhs, rhs, _) => match lhs.as_ref() { - // variable.prop - &Expr::Identifier(_, pos) => { - let r = all_identifiers(rhs); - (r.0, if r.0 { pos } else { r.1 }) - } - // all other property access is invalid - _ => (false, lhs.position()), - }, - // everything else is invalid _ => (false, expr.position()), } } - let r = all_identifiers(&lhs); - - if r.0 { - Ok(Expr::Assignment(Box::new(lhs), Box::new(rhs), pos)) - } else { - Err(ParseError::new(PERR::AssignmentToInvalidLHS, r.1)) + match &lhs { + Expr::Identifier(_, _) => return Ok(Expr::Assignment(Box::new(lhs), Box::new(rhs), pos)), + Expr::Index(idx_lhs, _, _) => match idx_lhs.as_ref() { + Expr::Identifier(_, _) => { + return Ok(Expr::Assignment(Box::new(lhs), Box::new(rhs), pos)) + } + _ => (), + }, + Expr::Dot(_, _, _) => match all_dots(&lhs) { + (true, _) => return Ok(Expr::Assignment(Box::new(lhs), Box::new(rhs), pos)), + (false, pos) => return Err(ParseError::new(PERR::AssignmentToInvalidLHS, pos)), + }, + _ => (), } + + return Err(ParseError::new( + PERR::AssignmentToInvalidLHS, + lhs.position(), + )); } fn parse_op_assignment( diff --git a/src/result.rs b/src/result.rs index 1d869c8b..8fcae55a 100644 --- a/src/result.rs +++ b/src/result.rs @@ -26,7 +26,7 @@ pub enum EvalAltResult { /// Wrapped values are the current number of characters in the string and the index number. ErrorStringBounds(usize, i64, Position), /// Trying to index into a type that is not an array and not a string. - ErrorIndexingType(Position), + ErrorIndexingType(String, Position), /// Trying to index into an array or string with an index that is not `i64`. ErrorIndexExpr(Position), /// The guard expression in an `if` statement does not return a boolean value. @@ -43,7 +43,7 @@ pub enum EvalAltResult { /// Error reading from a script file. Wrapped value is the path of the script file. ErrorReadingScriptFile(String, std::io::Error), /// Inappropriate member access. - ErrorDotExpr(Position), + ErrorDotExpr(String, Position), /// Arithmetic error encountered. Wrapped value is the error message. ErrorArithmetic(String, Position), /// Run-time error encountered. Wrapped value is the error message. @@ -65,7 +65,9 @@ impl Error for EvalAltResult { } Self::ErrorBooleanArgMismatch(_, _) => "Boolean operator expects boolean operands", Self::ErrorIndexExpr(_) => "Indexing into an array or string expects an integer index", - Self::ErrorIndexingType(_) => "Indexing can only be performed on an array or a string", + Self::ErrorIndexingType(_, _) => { + "Indexing can only be performed on an array or a string" + } Self::ErrorArrayBounds(_, index, _) if *index < 0 => { "Array access expects non-negative index" } @@ -84,7 +86,7 @@ impl Error for EvalAltResult { } Self::ErrorMismatchOutputType(_, _) => "Output type is incorrect", Self::ErrorReadingScriptFile(_, _) => "Cannot read from script file", - Self::ErrorDotExpr(_) => "Malformed dot expression", + Self::ErrorDotExpr(_, _) => "Malformed dot expression", Self::ErrorArithmetic(_, _) => "Arithmetic error", Self::ErrorRuntime(_, _) => "Runtime error", Self::LoopBreak => "[Not Error] Breaks out of loop", @@ -104,13 +106,14 @@ impl std::fmt::Display for EvalAltResult { match self { Self::ErrorFunctionNotFound(s, pos) => write!(f, "{}: '{}' ({})", desc, s, pos), Self::ErrorVariableNotFound(s, pos) => write!(f, "{}: '{}' ({})", desc, s, pos), - Self::ErrorIndexingType(pos) => write!(f, "{} ({})", desc, pos), + Self::ErrorIndexingType(_, pos) => write!(f, "{} ({})", desc, pos), Self::ErrorIndexExpr(pos) => write!(f, "{} ({})", desc, pos), Self::ErrorIfGuard(pos) => write!(f, "{} ({})", desc, pos), Self::ErrorFor(pos) => write!(f, "{} ({})", desc, pos), Self::ErrorAssignmentToUnknownLHS(pos) => write!(f, "{} ({})", desc, pos), Self::ErrorMismatchOutputType(s, pos) => write!(f, "{}: {} ({})", desc, s, pos), - Self::ErrorDotExpr(pos) => write!(f, "{} ({})", desc, pos), + Self::ErrorDotExpr(s, pos) if !s.is_empty() => write!(f, "{} {} ({})", desc, s, pos), + Self::ErrorDotExpr(_, pos) => write!(f, "{} ({})", desc, pos), Self::ErrorArithmetic(s, pos) => write!(f, "{}: {} ({})", desc, s, pos), Self::ErrorRuntime(s, pos) if s.is_empty() => write!(f, "{} ({})", desc, pos), Self::ErrorRuntime(s, pos) => write!(f, "{}: {} ({})", desc, s, pos),