diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 2a4d5c8c27a2..fcbc01c5e35d 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -20,6 +20,26 @@ private int numStmts(ForeachStmt fes) { else result = 1 } +private predicate terminatesCallable(Stmt s) { + exists(Stmt stripped | stripped = s.stripSingletonBlocks() | + stripped instanceof ReturnStmt + or + stripped instanceof YieldBreakStmt + or + stripped instanceof ThrowStmt + or + stripped instanceof BreakStmt + or + stripped = any(BlockStmt b | terminatesCallable(b.getLastStmt())) + or + stripped = + any(IfStmt nested | + 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 +172,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()) ) } diff --git a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp index 6b22d1a14edc..53ac540441c4 100644 --- a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp +++ b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp @@ -3,29 +3,38 @@ "qhelp.dtd"> -

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 if(!condition(var)) continue; as the initial statement in the loop, or by +

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 if(!condition(var)) continue; as the initial statement in the loop, or by enclosing the entire loop body with if(condition(var)).

+

This recommendation does not apply when the matching branch exits the loop without continuing to +later iterations, such as with return, yield break, or throw. +In those cases the loop is searching for a terminal condition rather than filtering the remaining +loop body.

+
-

This pattern works well and is also available as the Where 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 +

This pattern works well and is also available as the Where 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.

-

This example shows two ways of iterating over a series of integers and only performing an action +

This example shows two ways of iterating over a series of integers and only performing an action on the even ones.

This is far better expressed using the Where method.

+

The following example should not use Where, because the matching branch exits the +method or iterator instead of continuing with filtered loop work.

+ +
diff --git a/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs b/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs new file mode 100644 index 000000000000..b96db876583a --- /dev/null +++ b/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs @@ -0,0 +1,13 @@ +class MissedWhereOpportunityGood +{ + public int? FindFirstEven(System.Collections.Generic.IEnumerable values) + { + foreach (int value in values) + { + if (value % 2 == 0) + return value; + } + + return null; + } +} diff --git a/csharp/ql/src/change-notes/2026-09-03-missed-where-terminal-branches.md b/csharp/ql/src/change-notes/2026-09-03-missed-where-terminal-branches.md new file mode 100644 index 000000000000..33515e4c90cd --- /dev/null +++ b/csharp/ql/src/change-notes/2026-09-03-missed-where-terminal-branches.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `cs/linq/missed-where` query no longer flags `foreach` loops where the matching branch terminates the method, iterator, or loop instead of continuing with filtered loop work. diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs index 0fee1e9c48ff..7b9d35821299 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -76,6 +76,104 @@ public void M5(IEnumerable elements) } // $ Alert } + public int M6(IEnumerable 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 M7(IEnumerable 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 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 M9(IEnumerable 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 int M10(IEnumerable elements) + { + // GOOD: The filtered case ends with a return from the method instead of continuing the loop. + foreach (var element in elements) + { + if (element.GetHashCode() % 2 == 0) + { + Console.WriteLine(element); + return element; + } + } + + return 0; + } + + public int M11(IEnumerable elements) + { + // GOOD: Both nested filtered cases return from the method instead of continuing the loop. + foreach (var element in elements) + { + if (element.GetHashCode() % 2 == 0) + { + if (element > 10) + { + return element; + } + else + { + return 10; + } + } + } + + return 0; + } + + public void M12(IEnumerable elements) + { + // GOOD: The filtered case exits the loop instead of continuing with filtered loop work. + foreach (var element in elements) + { + if (element.GetHashCode() % 2 == 0) + { + break; + } + } + } + public class NonEnumerableClass { public IEnumerator GetEnumerator() => throw null; diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected index 5efde9aebedc..8b9398292ef2 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected +++ b/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 |