C#: Address review comments and update tests.

Author: calumgrant

csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll

@@ -657,7 +657,7 @@ module Internal {
         not mc.isConditional()
       )
     or
-    e.(DefaultValueExpr).getType() instanceof ValueType
+    e.(DefaultValueExpr).getType().isValueType()
   }
 
   /** Holds if expression `e2` is a non-`null` value whenever `e1` is. */

csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll

@@ -59,7 +59,7 @@ class AlwaysNullExpr extends Expr {
     or
     this.(Cast).getExpr() instanceof AlwaysNullExpr
     or
-    this instanceof DefaultValueExpr and this.getType() instanceof RefType
+    this instanceof DefaultValueExpr and this.getType().isRefType()
     or
     exists(Callable target |
       this.(Call).getTarget() = target and
@@ -83,7 +83,8 @@ class NonNullExpr extends Expr {
     exists(Callable target |
       this.(Call).getTarget() = target and
       not target.(Virtualizable).isVirtual() and
-      alwaysNotNullCallable(target)
+      alwaysNotNullCallable(target) and
+      not this.(QualifiableExpr).isConditional()
     )
   }
 }

csharp/ql/src/semmle/code/csharp/dataflow/internal/CallableReturns.qll

@@ -1,5 +1,5 @@
 /**
- * Provides predicates for analysing the return values of callables.
+ * Provides predicates for analyzing the return values of callables.
  */
 
 import csharp
@@ -11,14 +11,14 @@ private import semmle.code.cil.CallableReturns as CR
 predicate alwaysNullCallable(Callable c) {
   exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNullMethod(m))
   or
-  forex(Expr e | callableReturns(c, e) | e instanceof AlwaysNullExpr)
+  forex(Expr e | c.canReturn(e) | e instanceof AlwaysNullExpr)
 }
 
 /** Holds if callable `c` always returns a non-null value. */
 predicate alwaysNotNullCallable(Callable c) {
   exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNotNullMethod(m))
   or
-  forex(Expr e | callableReturns(c, e) | e instanceof NonNullExpr)
+  forex(Expr e | c.canReturn(e) | e instanceof NonNullExpr)
 }
 
 /** Holds if callable 'c' always throws an exception. */
@@ -39,12 +39,3 @@ predicate alwaysThrowsException(Callable c, Class ex) {
   exists(CIL::Method m, CIL::Type t | m.matchesHandle(c) | CR::alwaysThrowsException(m, t) and t.matchesHandle(ex))
 }
 
-/** Holds if callable `m' can return expression `ret` directly or indirectly. */
-private predicate callableReturns(Callable m, Expr e) {
-  exists(DataFlow::Node srcNode, DataFlow::Node retNode |
-    e = srcNode.asExpr() and m.canReturn(retNode.asExpr())
-  |
-    DataFlow::localFlow(srcNode, retNode)
-  )
-}
-

csharp/ql/test/library-tests/cil/dataflow/DataFlow.expected

@@ -5,3 +5,6 @@
 | dataflow.cs:20:45:20:53 | "tainted" | dataflow.cs:20:18:20:54 | call to method GetFullPath |
 | dataflow.cs:27:44:27:46 | 1 | dataflow.cs:27:18:27:52 | call to method IEEERemainder |
 | dataflow.cs:27:49:27:51 | 2 | dataflow.cs:27:18:27:52 | call to method IEEERemainder |
+| dataflow.cs:64:30:64:33 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |
+| dataflow.cs:64:30:64:33 | null | dataflow.cs:70:20:70:33 | call to method IndirectNull |
+| dataflow.cs:71:23:71:26 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |

csharp/ql/test/library-tests/cil/dataflow/Nullness.expected

@@ -1,9 +1,15 @@
 alwaysNull
 | dataflow.cs:54:21:54:35 | default(...) |
 | dataflow.cs:56:27:56:56 | access to property DeclaringMethod |
+| dataflow.cs:58:39:58:52 | call to method IndirectNull |
 alwaysNotNull
 | dataflow.cs:55:23:55:34 | default(...) |
 | dataflow.cs:56:27:56:30 | this access |
 | dataflow.cs:56:27:56:40 | call to method GetType |
 | dataflow.cs:57:30:57:33 | true |
 | dataflow.cs:57:30:57:44 | call to method ToString |
+| dataflow.cs:58:21:58:34 | this access |
+| dataflow.cs:58:39:58:52 | this access |
+| dataflow.cs:61:23:61:26 | this access |
+| dataflow.cs:61:30:61:43 | this access |
+| dataflow.cs:61:47:61:50 | this access |

csharp/ql/test/library-tests/cil/dataflow/TaintTracking.expected

@@ -11,3 +11,8 @@
 | dataflow.cs:27:49:27:51 | 2 | dataflow.cs:27:18:27:52 | call to method IEEERemainder |
 | dataflow.cs:30:60:30:60 | 1 | dataflow.cs:30:18:30:80 | call to method DivRem |
 | dataflow.cs:30:63:30:63 | 2 | dataflow.cs:30:18:30:80 | call to method DivRem |
+| dataflow.cs:64:30:64:33 | null | dataflow.cs:58:21:58:52 | ... ?? ... |
+| dataflow.cs:64:30:64:33 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |
+| dataflow.cs:64:30:64:33 | null | dataflow.cs:70:20:70:33 | call to method IndirectNull |
+| dataflow.cs:71:23:71:26 | null | dataflow.cs:58:21:58:52 | ... ?? ... |
+| dataflow.cs:71:23:71:26 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |

csharp/ql/test/library-tests/cil/dataflow/dataflow.cs

@@ -55,5 +55,20 @@ void Nullness()
         var nonNull = default(int);
         var nullFromCil = this.GetType().DeclaringMethod;
         var nonNullFromCil = true.ToString();
+        var null2 = NullFunction() ?? IndirectNull();
+
+        // The following are not always null:
+        var notNull = cond ? NullFunction() : this;
+    }
+
+    object IndirectNull() => null;
+
+    bool cond;
+
+    object NullFunction()
+    {
+        object x = IndirectNull();
+        if (cond) x = null;
+        return x;
     }
 }