Merge pull request #6 from hvitved/csharp/query/constant-condition

Approved by calumgrant

Author: semmle-qlci

change-notes/1.18/analysis-csharp.md

@@ -15,8 +15,11 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| [Exposing internal representation (cs/expose-implementation)] | Different results | The query has been rewritten, based on the equivalent Java query. |
+| Constant condition (cs/constant-condition) | More results | The query has been generalized to cover both `Null-coalescing left operand is constant (cs/constant-null-coalescing)` and `Switch selector is constant (cs/constant-switch-selector)`. |
+| Exposing internal representation (cs/expose-implementation) | Different results | The query has been rewritten, based on the equivalent Java query. |
 | Local scope variable shadows member (cs/local-shadows-member) | maintainability, readability | Replaces the existing queries [Local variable shadows class member (cs/local-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+class+member), [Local variable shadows struct member (cs/local-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+struct+member), [Parameter shadows class member (cs/parameter-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+class+member), and [Parameter shadows struct member (cs/parameter-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+struct+member). |
+| Null-coalescing left operand is constant (cs/constant-null-coalescing) | No results | The query has been removed, as it is now covered by `Constant condition (cs/constant-condition)`. |
+| Switch selector is constant (cs/constant-switch-selector) | No results | The query has been removed, as it is now covered by `Constant condition (cs/constant-condition)`. |
 
 ## Changes to existing queries
 

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 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 loop condition */
-private class LoopCondition extends Expr {
-  LoopCondition() {
+/** 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
+  }
+}
+
+/** 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'."
+  }
 }
 
-/** 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 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 Expr e, boolean b
-where isConstantCondition(e, b)
-  and not isWhiteListed(e)
-  and not isExprInAssertion(e)
-select e, "Condition always evaluates to '" + b + "'."
+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

@@ -81,11 +81,11 @@ class Completion extends TCompletion {
     or
     if mustHaveBooleanCompletion(cfe) then
       exists(boolean value |
-        isConstant(cfe, value) |
+        isBooleanConstant(cfe, value) |
         this = TBooleanCompletion(value, value)
       )
       or
-      not isConstant(cfe, _) and
+      not isBooleanConstant(cfe, _) and
       exists(boolean b | this = TBooleanCompletion(b, b))
       or
       // Corner case: In `if (x ?? y) { ... }`, `x` must have both a `true`
@@ -94,8 +94,20 @@ class Completion extends TCompletion {
       mustHaveNullnessCompletion(cfe) and
       this = TNullnessCompletion(true)
     else if mustHaveNullnessCompletion(cfe) then
+      exists(boolean value |
+        isNullnessConstant(cfe, value) |
+        this = TNullnessCompletion(value)
+      )
+      or
+      not isNullnessConstant(cfe, _) and
       this = TNullnessCompletion(_)
-    else if mustHaveMatchingCompletion(cfe) then
+    else if mustHaveMatchingCompletion(_, cfe) then
+      exists(boolean value |
+        isMatchingConstant(cfe, value) |
+        this = TMatchingCompletion(value)
+      )
+      or
+      not isMatchingConstant(cfe, _) and
       this = TMatchingCompletion(_)
     else if mustHaveEmptinessCompletion(cfe) then
       this = TEmptinessCompletion(_)
@@ -118,14 +130,82 @@ class Completion extends TCompletion {
   }
 }
 
-private predicate isConstant(Expr e, boolean value) {
-  e.getValue() = "true" and
-  value = true
-  or
-  e.getValue() = "false" and
-  value = false
-  or
-  isConstantComparison(e, value)
+/** Holds if expression `e` has the Boolean constant value `value`. */
+private predicate isBooleanConstant(Expr e, boolean value) {
+  mustHaveBooleanCompletion(e) and
+  (
+    e.getValue() = "true" and
+    value = true
+    or
+    e.getValue() = "false" and
+    value = false
+    or
+    isConstantComparison(e, value)
+  )
+}
+
+/**
+ * Holds if expression `e` is constantly `null` (`value = true`) or constantly
+ * non-`null` (`value = false`).
+ */
+private predicate isNullnessConstant(Expr e, boolean value) {
+  mustHaveNullnessCompletion(e) and
+  exists(Expr stripped |
+    stripped = e.stripCasts() |
+    stripped.getType() = any(ValueType t |
+      not t instanceof NullableType and
+      // Extractor bug: the type of `x?.Length` is reported as `int`, but it should
+      // be `int?`
+      not getQualifier*(stripped).(QualifiableExpr).isConditional()
+    ) and
+    value = false
+    or
+    stripped instanceof NullLiteral and
+    value = true
+    or
+    stripped.hasValue() and
+    not stripped instanceof NullLiteral and
+    value = false
+  )
+}
+
+private Expr getQualifier(QualifiableExpr e) {
+  // `e.getQualifier()` does not work for calls to extension methods
+  result = e.getChildExpr(-1)
+}
+
+/**
+ * Holds if expression `e` constantly matches (`value = true`) or constantly
+ * non-matches (`value = false`).
+ */
+private predicate isMatchingConstant(Expr e, boolean value) {
+  exists(SwitchStmt ss |
+    mustHaveMatchingCompletion(ss, e) |
+    exists(Expr stripped |
+      stripped = ss.getCondition().stripCasts() |
+      exists(ConstCase cc, string strippedValue |
+        cc = ss.getAConstCase() and
+        e = cc.getExpr() and
+        strippedValue = stripped.getValue() |
+        if strippedValue = e.getValue() then
+          value = true
+        else
+          value = false
+      )
+      or
+      exists(TypeCase tc, Type t, Type strippedType |
+        tc = ss.getATypeCase() |
+        e = tc.getTypeAccess() and
+        t = e.getType() and
+        strippedType = stripped.getType() and
+        not t.isImplicitlyConvertibleTo(strippedType) and
+        not t instanceof Interface and
+        not t.containsTypeParameters() and
+        not strippedType.containsTypeParameters() and
+        value = false
+      )
+    )
+  )
 }
 
 /** A control flow element that is inside a `try` block. */
@@ -325,12 +405,10 @@ private predicate inNullnessContext(Expr e, boolean isNullnessCompletionForParen
  * Holds if a normal completion of `e` must be a matching completion. Thats is,
  * whether `e` determines a match in a `switch` statement.
  */
-private predicate mustHaveMatchingCompletion(Expr e) {
-  exists(SwitchStmt ss |
-    e = ss.getAConstCase().getExpr()
-    or
-    e = ss.getATypeCase().getTypeAccess() // use type access to represent the type test
-  )
+private predicate mustHaveMatchingCompletion(SwitchStmt ss, Expr e) {
+  e = ss.getAConstCase().getExpr()
+  or
+  e = ss.getATypeCase().getTypeAccess() // use type access to represent the type test
 }
 
 /**