From ed15edf6df947adc5aa7bcc50676c8f73df71017 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 11:02:14 -0400 Subject: [PATCH 1/6] feat(csharp): adds a missed first or default opprtunity rule --- csharp/ql/lib/Linq/Helpers.qll | 37 +++++ .../Linq/MissedFirstOrDefaultOpportunity.cs | 18 +++ .../MissedFirstOrDefaultOpportunity.qhelp | 36 +++++ .../Linq/MissedFirstOrDefaultOpportunity.ql | 22 +++ .../MissedFirstOrDefaultOpportunityFix.cs | 8 ++ .../MissedFirstOrDefaultOpportunityGood.cs | 35 +++++ .../csharp-security-and-quality.qls | 1 + .../MissedFirstOrDefaultOpportunity.cs | 129 ++++++++++++++++++ .../MissedFirstOrDefaultOpportunity.expected | 3 + .../MissedFirstOrDefaultOpportunity.qlref | 2 + 10 files changed, 291 insertions(+) create mode 100644 csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs create mode 100644 csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp create mode 100644 csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql create mode 100644 csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs create mode 100644 csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs create mode 100644 csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs create mode 100644 csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected create mode 100644 csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 2a4d5c8c27a2..6f636aab4acc 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -20,6 +20,17 @@ 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 returnsDefaultValue(ReturnStmt ret) { + ret.getExpr().stripCasts() instanceof NullLiteral + or + ret.getExpr().stripCasts() instanceof DefaultValueExpr +} + /** Holds if the type's qualified name is "System.Linq.Enumerable" */ predicate isEnumerableType(ValueOrRefType t) { t.hasFullyQualifiedName("System.Linq", "Enumerable") @@ -156,6 +167,32 @@ 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, ReturnStmt ret, ReturnStmt defaultRet +) { + // 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 + returnsLoopVariable(fes, is.getThen(), ret) and + // If no element matches, the method returns the same value that FirstOrDefault would. + returnsDefaultValue(defaultRet) and + exists(BlockStmt enclosingBlock, int i | + 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..1a7a1dff6e93 --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs @@ -0,0 +1,18 @@ +class MissedFirstOrDefaultOpportunity +{ + public static Operation FindOperation(System.Collections.Generic.IEnumerable operations, string operationId) + { + foreach (var operation in operations) + { + if (string.Equals(operation.OperationId, operationId, System.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..adf0149478ac --- /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, ReturnStmt ret, ReturnStmt defaultRet +where missedFirstOrDefaultOpportunity(fes, is, ret, defaultRet) +select fes, + "This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.", + is.getCondition(), "returns the first sequence element satisfying a predicate" diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs new file mode 100644 index 000000000000..ba75401e9a73 --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs @@ -0,0 +1,8 @@ +class MissedFirstOrDefaultOpportunityFix +{ + public static Operation FindOperation(System.Collections.Generic.IEnumerable operations, string operationId) + { + return operations.FirstOrDefault(operation => + string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)); + } +} diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs new file mode 100644 index 000000000000..17593b87c001 --- /dev/null +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs @@ -0,0 +1,35 @@ +class MissedFirstOrDefaultOpportunityGood +{ + public static Operation FindOperationOrThrow(System.Collections.Generic.IEnumerable operations, string operationId) + { + foreach (var operation in operations) + { + if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + throw new System.InvalidOperationException("Unexpected operation."); + } + + return null; + } + + public static Operation FindReplacementOperation(System.Collections.Generic.IEnumerable operations, string operationId) + { + foreach (var operation in operations) + { + if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + return operation; + } + + return new Operation(); + } + + public static string FindOperationId(System.Collections.Generic.IEnumerable operations, string operationId) + { + foreach (var operation in operations) + { + if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + return operation.OperationId; + } + + return null; + } +} diff --git a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls index 9700c8b03410..c86e286862ec 100644 --- a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls +++ b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls @@ -51,6 +51,7 @@ - cs/linq/inconsistent-enumeration - cs/linq/missed-all - cs/linq/missed-cast + - cs/linq/missed-firstordefault - cs/linq/missed-oftype - cs/linq/missed-select - cs/linq/missed-where 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..2894ef26d3de --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs @@ -0,0 +1,129 @@ +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; + } + + 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..6724c12bc08e --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected @@ -0,0 +1,3 @@ +| 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 | +| 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 | +| 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 | 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 From 41a9a16b757559819b29c2a8cc0218e1c44cb671 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 1 Sep 2026 13:24:25 -0400 Subject: [PATCH 2/6] fix: handle non null default values in the rule --- csharp/ql/lib/Linq/Helpers.qll | 24 ++++++++-- .../MissedFirstOrDefaultOpportunity.cs | 48 +++++++++++++++++++ .../MissedFirstOrDefaultOpportunity.expected | 2 + 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 6f636aab4acc..870f328ea5b0 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -25,10 +25,24 @@ private predicate returnsLoopVariable(ForeachStmt fes, Stmt s, ReturnStmt ret) { ret.getExpr().stripCasts().(VariableAccess).getTarget() = fes.getVariable() } -private predicate returnsDefaultValue(ReturnStmt ret) { - ret.getExpr().stripCasts() instanceof NullLiteral - or - ret.getExpr().stripCasts() instanceof DefaultValueExpr +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" */ @@ -186,7 +200,7 @@ predicate missedFirstOrDefaultOpportunity( not is.getCondition().getAChildExpr*() instanceof AwaitExpr and returnsLoopVariable(fes, is.getThen(), ret) and // If no element matches, the method returns the same value that FirstOrDefault would. - returnsDefaultValue(defaultRet) and + returnsDefaultValue(fes, defaultRet) and exists(BlockStmt enclosingBlock, int i | enclosingBlock.getStmt(i) = fes and enclosingBlock.getStmt(i + 1) = defaultRet diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs index 2894ef26d3de..80584bd6ea6b 100644 --- a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs @@ -119,6 +119,54 @@ public Operation M9(IEnumerable operations, string operationId) 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)); } diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected index 6724c12bc08e..70689f5aaeb9 100644 --- a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected @@ -1,3 +1,5 @@ | 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 | | 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 | | 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 | +| MissedFirstOrDefaultOpportunity.cs:149:9:153:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:151:17:151:32 | ... > ... | returns the first sequence element satisfying a predicate | +| MissedFirstOrDefaultOpportunity.cs:161:9:165:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:163:17:163:25 | ... > ... | returns the first sequence element satisfying a predicate | From baab7592782f40626f8943ae781934a5ca5a5ac3 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 09:14:13 -0400 Subject: [PATCH 3/6] chore: applies review suggestion Co-authored-by: Michael Nebel --- csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql index adf0149478ac..d166eb440603 100644 --- a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql @@ -18,5 +18,5 @@ import Linq.Helpers from ForeachStmtGenericEnumerable fes, IfStmt is, ReturnStmt ret, ReturnStmt defaultRet where missedFirstOrDefaultOpportunity(fes, is, ret, defaultRet) select fes, - "This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.", - is.getCondition(), "returns the first sequence element satisfying a predicate" + "This foreach loop returns the first sequence element satisfying a $@ - consider finding the element explicitly using '.FirstOrDefault(...)'.", + is.getCondition(), "predicate" From a52c2cb24d762a19ac75675f904d84e5caa06774 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 09:14:30 -0400 Subject: [PATCH 4/6] chore: applies review suggestion Co-authored-by: Michael Nebel --- csharp/ql/lib/Linq/Helpers.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 870f328ea5b0..a1ffd30ee5bc 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -28,8 +28,11 @@ private predicate returnsLoopVariable(ForeachStmt fes, Stmt s, ReturnStmt ret) { 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() | + exists(BlockStmt enclosingBlock, int i, Type elementType | + enclosingBlock.getStmt(i) = fes and + enclosingBlock.getStmt(i + 1) = ret and + elementType = fes.getVariable().getType() + | ret.getExpr().stripCasts() instanceof NullLiteral and hasNullDefault(elementType) or From 6744d560243caf312907f70a3eae1541b757b869 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 09:16:33 -0400 Subject: [PATCH 5/6] chore: reverts addition to ruleset --- csharp/ql/src/codeql-suites/csharp-security-and-quality.qls | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls index c86e286862ec..9700c8b03410 100644 --- a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls +++ b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls @@ -51,7 +51,6 @@ - cs/linq/inconsistent-enumeration - cs/linq/missed-all - cs/linq/missed-cast - - cs/linq/missed-firstordefault - cs/linq/missed-oftype - cs/linq/missed-select - cs/linq/missed-where From c97f0f4b3059e4e772a63df44124d3a0ee522393 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 2 Sep 2026 09:22:20 -0400 Subject: [PATCH 6/6] chore: applies review suggestions --- csharp/ql/lib/Linq/Helpers.qll | 16 ++++++---------- .../src/Linq/MissedFirstOrDefaultOpportunity.cs | 7 +++++-- .../src/Linq/MissedFirstOrDefaultOpportunity.ql | 4 ++-- .../Linq/MissedFirstOrDefaultOpportunityFix.cs | 8 ++++++-- .../Linq/MissedFirstOrDefaultOpportunityGood.cs | 17 ++++++++++------- .../MissedFirstOrDefaultOpportunity.expected | 10 +++++----- 6 files changed, 34 insertions(+), 28 deletions(-) diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index a1ffd30ee5bc..7739ad610d09 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -28,9 +28,7 @@ private predicate returnsLoopVariable(ForeachStmt fes, Stmt s, ReturnStmt ret) { private predicate hasNullDefault(Type t) { t.isRefType() or t instanceof NullableType } private predicate returnsDefaultValue(ForeachStmt fes, ReturnStmt ret) { - exists(BlockStmt enclosingBlock, int i, Type elementType | - enclosingBlock.getStmt(i) = fes and - enclosingBlock.getStmt(i + 1) = ret and + exists(Type elementType | elementType = fes.getVariable().getType() | ret.getExpr().stripCasts() instanceof NullLiteral and @@ -189,9 +187,7 @@ predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) { * 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, ReturnStmt ret, ReturnStmt defaultRet -) { +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 @@ -201,10 +197,10 @@ predicate missedFirstOrDefaultOpportunity( va = is.getCondition().getAChildExpr*() ) and not is.getCondition().getAChildExpr*() instanceof AwaitExpr and - returnsLoopVariable(fes, is.getThen(), ret) and - // If no element matches, the method returns the same value that FirstOrDefault would. - returnsDefaultValue(fes, defaultRet) and - exists(BlockStmt enclosingBlock, int i | + 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 ) diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs index 1a7a1dff6e93..ef968cc7dfd9 100644 --- a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs @@ -1,10 +1,13 @@ +using System; +using System.Collections.Generic; + class MissedFirstOrDefaultOpportunity { - public static Operation FindOperation(System.Collections.Generic.IEnumerable operations, string operationId) + public static Operation FindOperation(IEnumerable operations, string operationId) { foreach (var operation in operations) { - if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)) return operation; } diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql index d166eb440603..705881a73bdf 100644 --- a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql @@ -15,8 +15,8 @@ import csharp import Linq.Helpers -from ForeachStmtGenericEnumerable fes, IfStmt is, ReturnStmt ret, ReturnStmt defaultRet -where missedFirstOrDefaultOpportunity(fes, is, ret, defaultRet) +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 index ba75401e9a73..3d7818fc0db8 100644 --- a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs @@ -1,8 +1,12 @@ +using System; +using System.Collections.Generic; +using System.Linq; + class MissedFirstOrDefaultOpportunityFix { - public static Operation FindOperation(System.Collections.Generic.IEnumerable operations, string operationId) + public static Operation FindOperation(IEnumerable operations, string operationId) { return operations.FirstOrDefault(operation => - string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)); + string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)); } } diff --git a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs index 17593b87c001..6c65760416de 100644 --- a/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs +++ b/csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs @@ -1,32 +1,35 @@ +using System; +using System.Collections.Generic; + class MissedFirstOrDefaultOpportunityGood { - public static Operation FindOperationOrThrow(System.Collections.Generic.IEnumerable operations, string operationId) + public static Operation FindOperationOrThrow(IEnumerable operations, string operationId) { foreach (var operation in operations) { - if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) - throw new System.InvalidOperationException("Unexpected operation."); + if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)) + throw new InvalidOperationException("Unexpected operation."); } return null; } - public static Operation FindReplacementOperation(System.Collections.Generic.IEnumerable operations, string operationId) + public static Operation FindReplacementOperation(IEnumerable operations, string operationId) { foreach (var operation in operations) { - if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)) return operation; } return new Operation(); } - public static string FindOperationId(System.Collections.Generic.IEnumerable operations, string operationId) + public static string FindOperationId(IEnumerable operations, string operationId) { foreach (var operation in operations) { - if (string.Equals(operation.OperationId, operationId, System.StringComparison.Ordinal)) + if (string.Equals(operation.OperationId, operationId, StringComparison.Ordinal)) return operation.OperationId; } diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected index 70689f5aaeb9..b4cfdff5fdd9 100644 --- a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected @@ -1,5 +1,5 @@ -| 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 | -| 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 | -| 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 | -| MissedFirstOrDefaultOpportunity.cs:149:9:153:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:151:17:151:32 | ... > ... | returns the first sequence element satisfying a predicate | -| MissedFirstOrDefaultOpportunity.cs:161:9:165:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider finding the element explicitly using '.FirstOrDefault(...)'. | MissedFirstOrDefaultOpportunity.cs:163:17:163:25 | ... > ... | returns the first sequence element satisfying a predicate | +| 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 |