diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 2a4d5c8c27a2..7739ad610d09 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -20,6 +20,32 @@ private int numStmts(ForeachStmt fes) { else result = 1 } +private predicate returnsLoopVariable(ForeachStmt fes, Stmt s, ReturnStmt ret) { + ret = s.stripSingletonBlocks() and + ret.getExpr().stripCasts().(VariableAccess).getTarget() = fes.getVariable() +} + +private predicate hasNullDefault(Type t) { t.isRefType() or t instanceof NullableType } + +private predicate returnsDefaultValue(ForeachStmt fes, ReturnStmt ret) { + exists(Type elementType | + elementType = fes.getVariable().getType() + | + ret.getExpr().stripCasts() instanceof NullLiteral and + hasNullDefault(elementType) + or + exists(DefaultValueExpr defaultValue | + defaultValue = ret.getExpr().stripCasts() and + ( + 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") @@ -156,6 +182,30 @@ 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 + exists(VariableAccess va | + va.getTarget() = fes.getVariable() and + va = is.getCondition().getAChildExpr*() + ) and + not is.getCondition().getAChildExpr*() instanceof AwaitExpr and + exists(ReturnStmt ret, ReturnStmt defaultRet, BlockStmt enclosingBlock, int i | + returnsLoopVariable(fes, is.getThen(), ret) and + // If no element matches, the method returns the same value that FirstOrDefault would. + returnsDefaultValue(fes, defaultRet) and + enclosingBlock.getStmt(i) = fes and + enclosingBlock.getStmt(i + 1) = defaultRet + ) +} + //#################### CLASSES #################### /** A LINQ Any(...) call. */ class AnyCall extends MethodCall { diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs new file mode 100644 index 000000000000..ef968cc7dfd9 --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs @@ -0,0 +1,21 @@ +using System; +using System.Collections.Generic; + +class MissedFirstOrDefaultOpportunity +{ + public static Operation FindOperation(IEnumerable 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; } +} diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp new file mode 100644 index 000000000000..578b062ca34e --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp @@ -0,0 +1,36 @@ + + + +

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 null or default.

+ +
+ +

This pattern is directly available as the FirstOrDefault method in LINQ. Using the +library method makes the search intent explicit and avoids manually spelling out the loop and +fallback return.

+ +
+ +

In this example the method searches a list of operations for the first operation with a matching +identifier, returning null if no match is found.

+ + +

The LINQ FirstOrDefault method can express this search more directly.

+ + +

The following examples should not use FirstOrDefault, because they do more than +return the matching element or because the fallback value is not the default value.

+ + +
+ + +
  • MSDN: Enumerable.FirstOrDefault Method.
  • + + +
    +
    diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql new file mode 100644 index 000000000000..705881a73bdf --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql @@ -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) +select fes, + "This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.", + is.getCondition(), "predicate" diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs new file mode 100644 index 000000000000..3d7818fc0db8 --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs @@ -0,0 +1,12 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +class MissedFirstOrDefaultOpportunityFix +{ + public static Operation FindOperation(IEnumerable operations, string operationId) + { + return operations.FirstOrDefault(operation => + string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)); + } +} diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs new file mode 100644 index 000000000000..6c65760416de --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs @@ -0,0 +1,38 @@ +using System; +using System.Collections.Generic; + +class MissedFirstOrDefaultOpportunityGood +{ + public static Operation FindOperationOrThrow(IEnumerable 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 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 operations, string operationId) + { + foreach (var operation in operations) + { + if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)) + return operation.OperationId; + } + + return null; + } +} diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs new file mode 100644 index 000000000000..80584bd6ea6b --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs @@ -0,0 +1,177 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; + +class MissedFirstOrDefaultOpportunity +{ + public Operation M1(IEnumerable 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 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 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 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 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 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 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 M8(IEnumerable 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 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 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 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 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 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 IsMatch(Operation operation, string operationId) => + Task.FromResult(string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)); +} + +class Operation +{ + public string OperationId { get; set; } +} diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected new file mode 100644 index 000000000000..b4cfdff5fdd9 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected @@ -0,0 +1,5 @@ +| MissedFirstOrDefaultOpportunity.cs:10:9:14:9 | foreach (... ... in ...) ... | This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:12:17:12:91 | call to method Equals | predicate | +| MissedFirstOrDefaultOpportunity.cs:22:9:28:9 | foreach (... ... in ...) ... | This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:24:17:24:25 | ... > ... | predicate | +| MissedFirstOrDefaultOpportunity.cs:36:9:40:9 | foreach (... ... in ...) ... | This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:38:17:38:25 | ... > ... | predicate | +| MissedFirstOrDefaultOpportunity.cs:149:9:153:9 | foreach (... ... in ...) ... | This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:151:17:151:32 | ... > ... | predicate | +| MissedFirstOrDefaultOpportunity.cs:161:9:165:9 | foreach (... ... in ...) ... | This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:163:17:163:25 | ... > ... | predicate | diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref new file mode 100644 index 000000000000..91cc5ae4d348 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref @@ -0,0 +1,2 @@ +query: Linq/MissedFirstOrDefaultOpportunity.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql