Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion csharp/ql/lib/Linq/Helpers.qll
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,26 @@ private int numStmts(ForeachStmt fes) {
else result = 1
}

private predicate terminatesCallable(Stmt s) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

terminatesCallable is recursive and has no base case for loops or switch statements.

Impact: terminatesCallable is recursive and has no base case for loops or switch statements. A foreach/while/for/do loop whose body contains a return will not be recognized as terminating, causing false positives. More critically, the recursion through BlockStmt.getLastStmt() and IfStmt branches can cycle or fail to terminate on malformed/cyclic ASTs, and the predicate lacks a recursion bound, risking evaluator non-terminat…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop.

Impact: The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop. A future maintainer reading missedWhereOpportunity will assume the predicate means method/iterator termination and will not realize break is included, leading to incorrect modifications.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, B

Impact: The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, BreakStmt) without documenting the intended control-flow semantics. There is no comment explaining why BreakStmt is included or why loops are excluded, making the logic opaque to a newcomer.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The predicate does not handle ContinueStmt, GotoStmt, or labeled statements.

Impact: The predicate does not handle ContinueStmt, GotoStmt, or labeled statements. A goto to a label outside the loop or a continue in a nested loop can terminate or alter control flow in ways the predicate misclassifies, leading to incorrect alert suppression or missed alerts.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling.

Impact: The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling. A try/catch/finally block where the try contains a return but the finally throws or returns is not modeled, so the query may suppress alerts for loops whose filtered branch does not actually terminate the callable, or flag loops that do. This could hide real code-quality…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

exists(Stmt stripped | stripped = s.stripSingletonBlocks() |
stripped instanceof ReturnStmt
or
stripped instanceof YieldBreakStmt
or
stripped instanceof ThrowStmt
or
stripped instanceof BreakStmt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator.

Impact: The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator. In a foreach nested inside another loop, a break in the filtered branch exits only the inner foreach and continues the outer loop, so the loop is still filtering work and should be flagged. This will suppress true positives.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

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")
Expand Down Expand Up @@ -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())
)
}

Expand Down
27 changes: 18 additions & 9 deletions csharp/ql/src/Linq/MissedWhereOpportunity.qhelp
Original file line number Diff line number Diff line change
Expand Up @@ -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 without continuing to
later iterations, such as with <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 example 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>

Expand Down
13 changes: 13 additions & 0 deletions csharp/ql/src/Linq/MissedWhereOpportunityGood.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class MissedWhereOpportunityGood
{
public int? FindFirstEven(System.Collections.Generic.IEnumerable<int> values)
{
foreach (int value in values)
{
if (value % 2 == 0)
return value;
}

return null;
}
}
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,104 @@ 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 int M10(IEnumerable<int> 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<int> 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<int> 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<int> GetEnumerator() => throw null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 |