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
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
53 changes: 53 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().stripImplicit().(VariableAccess).getTarget() = fes.getVariable()
)
}

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().stripImplicit() instanceof NullLiteral and
hasNullDefault(elementType)
or
exists(DefaultValueExpr defaultValue |
defaultValue = ret.getExpr().stripImplicit() and
(
defaultValue.getType() = elementType
or
hasNullDefault(elementType) and
hasNullDefault(defaultValue.getType())
)
)
)
}

private predicate terminatesCallable(Stmt s) {
exists(Stmt stripped | stripped = s.stripSingletonBlocks() |
stripped instanceof ReturnStmt
Expand Down Expand Up @@ -177,6 +207,29 @@ 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.
not fes.isAsync() and
not fes.getVariable().isCaptured() and
Comment on lines +225 to +227
returnsLoopVariable(fes, is.getThen()) and
fes.getElementType() = fes.getVariable().getType() and
returnsDefaultValueAfterForeach(fes)
}

//#################### 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.
Loading
Loading