Skip to content

Commit ed15edf

Browse files
baywetbaywet
committed
feat(csharp): adds a missed first or default opprtunity rule
1 parent 7b2695b commit ed15edf

10 files changed

Lines changed: 291 additions & 0 deletions

‎csharp/ql/lib/Linq/Helpers.qll‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,17 @@ private int numStmts(ForeachStmt fes) {
2020
else result = 1
2121
}
2222

23+
private predicate returnsLoopVariable(ForeachStmt fes, Stmt s, ReturnStmt ret) {
24+
ret = s.stripSingletonBlocks() and
25+
ret.getExpr().stripCasts().(VariableAccess).getTarget() = fes.getVariable()
26+
}
27+
28+
private predicate returnsDefaultValue(ReturnStmt ret) {
29+
ret.getExpr().stripCasts() instanceof NullLiteral
30+
or
31+
ret.getExpr().stripCasts() instanceof DefaultValueExpr
32+
}
33+
2334
/** Holds if the type's qualified name is "System.Linq.Enumerable" */
2435
predicate isEnumerableType(ValueOrRefType t) {
2536
t.hasFullyQualifiedName("System.Linq", "Enumerable")
@@ -156,6 +167,32 @@ predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) {
156167
)
157168
}
158169

170+
/**
171+
* Holds if `foreach` statement `fes` could be converted to a `.FirstOrDefault()` call.
172+
* That is, the loop contains a single `if` statement that accesses the loop variable,
173+
* returns the loop variable when the condition matches, and is followed by a default return.
174+
*/
175+
predicate missedFirstOrDefaultOpportunity(
176+
ForeachStmtGenericEnumerable fes, IfStmt is, ReturnStmt ret, ReturnStmt defaultRet
177+
) {
178+
// The loop only checks whether the current element is the first match.
179+
is = firstStmt(fes) and
180+
not exists(is.getElse()) and
181+
numStmts(fes) = 1 and
182+
exists(VariableAccess va |
183+
va.getTarget() = fes.getVariable() and
184+
va = is.getCondition().getAChildExpr*()
185+
) and
186+
not is.getCondition().getAChildExpr*() instanceof AwaitExpr and
187+
returnsLoopVariable(fes, is.getThen(), ret) and
188+
// If no element matches, the method returns the same value that FirstOrDefault would.
189+
returnsDefaultValue(defaultRet) and
190+
exists(BlockStmt enclosingBlock, int i |
191+
enclosingBlock.getStmt(i) = fes and
192+
enclosingBlock.getStmt(i + 1) = defaultRet
193+
)
194+
}
195+
159196
//#################### CLASSES ####################
160197
/** A LINQ Any(...) call. */
161198
class AnyCall extends MethodCall {

‎csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
class MissedFirstOrDefaultOpportunity
2+
{
3+
public static Operation FindOperation(System.Collections.Generic.IEnumerable<Operation> operations, string operationId)
4+
{
5+
foreach (var operation in operations)
6+
{
7+
if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal))
8+
return operation;
9+
}
10+
11+
return null;
12+
}
13+
}
14+
15+
class Operation
16+
{
17+
public string OperationId { get; set; }
18+
}

‎csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Programmers sometimes search a sequence by iterating over each element, testing it, and returning
7+
the first element that satisfies the test. If the loop completes without finding a match, the method
8+
then returns a default value such as <code>null</code> or <code>default</code>.</p>
9+
10+
</overview>
11+
<recommendation>
12+
<p>This pattern is directly available as the <code>FirstOrDefault</code> method in LINQ. Using the
13+
library method makes the search intent explicit and avoids manually spelling out the loop and
14+
fallback return.</p>
15+
16+
</recommendation>
17+
<example>
18+
<p>In this example the method searches a list of operations for the first operation with a matching
19+
identifier, returning <code>null</code> if no match is found.</p>
20+
<sample src="MissedFirstOrDefaultOpportunity.cs" />
21+
22+
<p>The LINQ <code>FirstOrDefault</code> method can express this search more directly.</p>
23+
<sample src="MissedFirstOrDefaultOpportunityFix.cs" />
24+
25+
<p>The following examples should not use <code>FirstOrDefault</code>, because they do more than
26+
return the matching element or because the fallback value is not the default value.</p>
27+
<sample src="MissedFirstOrDefaultOpportunityGood.cs" />
28+
29+
</example>
30+
<references>
31+
32+
<li>MSDN: <a href="https://learn.microsoft.com/dotnet/api/system.linq.enumerable.firstordefault">Enumerable.FirstOrDefault Method</a>.</li>
33+
34+
35+
</references>
36+
</qhelp>

‎csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Missed opportunity to use FirstOrDefault
3+
* @description The intent of a foreach loop that returns the first sequence element satisfying a predicate, or a default value otherwise,
4+
* can often be better expressed using LINQ's 'FirstOrDefault' method.
5+
* @kind problem
6+
* @problem.severity recommendation
7+
* @precision high
8+
* @id cs/linq/missed-firstordefault
9+
* @tags quality
10+
* maintainability
11+
* readability
12+
* language-features
13+
*/
14+
15+
import csharp
16+
import Linq.Helpers
17+
18+
from ForeachStmtGenericEnumerable fes, IfStmt is, ReturnStmt ret, ReturnStmt defaultRet
19+
where missedFirstOrDefaultOpportunity(fes, is, ret, defaultRet)
20+
select fes,
21+
"This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.",
22+
is.getCondition(), "returns the first sequence element satisfying a predicate"

