Skip to content
Open
Show file tree
Hide file tree
Changes from 10 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
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ ql/csharp/ql/src/Likely Bugs/UncheckedCastInEquals.ql
ql/csharp/ql/src/Linq/BadMultipleIteration.ql
ql/csharp/ql/src/Linq/MissedAllOpportunity.ql
ql/csharp/ql/src/Linq/MissedCastOpportunity.ql
ql/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql
ql/csharp/ql/src/Linq/MissedOfTypeOpportunity.ql
ql/csharp/ql/src/Linq/MissedSelectOpportunity.ql
ql/csharp/ql/src/Linq/MissedWhereOpportunity.ql
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ ql/csharp/ql/src/Likely Bugs/StringBuilderCharInit.ql
ql/csharp/ql/src/Likely Bugs/UncheckedCastInEquals.ql
ql/csharp/ql/src/Linq/MissedAllOpportunity.ql
ql/csharp/ql/src/Linq/MissedCastOpportunity.ql
ql/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql
ql/csharp/ql/src/Linq/MissedOfTypeOpportunity.ql
ql/csharp/ql/src/Linq/MissedSelectOpportunity.ql
ql/csharp/ql/src/Linq/MissedWhereOpportunity.ql
Expand Down
50 changes: 50 additions & 0 deletions csharp/ql/lib/Linq/Helpers.qll
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,36 @@ private int numStmts(ForeachStmt fes) {
else result = 1
}

private predicate returnsLoopVariable(ForeachStmt fes, Stmt s) {
exists(ReturnStmt ret |
ret = s.stripSingletonBlocks() and
ret.getExpr().stripCasts().(VariableAccess).getTarget() = fes.getVariable()
Comment thread
baywet marked this conversation as resolved.
Outdated
)
}

private predicate hasNullDefault(Type t) { t.isRefType() or t instanceof NullableType }

private predicate returnsDefaultValueAfterForeach(ForeachStmt fes) {
exists(BlockStmt enclosingBlock, int i, Type elementType, ReturnStmt ret |
enclosingBlock.getStmt(i) = fes and
enclosingBlock.getStmt(i + 1) = ret and
elementType = fes.getVariable().getType()
|
ret.getExpr().stripCasts() instanceof NullLiteral and
Comment thread
baywet marked this conversation as resolved.
Outdated
hasNullDefault(elementType)
or
exists(DefaultValueExpr defaultValue |
defaultValue = ret.getExpr().stripCasts() and
Comment thread
baywet marked this conversation as resolved.
Outdated
(
defaultValue.getType() = elementType
or
hasNullDefault(elementType) and
hasNullDefault(defaultValue.getType())
)
)
)
}

/** Holds if the type's qualified name is "System.Linq.Enumerable" */
predicate isEnumerableType(ValueOrRefType t) {
t.hasFullyQualifiedName("System.Linq", "Enumerable")
Expand Down Expand Up @@ -156,6 +186,26 @@ predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) {
)
}

/**
* Holds if `foreach` statement `fes` could be converted to a `.FirstOrDefault()` call.
* That is, the loop contains a single `if` statement that accesses the loop variable,
* returns the loop variable when the condition matches, and is followed by a default return.
*/
predicate missedFirstOrDefaultOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) {
// The loop only checks whether the current element is the first match.
is = firstStmt(fes) and
not exists(is.getElse()) and
numStmts(fes) = 1 and
// Condition relies on loop variable.
exists(VariableAccess va |
va.getTarget() = fes.getVariable() and
va = is.getCondition().getAChildExpr*()
) and
not is.getCondition().getAChildExpr*() instanceof AwaitExpr and
Comment thread
baywet marked this conversation as resolved.
returnsLoopVariable(fes, is.getThen()) and
returnsDefaultValueAfterForeach(fes)
Comment thread
baywet marked this conversation as resolved.
}

//#################### CLASSES ####################
/** A LINQ Any(...) call. */
class AnyCall extends MethodCall {
Expand Down
21 changes: 21 additions & 0 deletions csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using System;
using System.Collections.Generic;

class MissedFirstOrDefaultOpportunity
{
public static Operation FindOperation(IEnumerable<Operation> operations, string operationId)
{
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation;
}

return null;
}
}

class Operation
{
public string OperationId { get; set; }
}
36 changes: 36 additions & 0 deletions csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>Programmers sometimes search a sequence by iterating over each element, testing it, and returning
the first element that satisfies the test. If the loop completes without finding a match, the method
then returns a default value such as <code>null</code> or <code>default</code>.</p>

</overview>
<recommendation>
<p>This pattern is directly available as the <code>FirstOrDefault</code> method in LINQ. Using the
library method makes the search intent explicit and avoids manually spelling out the loop and
fallback return.</p>

</recommendation>
<example>
<p>In this example the method searches a list of operations for the first operation with a matching
identifier, returning <code>null</code> if no match is found.</p>
<sample src="MissedFirstOrDefaultOpportunity.cs" />

<p>The LINQ <code>FirstOrDefault</code> method can express this search more directly.</p>
<sample src="MissedFirstOrDefaultOpportunityFix.cs" />

<p>The following examples should not use <code>FirstOrDefault</code>, because they do more than
return the matching element or because the fallback value is not the default value.</p>
<sample src="MissedFirstOrDefaultOpportunityGood.cs" />

