Merge pull request #95 from ISibboI/sebschmi/i94_parenthese_parsing_bug

Sebschmi/i94 parenthese parsing bug

CHANGELOG.md

@@ -16,6 +16,22 @@
 
 ### Contributors
 
+## [7.0.0](https://github.com/ISibboI/evalexpr/compare/6.6.0...7.0.0) - 2022-01-13
+
+### Changed
+
+ * Made the `EvalexprError` enum non_exhaustive.
+
+### Fixed
+
+ * Expressions that have dangling parenthese expressions such as `4(5)` now produce an error.
+
+### Contributors
+
+My warmhearted thanks goes to
+
+ * [dbr/Ben](https://github.com/dbr)
+
 ## [6.6.0](https://github.com/ISibboI/evalexpr/compare/6.5.0...6.6.0) - 2021-10-13
 
 ### Added

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "evalexpr"
-version = "6.6.0"
+version = "7.0.0"
 description = "A powerful arithmetic and boolean expression evaluator"
 keywords = ["expression", "evaluate", "evaluator", "arithmetic", "boolean"]
 categories = ["parsing", "game-engines"]

benches/benchs.rs

@@ -1,5 +1,6 @@
 #![feature(test)]
 #![feature(bench_black_box)]
+#![cfg(not(tarpaulin_include))]
 
 extern crate rand;
 extern crate rand_pcg;

src/error/display.rs

@@ -69,6 +69,12 @@ impl fmt::Display for EvalexprError {
             ),
             UnmatchedLBrace => write!(f, "Found an unmatched opening parenthesis '('."),
             UnmatchedRBrace => write!(f, "Found an unmatched closing parenthesis ')'."),
+            MissingOperatorOutsideOfBrace { .. } => write!(
+                f,
+                "Found an opening parenthesis that is preceded by something that does not take \
+                 any arguments on the right, or found a closing parenthesis that is succeeded by \
+                 something that does not take any arguments on the left."
+            ),
             UnmatchedPartialToken { first, second } => {
                 if let Some(second) = second {
                     write!(

src/error/mod.rs

@@ -15,6 +15,7 @@ mod display;
 
 /// Errors used in this crate.
 #[derive(Debug, PartialEq)]
+#[non_exhaustive]
 pub enum EvalexprError {
     /// An operator was called with a wrong amount of arguments.
     WrongOperatorArgumentAmount {
@@ -128,6 +129,10 @@ pub enum EvalexprError {
     /// A closing brace without a matching opening brace was found.
     UnmatchedRBrace,
 
+    /// Left of an opening brace or right of a closing brace is a token that does not expect the brace next to it.
+    /// For example, writing `4(5)` would yield this error, as the `4` does not have any operands.
+    MissingOperatorOutsideOfBrace,
+
     /// A `PartialToken` is unmatched, such that it cannot be combined into a full `Token`.
     /// This happens if for example a single `=` is found, surrounded by whitespace.
     /// It is not a token, but it is part of the string representation of some tokens.
@@ -361,3 +366,36 @@ impl std::error::Error for EvalexprError {}
 
 /// Standard result type used by this crate.
 pub type EvalexprResult<T> = Result<T, EvalexprError>;
+
+#[cfg(test)]
+mod tests {
+    use crate::{EvalexprError, Value, ValueType};
+
+    /// Tests whose only use is to bring test coverage of trivial lines up, like trivial constructors.
+    #[test]
+    fn trivial_coverage_tests() {
+        assert_eq!(
+            EvalexprError::type_error(Value::Int(3), vec![ValueType::String]),
+            EvalexprError::TypeError {
+                actual: Value::Int(3),
+                expected: vec![ValueType::String]
+            }
+        );
+        assert_eq!(
+            EvalexprError::expected_type(&Value::String("abc".to_string()), Value::Empty),
+            EvalexprError::expected_string(Value::Empty)
+        );
+        assert_eq!(
+            EvalexprError::expected_type(&Value::Boolean(false), Value::Empty),
+            EvalexprError::expected_boolean(Value::Empty)
+        );
+        assert_eq!(
+            EvalexprError::expected_type(&Value::Tuple(vec![]), Value::Empty),
+            EvalexprError::expected_tuple(Value::Empty)
+        );
+        assert_eq!(
+            EvalexprError::expected_type(&Value::Empty, Value::String("abc".to_string())),
+            EvalexprError::expected_empty(Value::String("abc".to_string()))
+        );
+    }
+}

src/lib.rs

@@ -5,7 +5,7 @@
 //!
 //! ```toml
 //! [dependencies]
-//! evalexpr = "6"
+//! evalexpr = "7"
 //! ```
 //!
 //! Then you can use `evalexpr` to **evaluate expressions** like this:

src/token/mod.rs

@@ -372,7 +372,7 @@ fn partial_tokens_to_tokens(mut tokens: &[PartialToken]) -> EvalexprResult<Vec<T
                                 } else {
                                     Some(Token::Identifier(literal.to_string()))
                                 }
-                            }
+                            },
                             _ => Some(Token::Identifier(literal.to_string())),
                         }
                     }

src/tree/mod.rs

@@ -406,6 +406,14 @@ impl Node {
         Some(self.children().len()) == self.operator().max_argument_amount()
     }
 
+    fn has_too_many_children(&self) -> bool {
+        if let Some(max_argument_amount) = self.operator().max_argument_amount() {
+            self.children().len() > max_argument_amount
+        } else {
+            false
+        }
+    }
+
     fn insert_back_prioritized(&mut self, node: Node, is_root_node: bool) -> EvalexprResult<()> {
         // println!("Inserting {:?} into {:?}", node.operator, self.operator());
         if self.operator().precedence() < node.operator().precedence() || is_root_node
@@ -415,7 +423,7 @@ impl Node {
             if self.operator().is_leaf() {
                 Err(EvalexprError::AppendedToLeafNode)
             } else if self.has_enough_children() {
-                // Unwrap cannot fail because of has_enough_children
+                // Unwrap cannot fail because is_leaf being false and has_enough_children being true implies that the operator wants and has at least one child
                 let last_child_operator = self.children.last().unwrap().operator();
 
                 if last_child_operator.precedence()
@@ -425,7 +433,7 @@ impl Node {
                     == node.operator().precedence() && !last_child_operator.is_left_to_right() && !node.operator().is_left_to_right())
                 {
                     // println!("Recursing into {:?}", self.children.last().unwrap().operator());
-                    // Unwrap cannot fail because of has_enough_children
+                    // Unwrap cannot fail because is_leaf being false and has_enough_children being true implies that the operator wants and has at least one child
                     self.children
                         .last_mut()
                         .unwrap()
@@ -436,11 +444,35 @@ impl Node {
                         return Err(EvalexprError::AppendedToLeafNode);
                     }
 
-                    // Unwrap cannot fail because of has_enough_children
+                    // Unwrap cannot fail because is_leaf being false and has_enough_children being true implies that the operator wants and has at least one child
                     let last_child = self.children.pop().unwrap();
+                    // Root nodes have at most one child
+                    // TODO I am not sure if this is the correct error
+                    if self.operator() == &Operator::RootNode && !self.children().is_empty() {
+                        return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+                    }
+                    // Do not insert root nodes into root nodes.
+                    // TODO I am not sure if this is the correct error
+                    if self.operator() == &Operator::RootNode
+                        && node.operator() == &Operator::RootNode
+                    {
+                        return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+                    }
                     self.children.push(node);
                     let node = self.children.last_mut().unwrap();
 
+                    // Root nodes have at most one child
+                    // TODO I am not sure if this is the correct error
+                    if node.operator() == &Operator::RootNode && !node.children().is_empty() {
+                        return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+                    }
+                    // Do not insert root nodes into root nodes.
+                    // TODO I am not sure if this is the correct error
+                    if node.operator() == &Operator::RootNode
+                        && last_child.operator() == &Operator::RootNode
+                    {
+                        return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+                    }
                     node.children.push(last_child);
                     Ok(())
                 }
@@ -492,6 +524,11 @@ fn collapse_all_sequences(root_stack: &mut Vec<Node>) -> EvalexprResult<()> {
     loop {
         // println!("Root is: {:?}", root);
         if root.operator() == &Operator::RootNode {
+            // This should fire if parsing something like `4(5)`
+            if root.has_too_many_children() {
+                return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+            }
+
             root_stack.push(root);
             break;
         }
@@ -501,6 +538,11 @@ fn collapse_all_sequences(root_stack: &mut Vec<Node>) -> EvalexprResult<()> {
                 potential_higher_root.children.push(root);
                 root = potential_higher_root;
             } else {
+                // This should fire if parsing something like `4(5)`
+                if root.has_too_many_children() {
+                    return Err(EvalexprError::MissingOperatorOutsideOfBrace);
+                }
+
                 root_stack.push(potential_higher_root);
                 root_stack.push(root);
                 break;

tests/integration.rs

@@ -582,6 +582,7 @@ fn test_shortcut_functions() {
         eval_number("abc"),
         Err(EvalexprError::VariableIdentifierNotFound("abc".to_owned()))
     );
+    assert_eq!(eval_number_with_context("3.5", &context), Ok(3.5));
     assert_eq!(eval_number_with_context("3", &context), Ok(3.0));
     assert_eq!(
         eval_number_with_context("true", &context),
@@ -593,6 +594,7 @@ fn test_shortcut_functions() {
         eval_number_with_context("abc", &context),
         Err(EvalexprError::VariableIdentifierNotFound("abc".to_owned()))
     );
+    assert_eq!(eval_number_with_context_mut("3.5", &mut context), Ok(3.5));
     assert_eq!(eval_number_with_context_mut("3", &mut context), Ok(3.0));
     assert_eq!(
         eval_number_with_context_mut("true", &mut context),
@@ -1806,3 +1808,30 @@ fn test_value_type() {
         Ok(Value::String(String::new()))
     );
 }
+
+#[test]
+fn test_parenthese_combinations() {
+    // These are from issue #94
+    assert_eq!(
+        eval("123(1*2)"),
+        Err(EvalexprError::MissingOperatorOutsideOfBrace)
+    );
+    assert_eq!(
+        eval("1()"),
+        Err(EvalexprError::MissingOperatorOutsideOfBrace)
+    );
+    assert_eq!(
+        eval("1()()()()"),
+        Err(EvalexprError::MissingOperatorOutsideOfBrace)
+    );
+    assert_eq!(
+        eval("1()()()(9)()()"),
+        Err(EvalexprError::MissingOperatorOutsideOfBrace)
+    );
+    assert_eq!(
+        eval_with_context("a+100(a*2)", &context_map! {"a" => 4}.unwrap()),
+        Err(EvalexprError::MissingOperatorOutsideOfBrace)
+    );
+    assert_eq!(eval_int("(((1+2)*(3+4)+(5-(6)))/((7-8)))"), Ok(-20));
+    assert_eq!(eval_int("(((((5)))))"), Ok(5));
+}