‎csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
class MissedFirstOrDefaultOpportunityFix
2+
{
3+
public static Operation FindOperation(System.Collections.Generic.IEnumerable<Operation> operations, string operationId)
4+
{
5+
return operations.FirstOrDefault(operation =>
6+
string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal));
7+
}
8+
}

‎csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
class MissedFirstOrDefaultOpportunityGood
2+
{
3+
public static Operation FindOperationOrThrow(System.Collections.Generic.IEnumerable<Operation> operations, string operationId)
4+
{
5+
foreach (var operation in operations)
6+
{
7+
if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal))
8+
throw new System.InvalidOperationException("Unexpected operation.");
9+
}
10+
11+
return null;
12+
}
13+
14+
public static Operation FindReplacementOperation(System.Collections.Generic.IEnumerable<Operation> operations, string operationId)
15+
{
16+
foreach (var operation in operations)
17+
{
18+
if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal))
19+
return operation;
20+
}
21+
22+
return new Operation();
23+
}
24+
25+
public static string FindOperationId(System.Collections.Generic.IEnumerable<Operation> operations, string operationId)
26+
{
27+
foreach (var operation in operations)
28+
{
29+
if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal))
30+
return operation.OperationId;
31+
}
32+
33+
return null;
34+
}
35+
}

‎csharp/ql/src/codeql-suites/csharp-security-and-quality.qls‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
- cs/linq/inconsistent-enumeration
5252
- cs/linq/missed-all
5353
- cs/linq/missed-cast
54+
- cs/linq/missed-firstordefault
5455
- cs/linq/missed-oftype
5556
- cs/linq/missed-select
5657
- cs/linq/missed-where

‎csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Threading.Tasks;
4+
5+
class MissedFirstOrDefaultOpportunity
6+
{
7+
public Operation M1(IEnumerable<Operation> operations, string operationId)
8+
{
9+
// BAD: Can be replaced with operations.FirstOrDefault(operation => ...).
10+
foreach (var operation in operations)
11+
{
12+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
13+
return operation;
14+
} // $ Alert
15+
16+
return null;
17+
}
18+
19+
public int M2(IEnumerable<int> values)
20+
{
21+
// BAD: Can be replaced with values.FirstOrDefault(value => ...).
22+
foreach (var value in values)
23+
{
24+
if (value > 0)
25+
{
26+
return value;
27+
}
28+
} // $ Alert
29+
30+
return default;
31+
}
32+
33+
public int? M3(List<int> values)
34+
{
35+
// BAD: Can be replaced with values.FirstOrDefault(value => ...).
36+
foreach (var value in values)
37+
{
38+
if (value > 0)
39+
return value;
40+
} // $ Alert
41+
42+
return default(int);
43+
}
44+
45+
public Operation M4(IEnumerable<Operation> operations, string operationId)
46+
{
47+
// GOOD: FirstOrDefault does not throw when a match is found.
48+
foreach (var operation in operations)
49+
{
50+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
51+
throw new InvalidOperationException();
52+
}
53+
54+
return null;
55+
}
56+
57+
public Operation M5(IEnumerable<Operation> operations, string operationId)
58+
{
59+
// GOOD: FirstOrDefault would return null/default when no match is found.
60+
foreach (var operation in operations)
61+
{
62+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
63+
return operation;
64+
}
65+
66+
return new Operation();
67+
}
68+
69+
public string M6(IEnumerable<Operation> operations, string operationId)
70+
{
71+
// GOOD: FirstOrDefault would return the matching operation, not one of its properties.
72+
foreach (var operation in operations)
73+
{
74+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
75+
return operation.OperationId;
76+
}
77+
78+
return null;
79+
}
80+
81+
public Operation M7(IEnumerable<Operation> operations, string operationId)
82+
{
83+
// GOOD: The matched case has an additional side effect.
84+
foreach (var operation in operations)
85+
{
86+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
87+
{
88+
Console.WriteLine(operation.OperationId);
89+
return operation;
90+
}
91+
}
92+
93+
return null;
94+
}
95+
96+
public async Task<Operation> M8(IEnumerable<Operation> operations, string operationId)
97+
{
98+
// GOOD: FirstOrDefault does not support an async predicate.
99+
foreach (var operation in operations)
100+
{
101+
if (await IsMatch(operation, operationId))
102+
return operation;
103+
}
104+
105+
return null;
106+
}
107+
108+
public Operation M9(IEnumerable<Operation> operations, string operationId)
109+
{
110+
// GOOD: FirstOrDefault does not have an equivalent for an else branch in the loop.
111+
foreach (var operation in operations)
112+
{
113+
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
114+
return operation;
115+
else
116+
return null;
117+
}
118+
119+
return null;
120+
}
121+
122+
private static Task<bool> IsMatch(Operation operation, string operationId) =>
123+
Task.FromResult(string.Equals(operation.OperationId, operationId, StringComparison.Ordinal));
124+
}
125+
126+
class Operation
127+
{
128+
public string OperationId { get; set; }
129+
}

‎csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
| MissedFirstOrDefaultOpportunity.cs:10:9:14:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:12:17:12:91 | call to method Equals | returns the first sequence element satisfying a predicate |
2+
| MissedFirstOrDefaultOpportunity.cs:22:9:28:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:24:17:24:25 | ... > ... | returns the first sequence element satisfying a predicate |
3+
| MissedFirstOrDefaultOpportunity.cs:36:9:40:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:38:17:38:25 | ... > ... | returns the first sequence element satisfying a predicate |

‎csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Linq/MissedFirstOrDefaultOpportunity.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

0 commit comments

Comments
 (0)