C#: Remove more FPs in cs/dereferenced-value-may-be-null

Author: hvitved

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

@@ -197,43 +197,44 @@ private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullE
 
 /** Holds if `def` is an SSA definition that may be `null`. */
 private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason) {
-  // A variable compared to `null` might be `null`
-  exists(G::DereferenceableExpr de | de = def.getARead() |
-    reason = de.getANullCheck(_, true) and
-    msg = "as suggested by $@ null check" and
-    not de = any(Ssa::PseudoDefinition pdef).getARead() and
-    strictcount(Element e | e = any(Ssa::Definition def0 | de = def0.getARead()).getElement()) = 1 and
-    not nonNullDef(def) and
-    // Don't use a check as reason if there is a `null` assignment
-    // or argument
-    not def.(Ssa::ExplicitDefinition).getADefinition().getSource() instanceof MaybeNullExpr and
-    not isMaybeNullArgument(def, _)
-  )
-  or
-  // A parameter might be `null` if there is a `null` argument somewhere
-  isMaybeNullArgument(def, reason) and
+  not nonNullDef(def) and
   (
-    if reason instanceof AlwaysNullExpr
-    then msg = "because of $@ null argument"
-    else msg = "because of $@ potential null argument"
-  )
-  or
-  isNullDefaultArgument(def, reason) and msg = "because the parameter has a null default value"
-  or
-  // If the source of a variable is `null` then the variable may be `null`
-  exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
-    adef.getSource() instanceof MaybeNullExpr and
-    reason = adef.getExpr() and
-    msg = "because of $@ assignment"
-  )
-  or
-  // A variable of nullable type may be null
-  exists(Dereference d | dereferenceAt(_, _, def, d) |
-    d.hasNullableType() and
-    not def instanceof Ssa::PseudoDefinition and
-    not nonNullDef(def) and
-    reason = def.getSourceVariable().getAssignable() and
-    msg = "because it has a nullable type"
+    // A variable compared to `null` might be `null`
+    exists(G::DereferenceableExpr de | de = def.getARead() |
+      reason = de.getANullCheck(_, true) and
+      msg = "as suggested by $@ null check" and
+      not de = any(Ssa::PseudoDefinition pdef).getARead() and
+      strictcount(Element e | e = any(Ssa::Definition def0 | de = def0.getARead()).getElement()) = 1 and
+      // Don't use a check as reason if there is a `null` assignment
+      // or argument
+      not def.(Ssa::ExplicitDefinition).getADefinition().getSource() instanceof MaybeNullExpr and
+      not isMaybeNullArgument(def, _)
+    )
+    or
+    // A parameter might be `null` if there is a `null` argument somewhere
+    isMaybeNullArgument(def, reason) and
+    (
+      if reason instanceof AlwaysNullExpr
+      then msg = "because of $@ null argument"
+      else msg = "because of $@ potential null argument"
+    )
+    or
+    isNullDefaultArgument(def, reason) and msg = "because the parameter has a null default value"
+    or
+    // If the source of a variable is `null` then the variable may be `null`
+    exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
+      adef.getSource() instanceof MaybeNullExpr and
+      reason = adef.getExpr() and
+      msg = "because of $@ assignment"
+    )
+    or
+    // A variable of nullable type may be null
+    exists(Dereference d | dereferenceAt(_, _, def, d) |
+      d.hasNullableType() and
+      not def instanceof Ssa::PseudoDefinition and
+      reason = def.getSourceVariable().getAssignable() and
+      msg = "because it has a nullable type"
+    )
   )
 }
 

csharp/ql/test/query-tests/Nullness/E.cs

@@ -409,7 +409,7 @@ int Ex41()
     {
         int? i = 1;
         i ??= null;
-        return i.Value; // GOOD (false positive)
+        return i.Value; // GOOD
     }
 }
 

csharp/ql/test/query-tests/Nullness/NullMaybe.expected

@@ -373,8 +373,6 @@ nodes
 | E.cs:404:9:404:18 | SSA def(i) |
 | E.cs:404:9:404:18 | SSA def(i) |
 | E.cs:405:16:405:16 | access to local variable i |
-| E.cs:411:9:411:18 | SSA def(i) |
-| E.cs:412:16:412:16 | access to local variable i |
 | Forwarding.cs:7:16:7:23 | SSA def(s) |
 | Forwarding.cs:14:9:17:9 | if (...) ... |
 | Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -726,7 +724,6 @@ edges
 | E.cs:384:27:384:28 | access to parameter e2 | E.cs:386:16:386:17 | access to parameter e1 |
 | E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
 | E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
-| E.cs:411:9:411:18 | SSA def(i) | E.cs:412:16:412:16 | access to local variable i |
 | Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
 | Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
 | Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |
@@ -835,7 +832,6 @@ edges
 | E.cs:386:27:386:28 | access to parameter e2 | E.cs:380:30:380:31 | SSA param(e2) | E.cs:386:27:386:28 | access to parameter e2 | Variable $@ may be null here as suggested by $@ null check. | E.cs:380:30:380:31 | e2 | e2 | E.cs:382:28:382:37 | ... != ... | this |
 | E.cs:386:27:386:28 | access to parameter e2 | E.cs:380:30:380:31 | SSA param(e2) | E.cs:386:27:386:28 | access to parameter e2 | Variable $@ may be null here as suggested by $@ null check. | E.cs:380:30:380:31 | e2 | e2 | E.cs:382:58:382:67 | ... == ... | this |
 | E.cs:386:27:386:28 | access to parameter e2 | E.cs:380:30:380:31 | SSA param(e2) | E.cs:386:27:386:28 | access to parameter e2 | Variable $@ may be null here as suggested by $@ null check. | E.cs:380:30:380:31 | e2 | e2 | E.cs:384:27:384:36 | ... == ... | this |
-| E.cs:412:16:412:16 | access to local variable i | E.cs:411:9:411:18 | SSA def(i) | E.cs:412:16:412:16 | access to local variable i | Variable $@ may be null here because of $@ assignment. | E.cs:410:14:410:14 | i | i | E.cs:411:9:411:18 | ... = ... | this |
 | GuardedString.cs:35:31:35:31 | access to local variable s | GuardedString.cs:7:16:7:32 | SSA def(s) | GuardedString.cs:35:31:35:31 | access to local variable s | Variable $@ may be null here because of $@ assignment. | GuardedString.cs:7:16:7:16 | s | s | GuardedString.cs:7:16:7:32 | String s = ... | this |
 | NullMaybeBad.cs:7:27:7:27 | access to parameter o | NullMaybeBad.cs:13:17:13:20 | null | NullMaybeBad.cs:7:27:7:27 | access to parameter o | Variable $@ may be null here because of $@ null argument. | NullMaybeBad.cs:5:25:5:25 | o | o | NullMaybeBad.cs:13:17:13:20 | null | this |
 | StringConcatenation.cs:16:17:16:17 | access to local variable s | StringConcatenation.cs:14:16:14:23 | SSA def(s) | StringConcatenation.cs:16:17:16:17 | access to local variable s | Variable $@ may be null here because of $@ assignment. | StringConcatenation.cs:14:16:14:16 | s | s | StringConcatenation.cs:14:16:14:23 | String s = ... | this |