Merge pull request #1187 from calumgrant/cs/expression-null

C#: Fix FP in cs/constant-condition

Author: hvitved

change-notes/1.21/analysis-csharp.md

@@ -5,6 +5,7 @@
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads (`cs/thread-unsafe-icryptotransform-field-in-class`) | Fewer false positive results | The criteria for a result has changed to include nested properties, nested fields and collections. The format of the alert message has changed to highlight the static field. |
+| Constant condition (`cs/constant-condition`) | Fewer false positive results | Results have been removed where the `null` value is in a conditional expression on the left hand side of a null-coalescing expression. For example, in `(a ? b : null) ?? c`, `null` is not considered to be a constant condition. |
 
 ## Changes to code extraction
 

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

@@ -71,8 +71,10 @@ class ConstantNullnessCondition extends ConstantCondition {
 
   ConstantNullnessCondition() {
     forex(ControlFlow::Node cfn | cfn = this.getAControlFlowNode() |
-      exists(ControlFlow::SuccessorTypes::NullnessSuccessor t | exists(cfn.getASuccessorByType(t)) |
-        b = t.getValue()
+      exists(ControlFlow::SuccessorTypes::NullnessSuccessor t, ControlFlow::Node s |
+        s = cfn.getASuccessorByType(t) |
+        b = t.getValue() and
+        not s.isJoin()
       ) and
       strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1
     )

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

@@ -50,6 +50,8 @@ void M2(int i)
         j = (int?)i ?? 1; // BAD
         s = ""?.CommaJoinWith(s); // BAD
         s = s ?? ""; // GOOD
+        s = (i==0 ? s : null) ?? s; // GOOD
+        var k = (i==0 ? s : null)?.Length; // GOOD
     }
 }
 

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

@@ -4,9 +4,9 @@
 | 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. |
+| ConstantCondition.cs:64:18:64:18 | 2 | Pattern never matches. |
+| ConstantCondition.cs:66:18:66:18 | 3 | Pattern always matches. |
+| ConstantCondition.cs:77:18:77: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'. |