C#: Fix bug in guards logic for foreach loops

hvitved · hvitved · commit b205702853e1 · 2020-08-28T15:19:11.000+02:00
diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
@@ -212,7 +212,12 @@ module AbstractValues {
         c.isValidFor(cfe) and
         foreachEmptiness(fs, cfe) and
         e = fs.getIterableExpr()
-      )
+      ) and
+      // Only when taking the non-empty successor do we know that the original iterator
+      // expression was non-empty. When taking the empty successor, we may have already
+      // iterated through the `foreach` loop zero or more times, hence the iterator
+      // expression can be both empty and non-empty
+      this.isNonEmpty()
     }
 
     override EmptyCollectionValue getDualValue() {
diff --git a/csharp/ql/test/library-tests/controlflow/guards/Collections.cs b/csharp/ql/test/library-tests/controlflow/guards/Collections.cs
@@ -100,7 +100,7 @@ void M10(string[] args)
     {
         foreach (var arg in args)
             ;
-        Console.WriteLine(args); // INCORRECT: guarded by `args` being empty
+        Console.WriteLine(args);
     }
 }
 
diff --git a/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.expected b/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.expected
@@ -38,7 +38,6 @@
 | Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
 | Collections.cs:68:13:68:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
 | Collections.cs:96:31:96:34 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | non-empty |
-| Collections.cs:103:27:103:30 | access to parameter args | Collections.cs:101:29:101:32 | access to parameter args | Collections.cs:101:29:101:32 | access to parameter args | empty |
 | Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |
 | Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:24 | ... == ... | Guards.cs:10:16:10:16 | access to parameter s | false |
 | Guards.cs:14:31:14:31 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |
diff --git a/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected b/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected
@@ -38,7 +38,6 @@
 | Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
 | Collections.cs:68:13:68:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
 | Collections.cs:96:31:96:34 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | non-empty |
-| Collections.cs:103:27:103:30 | access to parameter args | Collections.cs:101:29:101:32 | access to parameter args | Collections.cs:101:29:101:32 | access to parameter args | empty |
 | Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |
 | Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:24 | ... == ... | Guards.cs:10:16:10:16 | access to parameter s | false |
 | Guards.cs:14:31:14:31 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@ void M10(string[] args)`
`100`	`100`	`{`
`101`	`101`	`foreach (var arg in args)`
`102`	`102`	`;`
`103`		- Console.WriteLine(args); // INCORRECT: guarded by `args` being empty
	`103`	`+ Console.WriteLine(args);`
`104`	`104`	`}`
`105`	`105`	`}`
`106`	`106`