C#: Generalize cs/constant-condition

csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql

@@ -14,39 +14,111 @@
 import csharp
 import semmle.code.csharp.commons.Assertions
 import semmle.code.csharp.commons.Constants
+import ControlFlowGraph
 
-/** A condition of an `if` statement or a conditional expression. */
-private class IfCondition extends Expr {
-  IfCondition() {
-    this = any(IfStmt is).getCondition() or
-    this = any(ConditionalExpr ce).getCondition()
+/** A constant condition. */
+abstract class ConstantCondition extends Expr {
+  /** Gets the alert message for this constant condition. */
+  abstract string getMessage();
+
+  /** Holds if this constant condition is white-listed. */
+  predicate isWhiteListed() { none() }
+}
+
+/** A constant Boolean condition. */
+class ConstantBooleanCondition extends ConstantCondition {
+  boolean b;
+
+  ConstantBooleanCondition() {
+    isConstantCondition(this, b)
+  }
+
+  override string getMessage() {
+    result = "Condition always evaluates to '" + b + "'."
+  }
+
+  override predicate isWhiteListed() {
+    // E.g. `x ?? false`
+    this.(BoolLiteral) = any(NullCoalescingExpr nce).getRightOperand()
   }
 }
 
-/** A loop condition */
-private class LoopCondition extends Expr {
-  LoopCondition() {
+/** A constant condition in an `if` statement or a conditional expression. */
+class ConstantIfCondition extends ConstantBooleanCondition {
+  ConstantIfCondition() {
+    this = any(IfStmt is).getCondition().getAChildExpr*() or
+    this = any(ConditionalExpr ce).getCondition().getAChildExpr*()
+  }
+
+  override predicate isWhiteListed() {
+    ConstantBooleanCondition.super.isWhiteListed()
+    or
+    // It is a common pattern to use a local constant/constant field to control
+    // whether code parts must be executed or not
+    this instanceof AssignableRead
+  }
+}
+
+/** A constant loop condition. */
+class ConstantLoopCondition extends ConstantBooleanCondition {
+  ConstantLoopCondition() {
     this = any(LoopStmt ls).getCondition()
   }
+
+  override predicate isWhiteListed() {
+    // Clearly intentional infinite loops are allowed
+    this.(BoolLiteral).getBoolValue() = true
+  }
 }
 
-/** Holds if `e` is a conditional expression that is allowed to be constant. */
-predicate isWhiteListed(Expr e) {
-  // It is a common pattern to use a local constant/constant field to control
-  // whether code parts must be executed or not
-  e = any(IfCondition ic).getAChildExpr*() and
-  e instanceof AssignableRead
-  or
-  // Clearly intentional infinite loops are allowed
-  e instanceof LoopCondition and
-  e.(BoolLiteral).getBoolValue() = true
-  or
-  // E.g. `x ?? false`
-  e.(BoolLiteral) = any(NullCoalescingExpr nce).getRightOperand()
+/** A constant nullness condition. */
+class ConstantNullnessCondition extends ConstantCondition {
+  boolean b;
+
+  ConstantNullnessCondition() {
+    forex(ControlFlowNode cfn |
+      cfn = this.getAControlFlowNode() |
+      exists(ControlFlowEdgeNullness t |
+        exists(cfn.getASuccessorByType(t)) |
+        if t.isNull() then b = true else b = false
+      ) and
+      strictcount(ControlFlowEdgeType t | exists(cfn.getASuccessorByType(t))) = 1
+    )
+  }
+
+  override string getMessage() {
+    if b = true then
+      result = "Expression is always 'null'."
+    else
+      result = "Expression is never 'null'."
+  }
 }
 
-from Expr e, boolean b
-where isConstantCondition(e, b)
-  and not isWhiteListed(e)
-  and not isExprInAssertion(e)
-select e, "Condition always evaluates to '" + b + "'."
+/** A constant matching condition. */
+class ConstantMatchingCondition extends ConstantCondition {
+  boolean b;
+
+  ConstantMatchingCondition() {
+    forex(ControlFlowNode cfn |
+      cfn = this.getAControlFlowNode() |
+      exists(ControlFlowEdgeMatching t |
+        exists(cfn.getASuccessorByType(t)) |
+        if t.isMatch() then b = true else b = false
+      ) and
+      strictcount(ControlFlowEdgeType t | exists(cfn.getASuccessorByType(t))) = 1
+    )
+  }
+
+  override string getMessage() {
+    if b = true then
+      result = "Pattern always matches."
+    else
+      result = "Pattern never matches."
+  }
+}
+
+from ConstantCondition c, string msg
+where msg = c.getMessage()
+  and not c.isWhiteListed()
+  and not isExprInAssertion(c)
+select c, msg

csharp/ql/src/Bad Practices/Control-Flow/ConstantNullCoalescingLeftHandOperand.qhelp

@@ -1,15 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-<overview>
-<p>If the left hand operand of a null-coalescing operator always evaluates to <code>null</code> the 
-null-coalescing operation is useless. The whole expression will always evaluate to the right hand 
-operand at runtime. This impedes readability and maintainability unnecessarily, by making the code 
-more difficult to read than it should be.</p>
-
-</overview>
-<recommendation>
-<p>Remove the useless null-coalescing expression and any potential dead code associated with it.</p>
-</recommendation>
-</qhelp>

csharp/ql/src/Bad Practices/Control-Flow/ConstantNullCoalescingLeftHandOperand.ql

@@ -1,16 +0,0 @@
-/**
- * @name Null-coalescing left operand is constant
- * @description Finds left operands in null-coalescing expressions that always evaluate to null
- * @kind problem
- * @problem.severity warning
- * @precision high
- * @id cs/constant-null-coalescing
- * @tags maintainability
- *       readability
- */
-import csharp
-
-from NullCoalescingExpr nce, Expr e
-where e = nce.getLeftOperand()
-  and exists(e.getValue())
-select e, "Left operand always evaluates to " + e.getValue() + "."

csharp/ql/src/Bad Practices/Control-Flow/ConstantSwitchSelector.cs

@@ -1,32 +0,0 @@
-using System;
-
-namespace ConstantSwitchSelector
-{
-    class Main
-    {
-        const int ZERO = 0;
-
-        public void Foo()
-        {
-            switch (ZERO + 1)
-            { // BAD
-                case 1: break;
-            }
-            switch ('a')
-            { // BAD
-                default: break;
-            }
-            switch (Bar())
-            { // GOOD
-                case 1: break;
-            }
-        }
-
-        public int Bar()
-        {
-            return ZERO;
-        }
-
-    }
-
-}

csharp/ql/src/Bad Practices/Control-Flow/ConstantSwitchSelector.qhelp

@@ -1,14 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-<overview>
-<p>A switch selector is useless if it always evaluates to the same constant at runtime. This 
-impedes readability and maintainability unnecessarily, by making the control-flow of the method 
-more difficult to read than it should be.</p>
-
-</overview>
-<recommendation>
-<p>Remove the useless switch selector and all cases that will never be reached.</p>
-</recommendation>
-</qhelp>

csharp/ql/src/Bad Practices/Control-Flow/ConstantSwitchSelector.ql

@@ -1,16 +0,0 @@
-/**
- * @name Switch selector is constant
- * @description Finds selectors in switch statements that always evaluate to the same constant
- * @kind problem
- * @problem.severity warning
- * @precision high
- * @id cs/constant-switch-selector
- * @tags maintainability
- *       readability
- */
-import csharp
-
-from SwitchStmt s, Expr e
-where e = s.getCondition()
-  and exists(e.getValue())
-select e, "Selector always evaluates to " + e.getValue() + "."

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs

@@ -1,6 +1,7 @@
 // semmle-extractor-options: /r:System.Threading.Thread.dll /r:System.Diagnostics.Debug.dll
 
 using System;
+using System.Collections;
 using System.Diagnostics;
 
 class ConstantCondition
@@ -32,6 +33,62 @@ class ConstantCondition
     bool M3(double d) => d == d; // BAD: but flagged by cs/constant-comparison
 }
 
+class ConstantNullness
+{
+    void M1(int i)
+    {
+        var j = ((string)null)?.Length; // BAD
+        var s = ((int?)i)?.ToString(); // BAD
+        var k = s?.Length; // GOOD
+        k = s?.ToLower()?.Length; // GOOD
+    }
+
+    void M2(int i)
+    {
+        var j = (int?)null ?? 0; // BAD
+        var s = "" ?? "a"; // BAD
+        j = (int?)i ?? 1; // BAD
+        s = ""?.CommaJoinWith(s); // BAD
+        s = s ?? ""; // GOOD
+    }
+}
+
+class ConstantMatching
+{
+    void M1()
+    {
+        switch (1 + 2)
+        {
+            case 2 : // BAD
+              break;
+            case 3 : // BAD
+              break;
+            case int _ : // GOOD
+              break;
+        }
+    }
+
+    void M2(string s)
+    {
+        switch ((object)s)
+        {
+            case int _ : // BAD
+              break;
+            case "" : // GOOD
+              break;
+        }
+    }
+
+    void M3(object o)
+    {
+        switch (o)
+        {
+            case IList _ : // GOOD
+              break;
+        }
+    }
+}
+
 class Assertions
 {
     void F()
@@ -39,3 +96,8 @@ class Assertions
         Debug.Assert(false ? false : true);  // GOOD
     }
 }
+
+static class Ext
+{
+    public static string CommaJoinWith(this string s1, string s2) => s1 + ", " + s2;
+}

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected

@@ -1,3 +1,12 @@
+| ConstantCondition.cs:40:18:40:29 | (...) ... | Expression is always 'null'. |
+| ConstantCondition.cs:41:18:41:24 | (...) ... | Expression is never 'null'. |
+| ConstantCondition.cs:48:17:48:26 | (...) ... | Expression is always 'null'. |
+| ConstantCondition.cs:49:17:49:18 | "" | Expression is never 'null'. |
+| ConstantCondition.cs:50:13:50:19 | (...) ... | Expression is never 'null'. |
+| ConstantCondition.cs:51:13:51:14 | "" | Expression is never 'null'. |
+| ConstantCondition.cs:62:18:62:18 | 2 | Pattern never matches. |
+| ConstantCondition.cs:64:18:64:18 | 3 | Pattern always matches. |
+| ConstantCondition.cs:75:18:75:20 | access to type Int32 | Pattern never matches. |
 | ConstantConditionBad.cs:5:16:5:20 | ... > ... | Condition always evaluates to 'false'. |
 | ConstantConditionalExpressionCondition.cs:11:22:11:34 | ... == ... | Condition always evaluates to 'true'. |
 | ConstantConditionalExpressionCondition.cs:12:21:12:25 | false | Condition always evaluates to 'false'. |
@@ -7,6 +16,8 @@
 | ConstantIfCondition.cs:11:17:11:29 | ... == ... | Condition always evaluates to 'true'. |
 | ConstantIfCondition.cs:14:17:14:21 | false | Condition always evaluates to 'false'. |
 | ConstantIfCondition.cs:17:17:17:26 | ... == ... | Condition always evaluates to 'true'. |
+| ConstantNullCoalescingLeftHandOperand.cs:11:24:11:34 | access to constant NULL_OBJECT | Expression is never 'null'. |
+| ConstantNullCoalescingLeftHandOperand.cs:12:24:12:27 | null | Expression is always 'null'. |
 | ConstantWhileCondition.cs:12:20:12:32 | ... == ... | Condition always evaluates to 'true'. |
 | ConstantWhileCondition.cs:16:20:16:24 | false | Condition always evaluates to 'false'. |
 | ConstantWhileCondition.cs:24:20:24:29 | ... == ... | Condition always evaluates to 'true'. |