fix: missed where opporunity false positive

Author: baywet

csharp/ql/lib/Linq/Helpers.qll

@@ -20,6 +20,23 @@ private int numStmts(ForeachStmt fes) {
   else result = 1
 }
 
+private predicate terminatesCallable(Stmt s) {
+  s.stripSingletonBlocks() instanceof ReturnStmt
+  or
+  s.stripSingletonBlocks() instanceof YieldBreakStmt
+  or
+  s.stripSingletonBlocks() instanceof ThrowStmt
+  or
+  exists(BlockStmt b | b = s.stripSingletonBlocks() | terminatesCallable(b.getLastStmt()))
+  or
+  exists(IfStmt nested |
+    nested = s.stripSingletonBlocks() and
+    exists(nested.getElse()) and
+    terminatesCallable(nested.getThen()) and
+    terminatesCallable(nested.getElse())
+  )
+}
+
 /** Holds if the type's qualified name is "System.Linq.Enumerable" */
 predicate isEnumerableType(ValueOrRefType t) {
   t.hasFullyQualifiedName("System.Linq", "Enumerable")
@@ -152,7 +169,8 @@ predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) {
     is.getThen() instanceof ContinueStmt
     or
     not exists(is.getElse()) and
-    numStmts(fes) = 1
+    numStmts(fes) = 1 and
+    not terminatesCallable(is.getThen())
   )
 }
 

csharp/ql/src/Linq/MissedWhereOpportunity.qhelp

@@ -3,29 +3,38 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Programmers sometimes need to iterative over a filtered version of a sequence, rather than the 
-sequence itself. For example, you might want to print out only the numbers in the range [1,10] that 
-are even. One standard way of doing this is to write a loop that iterates over the whole sequence, 
-testing the variable each iteration to determine whether or not it is even. This is often written 
-using either <code>if(!condition(var)) continue;</code> as the initial statement in the loop, or by 
+<p>Programmers sometimes need to iterate over a filtered version of a sequence, rather than the
+sequence itself. For example, you might want to print out only the numbers in the range [1,10] that
+are even. One standard way of doing this is to write a loop that iterates over the whole sequence,
+testing the variable each iteration to determine whether or not it is even. This is often written
+using either <code>if(!condition(var)) continue;</code> as the initial statement in the loop, or by
 enclosing the entire loop body with <code>if(condition(var))</code>.</p>
 
+<p>This recommendation does not apply when the matching branch exits the loop by exiting the current
+method or iterator, such as with a full <code>return</code>, <code>yield break</code>, or
+<code>throw</code>. In those cases the loop is searching for a terminal condition rather than
+filtering the remaining loop body.</p>
+
 </overview>
 <recommendation>
-<p>This pattern works well and is also available as the <code>Where</code> method in LINQ in C# 3.5 
-and above. It is better to use a library method in preference to writing your own pattern unless you 
-have a specific need for a custom version. In particular, this makes the code easier to read by 
+<p>This pattern works well and is also available as the <code>Where</code> method in LINQ in C# 3.5
+and above. It is better to use a library method in preference to writing your own pattern unless you
+have a specific need for a custom version. In particular, this makes the code easier to read by
 expressing the intent better and by reducing the nesting depth of the code.</p>
 
 </recommendation>
 <example>
-<p>This example shows two ways of iterating over a series of integers and only performing an action 
+<p>This example shows two ways of iterating over a series of integers and only performing an action
 on the even ones.</p>
 <sample src="MissedWhereOpportunity.cs" />
 
 <p>This is far better expressed using the <code>Where</code> method.</p>
 <sample src="MissedWhereOpportunityFix.cs" />
 
+<p>The following examples should not use <code>Where</code>, because the matching branch exits the
+method or iterator instead of continuing with filtered loop work.</p>
+<sample src="MissedWhereOpportunityGood.cs" />
+
 </example>
 <references>
 

csharp/ql/src/Linq/MissedWhereOpportunityGood.cs

@@ -0,0 +1,33 @@
+class MissedWhereOpportunityGood
+{
+    public int? FindFirstEven(System.Collections.Generic.IEnumerable<int> values)
+    {
+        foreach (int value in values)
+        {
+            if (value % 2 == 0)
+                return value;
+        }
+
+        return null;
+    }
+
+    public System.Collections.Generic.IEnumerable<int> ValuesUntilFirstEven(System.Collections.Generic.IEnumerable<int> values)
+    {
+        foreach (int value in values)
+        {
+            if (value % 2 == 0)
+                yield break;
+
+            yield return value;
+        }
+    }
+
+    public void ThrowOnFirstEven(System.Collections.Generic.IEnumerable<int> values)
+    {
+        foreach (int value in values)
+        {
+            if (value % 2 == 0)
+                throw new System.InvalidOperationException("Unexpected even value.");
+        }
+    }
+}

csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs

@@ -76,6 +76,56 @@ public void M5(IEnumerable<int> elements)
         } // $ Alert
     }
 
+    public int M6(IEnumerable<int> elements)
+    {
+        // GOOD: The filtered case returns from the method instead of continuing the loop.
+        foreach (var element in elements)
+        {
+            if (element.GetHashCode() % 2 == 0)
+            {
+                return element;
+            }
+        }
+
+        return 0;
+    }
+
+    public IEnumerable<int> M7(IEnumerable<int> elements)
+    {
+        // GOOD: The filtered case exits the iterator instead of continuing the loop.
+        foreach (var element in elements)
+        {
+            if (element.GetHashCode() % 2 == 0)
+            {
+                yield break;
+            }
+        }
+    }
+
+    public void M8(IEnumerable<int> elements)
+    {
+        // GOOD: The filtered case throws instead of continuing the loop.
+        foreach (var element in elements)
+        {
+            if (element.GetHashCode() % 2 == 0)
+            {
+                throw new InvalidOperationException();
+            }
+        }
+    }
+
+    public IEnumerable<int> M9(IEnumerable<int> elements)
+    {
+        // BAD: A yield return does not exit the iterator, so the loop still filters yielded values.
+        foreach (var element in elements)
+        {
+            if (element.GetHashCode() % 2 == 0)
+            {
+                yield return element;
+            }
+        } // $ Alert
+    }
+
     public class NonEnumerableClass
     {
         public IEnumerator<int> GetEnumerator() => throw null;

csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected

@@ -2,3 +2,4 @@
 | MissedWhereOpportunity.cs:19:9:26:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:21:17:21:26 | ... == ... | implicitly filters its target sequence |
 | MissedWhereOpportunity.cs:45:9:52:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:47:17:47:26 | ... == ... | implicitly filters its target sequence |
 | MissedWhereOpportunity.cs:70:9:76:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:72:17:72:46 | ... == ... | implicitly filters its target sequence |
+| MissedWhereOpportunity.cs:120:9:126:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:122:17:122:46 | ... == ... | implicitly filters its target sequence |