From 41ad940adf3da52ad444da61d2a3350eb66e63a0 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 10:16:01 -0400 Subject: [PATCH 1/9] fix: missed where opporunity false positive --- csharp/ql/lib/Linq/Helpers.qll | 20 +++++++- .../ql/src/Linq/MissedWhereOpportunity.qhelp | 27 ++++++---- .../ql/src/Linq/MissedWhereOpportunityGood.cs | 33 ++++++++++++ .../MissedWhereOpportunity.cs | 50 +++++++++++++++++++ .../MissedWhereOpportunity.expected | 1 + 5 files changed, 121 insertions(+), 10 deletions(-) create mode 100644 csharp/ql/src/Linq/MissedWhereOpportunityGood.cs diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 2a4d5c8c27a2..4d0c30e55f1a 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/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()) ) } diff --git a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp index 6b22d1a14edc..a1f98a76e6d1 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 by exiting the current +method or iterator, such as with a full 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 examples 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..da28fd6e79f1 --- /dev/null +++ b/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs @@ -0,0 +1,33 @@ +class MissedWhereOpportunityGood +{ + public int? FindFirstEven(System.Collections.Generic.IEnumerable values) + { + foreach (int value in values) + { + if (value % 2 == 0) + return value; + } + + return null; + } + + public System.Collections.Generic.IEnumerable ValuesUntilFirstEven(System.Collections.Generic.IEnumerable values) + { + foreach (int value in values) + { + if (value % 2 == 0) + yield break; + + yield return value; + } + } + + public void ThrowOnFirstEven(System.Collections.Generic.IEnumerable values) + { + foreach (int value in values) + { + if (value % 2 == 0) + throw new System.InvalidOperationException("Unexpected even value."); + } + } +} diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs index 0fee1e9c48ff..abee5da38f9d 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -76,6 +76,56 @@ 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 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 | From dc42a4c645b97d20bc8ee5c53727068a5a40ecb9 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 10:23:54 -0400 Subject: [PATCH 2/9] docs: rewording Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- csharp/ql/src/Linq/MissedWhereOpportunity.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp index a1f98a76e6d1..f8f2769b0812 100644 --- a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp +++ b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp @@ -10,10 +10,10 @@ testing the variable each iteration to determine whether or not it is even. This 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 by exiting the current -method or iterator, such as with a full return, yield break, or -throw. In those cases the loop is searching for a terminal condition rather than -filtering the remaining loop body.

+

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.

From 17412f33db75e9688b9977d80bafa119846291ee Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 10:50:59 -0400 Subject: [PATCH 3/9] tests: adds a unit test to prevent regression on else cases --- .../MissedWhereOpportunity.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs index abee5da38f9d..192e9893c955 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -126,6 +126,42 @@ public IEnumerable M9(IEnumerable elements) } // $ 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 class NonEnumerableClass { public IEnumerator GetEnumerator() => throw null; From 327158734400bdfb1a1387787d88c560122c2519 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 10:58:11 -0400 Subject: [PATCH 4/9] chore: fixes missing break case Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- csharp/ql/lib/Linq/Helpers.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 4d0c30e55f1a..2909c6696a73 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -27,6 +27,8 @@ private predicate terminatesCallable(Stmt s) { or s.stripSingletonBlocks() instanceof ThrowStmt or + s.stripSingletonBlocks() instanceof BreakStmt + or exists(BlockStmt b | b = s.stripSingletonBlocks() | terminatesCallable(b.getLastStmt())) or exists(IfStmt nested | From 5e11c9aeb65fbba2432107ad9603c525d1bd90be Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 13:09:33 -0400 Subject: [PATCH 5/9] tests: adds a negative test for break --- .../MissedWhereOpportunity/MissedWhereOpportunity.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs index 192e9893c955..7b9d35821299 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -162,6 +162,18 @@ public int M11(IEnumerable elements) 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; From dd005e51f3027230dc5611dd982b3fc0c39261d3 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 08:15:06 -0400 Subject: [PATCH 6/9] chore: applies review suggestion Co-authored-by: Michael Nebel --- csharp/ql/lib/Linq/Helpers.qll | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 2909c6696a73..ad4e83e91ba6 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -21,21 +21,23 @@ private int numStmts(ForeachStmt fes) { } private predicate terminatesCallable(Stmt s) { - s.stripSingletonBlocks() instanceof ReturnStmt - or - s.stripSingletonBlocks() instanceof YieldBreakStmt - or - s.stripSingletonBlocks() instanceof ThrowStmt - or - s.stripSingletonBlocks() instanceof BreakStmt - 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()) + 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 | + exists(nested.getElse()) and + terminatesCallable(nested.getThen()) and + terminatesCallable(nested.getElse()) + ) ) } From d6ffc18f790e52831a546c0b1bafc91284be7da2 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 09:12:32 -0400 Subject: [PATCH 7/9] docs: removes additional cases to avoid the page being too long Signed-off-by: Vincent Biret --- .../ql/src/Linq/MissedWhereOpportunity.qhelp | 2 +- .../ql/src/Linq/MissedWhereOpportunityGood.cs | 20 ------------------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp index f8f2769b0812..53ac540441c4 100644 --- a/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp +++ b/csharp/ql/src/Linq/MissedWhereOpportunity.qhelp @@ -31,7 +31,7 @@ on the even ones.

This is far better expressed using the Where method.

-

The following examples should not use Where, because the matching branch exits the +

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 index da28fd6e79f1..b96db876583a 100644 --- a/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs +++ b/csharp/ql/src/Linq/MissedWhereOpportunityGood.cs @@ -10,24 +10,4 @@ class MissedWhereOpportunityGood return null; } - - public System.Collections.Generic.IEnumerable ValuesUntilFirstEven(System.Collections.Generic.IEnumerable values) - { - foreach (int value in values) - { - if (value % 2 == 0) - yield break; - - yield return value; - } - } - - public void ThrowOnFirstEven(System.Collections.Generic.IEnumerable values) - { - foreach (int value in values) - { - if (value % 2 == 0) - throw new System.InvalidOperationException("Unexpected even value."); - } - } } From 2a7faa5e781cf9694e573a5e386e7bad028dcfc3 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Thu, 3 Sep 2026 07:05:11 -0400 Subject: [PATCH 8/9] chore: applies review suggestion Co-authored-by: Michael Nebel --- csharp/ql/lib/Linq/Helpers.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index ad4e83e91ba6..fcbc01c5e35d 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -34,7 +34,6 @@ private predicate terminatesCallable(Stmt s) { or stripped = any(IfStmt nested | - exists(nested.getElse()) and terminatesCallable(nested.getThen()) and terminatesCallable(nested.getElse()) ) From dd84d7f14473cb1f025259f8c4c935b7e86b8697 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Thu, 3 Sep 2026 07:22:40 -0400 Subject: [PATCH 9/9] docs: adds change note for linq where fixes --- .../change-notes/2026-09-03-missed-where-terminal-branches.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2026-09-03-missed-where-terminal-branches.md 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.