Fix incorrect boolean calculations and add tests

The following tests failed before the change: [info] Ast$Test: [info] - considers `false and ?` constant *** FAILED *** [info] None was not equal to Some(false) (Ast$Test.scala:67) [info] - considers `? and false` constant *** FAILED *** [info] None was not equal to Some(false) (Ast$Test.scala:74) [info] - considers `true or ?` constant *** FAILED *** [info] None was not equal to Some(true) (Ast$Test.scala:81) [info] - considers `? or true` constant *** FAILED *** [info] None was not equal to Some(true) (Ast$Test.scala:88)
kaitai-io · Sep 12, 2023 · cc217c3 · cc217c3
1 parent 527ba04
commit cc217c3
Show file tree

Hide file tree

Showing 3 changed files with 177 additions and 11 deletions.
diff --git a/jvm/src/test/scala/io/kaitai/struct/exprlang/Ast$Test.scala b/jvm/src/test/scala/io/kaitai/struct/exprlang/Ast$Test.scala
@@ -51,4 +51,138 @@ class Ast$Test extends AnyFunSpec {
       Expressions.parse("(x + 1) * (x + 1) - (x * x + 2 * x + 2)").evaluateIntConst should be(None) // be(Some(0))
     }
   }
+
+  describe("Ast.expr.evaluateBoolConst") {
+    it ("considers `true` constant") {
+      Expressions.parse("true").evaluateBoolConst should be(Some(true))
+    }
+
+    it ("considers `false` constant") {
+      Expressions.parse("false").evaluateBoolConst should be(Some(false))
+    }
+
+    it ("considers `false and ?` constant") {
+      Expressions.parse("false and false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("false and true").evaluateBoolConst should be(Some(false))
+      Expressions.parse("false and x").evaluateBoolConst should be(Some(false))
+      Expressions.parse("false and (1==1)").evaluateBoolConst should be(Some(false))
+    }
+
+    it ("considers `? and false` constant") {
+      Expressions.parse("false and false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("true and false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("x and false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("(1==1) and false").evaluateBoolConst should be(Some(false))
+    }
+
+    it ("considers `true or ?` constant") {
+      Expressions.parse("true or false" ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("true or true"  ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("true or x"     ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("true or (1==1)").evaluateBoolConst should be(Some(true))
+    }
+
+    it ("considers `? or true` constant") {
+      Expressions.parse("false  or true").evaluateBoolConst should be(Some(true))
+      Expressions.parse("true   or true").evaluateBoolConst should be(Some(true))
+      Expressions.parse("x      or true").evaluateBoolConst should be(Some(true))
+      Expressions.parse("(1==1) or true").evaluateBoolConst should be(Some(true))
+    }
+
+    it ("evaluates `? == ?`") {
+      Expressions.parse("true  == true" ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("false == false").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42    == 42"   ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("field == field").evaluateBoolConst should be(None)//(Some(true))//TODO: symbolic calculations
+
+      Expressions.parse("true  == false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("false == true" ).evaluateBoolConst should be(Some(false))
+      Expressions.parse("42    == 420"  ).evaluateBoolConst should be(Some(false))
+      Expressions.parse("field == other").evaluateBoolConst should be(None)
+    }
+
+    it ("evaluates `? != ?`") {
+      Expressions.parse("true  != true" ).evaluateBoolConst should be(Some(false))
+      Expressions.parse("false != false").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42    != 42"   ).evaluateBoolConst should be(Some(false))
+      Expressions.parse("field != field").evaluateBoolConst should be(None)//(Some(false))//TODO: symbolic calculations
+
+      Expressions.parse("true  != false").evaluateBoolConst should be(Some(true))
+      Expressions.parse("false != true" ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("42    != 420"  ).evaluateBoolConst should be(Some(true))
+      Expressions.parse("field != other").evaluateBoolConst should be(None)
+    }
+
+    it ("evaluates `? < ?`") {
+      Expressions.parse("42 < 10").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 < 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 < 99").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 < xx").evaluateBoolConst should be(None)
+      Expressions.parse("xx < xx").evaluateBoolConst should be(None)//(Some(false))//TODO: symbolic calculations
+      Expressions.parse("xx < yy").evaluateBoolConst should be(None)
+
+      Expressions.parse("10 < 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 < 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("99 < 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("xx < 42").evaluateBoolConst should be(None)
+    }
+
+    it ("evaluates `? <= ?`") {
+      Expressions.parse("42 <= 10").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 <= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 <= 99").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 <= xx").evaluateBoolConst should be(None)
+      Expressions.parse("xx <= xx").evaluateBoolConst should be(None)//(Some(true))//TODO: symbolic calculations
+      Expressions.parse("xx <= yy").evaluateBoolConst should be(None)
+
+      Expressions.parse("10 <= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 <= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("99 <= 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("xx <= 42").evaluateBoolConst should be(None)
+    }
+
+    it ("evaluates `? > ?`") {
+      Expressions.parse("42 > 10").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 > 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 > 99").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 > xx").evaluateBoolConst should be(None)
+      Expressions.parse("xx > xx").evaluateBoolConst should be(None)//(Some(false))//TODO: symbolic calculations
+      Expressions.parse("xx > yy").evaluateBoolConst should be(None)
+
+      Expressions.parse("10 > 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 > 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("99 > 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("xx > 42").evaluateBoolConst should be(None)
+    }
+
+    it ("evaluates `? >= ?`") {
+      Expressions.parse("42 >= 10").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 >= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("42 >= 99").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 >= xx").evaluateBoolConst should be(None)
+      Expressions.parse("xx >= xx").evaluateBoolConst should be(None)//(Some(true))//TODO: symbolic calculations
+      Expressions.parse("xx >= yy").evaluateBoolConst should be(None)
+
+      Expressions.parse("10 >= 42").evaluateBoolConst should be(Some(false))
+      Expressions.parse("42 >= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("99 >= 42").evaluateBoolConst should be(Some(true))
+      Expressions.parse("xx >= 42").evaluateBoolConst should be(None)
+    }
+
+    it ("considers `[true, false, 7==7][2]` constant") {
+      Expressions.parse("[true, false, 7==7][2]").evaluateBoolConst should be(Some(true))
+    }
+
+    it ("considers `4 > 2 ? true : false` constant") {
+      Expressions.parse("4 > 2 ? true : false").evaluateBoolConst should be(Some(true))
+    }
+
+    it ("considers `x` variable") {
+      Expressions.parse("x").evaluateBoolConst should be(None)
+    }
+
+    it ("considers `[true, false, 7==7][x]` variable") {
+      Expressions.parse("[true, false, 7==7][x]").evaluateBoolConst should be(None)
+    }
+  }
 }
diff --git a/shared/src/main/scala/io/kaitai/struct/exprlang/Ast.scala b/shared/src/main/scala/io/kaitai/struct/exprlang/Ast.scala
@@ -61,6 +61,17 @@ object Ast {
       case ConstEvaluator.value.Str(x) => Some(x)
       case _ => None
     }
+    /**
+      * Evaluates the expression, if it's possible to get a static boolean
+      * constant as the result of evaluation (i.e. if it does not involve any
+      * variables or anything like that). Expect no complex logic or symbolic
+      * simplification of expressions here: something like "x and !x", which is
+      * known to be always `false`, will still report it as "None".
+      *
+      * @return boolean result of evaluation if it's constant or None, if it's
+      *         variable
+      */
+    def evaluateBoolConst: Option[Boolean] = ConstEvaluator.evaluateBoolConst(this)
   }
 
   object expr{

diff --git a/shared/src/main/scala/io/kaitai/struct/exprlang/ConstEvaluator.scala b/shared/src/main/scala/io/kaitai/struct/exprlang/ConstEvaluator.scala
@@ -29,6 +29,22 @@ object ConstEvaluator {
     }
   }
 
+  /**
+    * Evaluates the expression, if it's possible to get a boolean constant
+    * as the result of evaluation (i.e. if it does not involve any variables
+    * or anything like that).
+    *
+    * @param ex expression to evaluate.
+    * @return boolean result of evaluation if it's constant or None, if it's
+    *         variable or potentially variable.
+    */
+  def evaluateBoolConst(ex: Ast.expr): Option[Boolean] = {
+    evaluate(ex) match {
+      case value.Bool(x) => Some(x)
+      case _ => None
+    }
+  }
+
   /**
     * Evaluates the expression, if it's possible to get a constant as the result
     * of evaluation (i.e. if it does not involve any variables or anything like
@@ -72,17 +88,22 @@ object ConstEvaluator {
         case _ => value.NonConst
       }
 
-    case expr.BoolOp(op, values) =>
-      value.Bool(values.foldLeft(true)((acc, right) => {
-        val rightValue = evaluate(right) match {
-          case value.Bool(x) => x
-          case _ => return value.NonConst
-        }
-        op match {
-          case boolop.And => acc && rightValue
-          case boolop.Or => acc || rightValue
-        }
-      }))
+    case expr.BoolOp(boolop.And, values) => values.foldLeft[value](value.Bool(true))(
+      (acc, right) => evaluate(right) match {
+        // `... && false` always produce `false`, so do early return
+        case value.Bool(false) => return value.Bool(false)
+        case value.Bool(true)  => value.Bool(true)
+        case _ => value.NonConst
+      }
+    )
+    case expr.BoolOp(boolop.Or, values) => values.foldLeft[value](value.Bool(false))(
+      (acc, right) => evaluate(right) match {
+        // `... || true` always produce `false`, so do early return
+        case value.Bool(true)  => return value.Bool(true)
+        case value.Bool(false) => value.Bool(false)
+        case _ => value.NonConst
+      }
+    )
 
     case expr.Compare(left, op, right) =>
       val leftValue = evaluate(left)