</example>
<references>

<li>MSDN: <a href="https://learn.microsoft.com/dotnet/api/system.linq.enumerable.firstordefault">Enumerable.FirstOrDefault Method</a>.</li>


</references>
</qhelp>
22 changes: 22 additions & 0 deletions csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* @name Missed opportunity to use FirstOrDefault
* @description The intent of a foreach loop that returns the first sequence element satisfying a predicate, or a default value otherwise,
* can often be better expressed using LINQ's 'FirstOrDefault' method.
* @kind problem
* @problem.severity recommendation
* @precision high
* @id cs/linq/missed-firstordefault
* @tags quality
* maintainability
* readability
* language-features
*/

import csharp
import Linq.Helpers

from ForeachStmtGenericEnumerable fes, IfStmt is
where missedFirstOrDefaultOpportunity(fes, is)
Comment thread
baywet marked this conversation as resolved.
select fes,
"This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.",
is.getCondition(), "predicate"
12 changes: 12 additions & 0 deletions csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
using System.Collections.Generic;
using System.Linq;

class MissedFirstOrDefaultOpportunityFix
{
public static Operation FindOperation(IEnumerable<Operation> operations, string operationId)
{
return operations.FirstOrDefault(operation =>
string.Equals(operation.OperationId, operationId, StringComparison.Ordinal));
}
}
38 changes: 38 additions & 0 deletions csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
using System;
using System.Collections.Generic;

class MissedFirstOrDefaultOpportunityGood
{
public static Operation FindOperationOrThrow(IEnumerable<Operation> operations, string operationId)
{
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
throw new InvalidOperationException("Unexpected operation.");
}

return null;
}

public static Operation FindReplacementOperation(IEnumerable<Operation> operations, string operationId)
{
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation;
}

return new Operation();
}

public static string FindOperationId(IEnumerable<Operation> operations, string operationId)
{
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation.OperationId;
}

return null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new query, `cs/linq/missed-firstordefault`, that detects `foreach` loops that can be expressed more clearly using LINQ's `FirstOrDefault` method.
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;

class MissedFirstOrDefaultOpportunity
{
public Operation M1(IEnumerable<Operation> operations, string operationId)
{
// BAD: Can be replaced with operations.FirstOrDefault(operation => ...).
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation;
} // $ Alert

return null;
}

public int M2(IEnumerable<int> values)
{
// BAD: Can be replaced with values.FirstOrDefault(value => ...).
foreach (var value in values)
{
if (value > 0)
{
return value;
}
} // $ Alert

return default;
}

public int? M3(List<int> values)
{
// BAD: Can be replaced with values.FirstOrDefault(value => ...).
foreach (var value in values)
{
if (value > 0)
return value;
} // $ Alert

return default(int);
}

public Operation M4(IEnumerable<Operation> operations, string operationId)
{
// GOOD: FirstOrDefault does not throw when a match is found.
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
throw new InvalidOperationException();
}

return null;
}

public Operation M5(IEnumerable<Operation> operations, string operationId)
{
// GOOD: FirstOrDefault would return null/default when no match is found.
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation;
}

return new Operation();
}

public string M6(IEnumerable<Operation> operations, string operationId)
{
// GOOD: FirstOrDefault would return the matching operation, not one of its properties.
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation.OperationId;
}

return null;
}

public Operation M7(IEnumerable<Operation> operations, string operationId)
{
// GOOD: The matched case has an additional side effect.
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
{
Console.WriteLine(operation.OperationId);
return operation;
}
}

return null;
}

public async Task<Operation> M8(IEnumerable<Operation> operations, string operationId)
{
// GOOD: FirstOrDefault does not support an async predicate.
foreach (var operation in operations)
{
if (await IsMatch(operation, operationId))
return operation;
}

return null;
}

public Operation M9(IEnumerable<Operation> operations, string operationId)
{
// GOOD: FirstOrDefault does not have an equivalent for an else branch in the loop.
foreach (var operation in operations)
{
if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal))
return operation;
else
return null;
}

return null;
}

public object M10(IEnumerable<int> values)
{
// GOOD: FirstOrDefault would return boxed 0 when no match is found, not null.
foreach (var value in values)
{
if (value > 0)
return value;
}

return null;
}

public object M11(IEnumerable<int> values)
{
// GOOD: FirstOrDefault would return boxed 0 when no match is found, not default(object).
foreach (var value in values)
{
if (value > 0)
return value;
}

return default(object);
}

public object M12(IEnumerable<string> values)
{
// BAD: FirstOrDefault returns null for missing reference-type elements, matching the fallback.
foreach (var value in values)
{
if (value.Length > 0)
return value;
} // $ Alert

return null;
}

public object M13(IEnumerable<int> values)
{
// BAD: FirstOrDefault returns 0 for missing int elements, matching the fallback before boxing.
foreach (var value in values)
{
if (value > 0)
return value;
} // $ Alert

return default(int);
}

private static Task<bool> IsMatch(Operation operation, string operationId) =>
Task.FromResult(string.Equals(operation.OperationId, operationId, StringComparison.Ordinal));
}

class Operation
{
public string OperationId { get; set; }
}
Loading
Loading