Merge pull request #22708 from michaelnebel/csharp/improvesimplifyboolexpr

C#: Remove some FPs for `cs/simplifiable-boolean-expression`.

Author: michaelnebel

csharp/ql/src/Language Abuse/SimplifyBoolExpr.ql

@@ -13,56 +13,88 @@
 import csharp
 
 /**
- * Holds if expression `expr` has Boolean `value` at child `child`.
- * No other child nodes are boolean literals.
+ * Holds if the left operand of a binary operation is a Boolean literal with the specified value
+ * and the right operand is not a Boolean literal.
  */
-predicate literalChild(Expr expr, int child, boolean value) {
-  value = expr.getChild(child).(BoolLiteral).getBoolValue() and
-  forall(int c | c != child | not expr.getChild(c) instanceof BoolLiteral)
+predicate binaryLiteralLeft(BinaryOperation op, boolean value) {
+  value = op.getLeftOperand().(BoolLiteral).getBoolValue() and
+  not op.getRightOperand() instanceof BoolLiteral
 }
 
 /**
- * Expression `expr` has Boolean `value1` at child `child1`, and boolean `value2` at `child2`.
- * No other child nodes are boolean literals.
+ * Holds if the right operand of a binary operation is a Boolean literal with the specified value
+ * and the left operand is not a Boolean literal.
  */
-predicate literalChildren(Expr expr, int child1, boolean value1, int child2, boolean value2) {
-  value1 = expr.getChild(child1).(BoolLiteral).getBoolValue() and
-  value2 = expr.getChild(child2).(BoolLiteral).getBoolValue() and
-  forall(int c | c != child1 and c != child2 | not expr.getChild(c) instanceof BoolLiteral)
+predicate binaryLiteralRight(BinaryOperation op, boolean value) {
+  value = op.getRightOperand().(BoolLiteral).getBoolValue() and
+  not op.getLeftOperand() instanceof BoolLiteral
 }
 
-predicate rewriteBinaryExpr(BinaryOperation op, boolean value, string oldPattern) {
-  literalChild(op, 0, value) and oldPattern = value + " " + op.getOperator() + " A"
-  or
-  literalChild(op, 1, value) and oldPattern = "A " + op.getOperator() + " " + value
+/**
+ * Holds if the 'then' branch of a conditional expression is a Boolean literal with the specified value
+ * and the 'condition' or 'else' branch are not Boolean literals.
+ */
+predicate conditionalThenLiteral(ConditionalExpr cond, boolean value) {
+  value = cond.getThen().(BoolLiteral).getBoolValue() and
+  not cond.getCondition() instanceof BoolLiteral and
+  not cond.getElse() instanceof BoolLiteral
 }
 
-bindingset[withFalseOperand, withTrueOperand]
-predicate rewriteBinaryExpr(
-  BinaryOperation op, string oldPattern, string withFalseOperand, string withTrueOperand,
-  string newPattern
-) {
-  rewriteBinaryExpr(op, false, oldPattern) and newPattern = withFalseOperand
-  or
-  rewriteBinaryExpr(op, true, oldPattern) and newPattern = withTrueOperand
+/**
+ * Holds if the 'else' branch of a conditional expression is a Boolean literal with the specified value
+ * and the 'condition' or 'then' branch are not Boolean literals.
+ */
+predicate conditionalElseLiteral(ConditionalExpr cond, boolean value) {
+  value = cond.getElse().(BoolLiteral).getBoolValue() and
+  not cond.getCondition() instanceof BoolLiteral and
+  not cond.getThen() instanceof BoolLiteral
+}
+
+/**
+ * Holds if both the 'then' and 'else' branches of a conditional expression are Boolean literals with the specified values
+ * and the 'condition' branch is not a Boolean literal.
+ */
+predicate conditionalThenAndElseLiteral(ConditionalExpr cond, boolean thenValue, boolean elseValue) {
+  thenValue = cond.getThen().(BoolLiteral).getBoolValue() and
+  elseValue = cond.getElse().(BoolLiteral).getBoolValue() and
+  not cond.getCondition() instanceof BoolLiteral
+}
+
+predicate rewriteBinaryExpr(BinaryOperation op, boolean value, string oldPattern) {
+  op.getLeftOperand().getType() instanceof BoolType and
+  op.getRightOperand().getType() instanceof BoolType and
+  (
+    binaryLiteralLeft(op, value) and oldPattern = value + " " + op.getOperator() + " A"
+    or
+    binaryLiteralRight(op, value) and oldPattern = "A " + op.getOperator() + " " + value
+  )
 }
 
 predicate rewriteConditionalExpr(ConditionalExpr cond, string oldPattern, string newPattern) {
-  literalChild(cond, 1, false) and oldPattern = "A ? false : B" and newPattern = "!A && B"
-  or
-  literalChild(cond, 1, true) and oldPattern = "A ? true : B" and newPattern = "A || B"
-  or
-  literalChild(cond, 2, false) and oldPattern = "A ? B : false" and newPattern = "A && B"
-  or
-  literalChild(cond, 2, true) and oldPattern = "A ? B : true" and newPattern = "!A || B"
-  or
-  exists(boolean b | literalChildren(cond, 1, b, 2, b) |
-    oldPattern = "A ? " + b + " : " + b and newPattern = b.toString()
+  cond.getCondition().getType() instanceof BoolType and
+  cond.getThen().getType() instanceof BoolType and
+  cond.getElse().getType() instanceof BoolType and
+  (
+    conditionalThenLiteral(cond, false) and oldPattern = "A ? false : B" and newPattern = "!A && B"
+    or
+    conditionalThenLiteral(cond, true) and oldPattern = "A ? true : B" and newPattern = "A || B"
+    or
+    conditionalElseLiteral(cond, false) and oldPattern = "A ? B : false" and newPattern = "A && B"
+    or
+    conditionalElseLiteral(cond, true) and oldPattern = "A ? B : true" and newPattern = "!A || B"
+    or
+    exists(boolean b | conditionalThenAndElseLiteral(cond, b, b) |
+      oldPattern = "A ? " + b + " : " + b and newPattern = b.toString()
+    )
+    or
+    conditionalThenAndElseLiteral(cond, true, false) and
+    oldPattern = "A ? true : false" and
+    newPattern = "A"
+    or
+    conditionalThenAndElseLiteral(cond, false, true) and
+    oldPattern = "A ? false : true" and
+    newPattern = "!A"
   )
-  or
-  literalChildren(cond, 1, true, 2, false) and oldPattern = "A ? true : false" and newPattern = "A"
-  or
-  literalChildren(cond, 1, false, 2, true) and oldPattern = "A ? false : true" and newPattern = "!A"
 }
 
 predicate negatedOperators(string op, string negated) {
@@ -115,12 +147,20 @@ predicate pushNegation(LogicalNotExpr expr, string oldPattern, string newPattern
   )
 }
 
-predicate rewrite(Expr expr, string oldPattern, string newPattern) {
+predicate rewriteBinaryOperation(BinaryOperation op, string oldPattern, string newPattern) {
   exists(string withFalseOperand, string withTrueOperand |
-    simplifyBinaryExpr(expr.(BinaryOperation).getOperator(), withFalseOperand, withTrueOperand)
+    simplifyBinaryExpr(op.getOperator(), withFalseOperand, withTrueOperand)
   |
-    rewriteBinaryExpr(expr, oldPattern, withFalseOperand, withTrueOperand, newPattern)
+    rewriteBinaryExpr(op, false, oldPattern) and
+    newPattern = withFalseOperand
+    or
+    rewriteBinaryExpr(op, true, oldPattern) and
+    newPattern = withTrueOperand
   )
+}
+
+predicate rewrite(Expr expr, string oldPattern, string newPattern) {
+  rewriteBinaryOperation(expr, oldPattern, newPattern)
   or
   rewriteConditionalExpr(expr, oldPattern, newPattern)
   or

csharp/ql/src/change-notes/2026-09-30-simplify-bool-expr.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cs/simplifiable-boolean-expression` query now produces fewer false-positive results when type information is incomplete.

csharp/ql/test/query-tests/Language Abuse/SimplifyBoolExpr/SimplifyBoolExpr.cs

@@ -44,5 +44,8 @@ void Fn()
         if (true != false) ;
         if (true && true) ;
         if (true || false) ;
+
+        bool? boption = false;
+        if (boption == true) ; // GOOD. Can't be simplified like a regular bool
     }
 }

csharp/ql/test/query-tests/standalone/Language Abuse/SimplifyBoolExpr/SimplifyBoolExpr.cs

@@ -58,3 +58,25 @@ class IncompleteNestedOperatorTest
         return lambda() || Local();
     }
 }
+
+class Test
+{
+    public class Container
+    {
+        public bool? Field;
+    }
+
+    public void Fn()
+    {
+        bool? boption = false;
+        if (boption == true) ; // GOOD. Can't be simplified like a regular bool
+
+        bool b = false;
+        if (b ? boption : false) ; // GOOD. Can't be simplified like a regular bool
+
+        // Emulating incomplete type information by not declaring container explicitly.
+        if (container.Field == true) ; // GOOD. Can't be simplified like a regular bool
+
+        if (b ? container.Field : false) ; // GOOD. Can't be simplified like a regular bool
+    }
+}