feat(csharp): adds a missed first or default opprtunity rule - #22485
feat(csharp): adds a missed first or default opprtunity rule#22485baywet wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The detection currently produces behavior-changing recommendations for converted iteration types, incompatible defaults, and asynchronous enumeration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a C# quality query detecting loops that can use LINQ’s FirstOrDefault.
Changes:
- Implements and registers the new query.
- Adds documentation, examples, and tests.
- Extends shared LINQ detection helpers.
File summaries
| File | Description |
|---|---|
csharp/ql/lib/Linq/Helpers.qll |
Adds detection logic. |
csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.ql |
Defines the query. |
csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelp |
Documents the recommendation. |
csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.cs |
Provides a flagged example. |
csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityFix.cs |
Provides the recommended fix. |
csharp/ql/src/Linq/MissedFirstOrDefaultOpportunityGood.cs |
Provides non-alerting examples. |
csharp/ql/src/codeql-suites/csharp-security-and-quality.qls |
Enables the query in the quality suite. |
csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs |
Adds test cases. |
csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.qlref |
Configures the query test. |
csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.expected |
Records expected results. |
Review details
Suppressed comments (2)
csharp/ql/lib/Linq/Helpers.qll:184
- This only checks that the condition accesses the iteration variable, but
foreachmay implicitly convert each source element to a different variable type. For example,foreach (string value in IEnumerable<object>)can match here, whileFirstOrDefaultreceives and returnsobject, so the proposed replacement neither exposesstringmembers in the predicate nor returns the same type. Require an identity conversion betweenfes.getElementType()and the iteration-variable type (or account for the requiredCast<T>()).
exists(VariableAccess va |
va.getTarget() = fes.getVariable() and
va = is.getCondition().getAChildExpr*()
csharp/ql/lib/Linq/Helpers.qll:179
- Exclude asynchronous foreach statements here. A type can implement both
IAsyncEnumerable<T>andIEnumerable<T>, so it satisfiesForeachStmtGenericEnumerable, but replacing itsawait foreachwithFirstOrDefaultswitches to synchronous enumeration and can change behavior.
is = firstStmt(fes) and
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
michaelnebel
left a comment
There was a problem hiding this comment.
Thank you very much! I was just thinking about the same thing, when I reviewed the other PR about the missed-where false positives 😄 .
I have added some initial comments.
Co-authored-by: Michael Nebel <michaelnebel@github.com>
Co-authored-by: Michael Nebel <michaelnebel@github.com>
|
QHelp previews: csharp/ql/src/Linq/MissedFirstOrDefaultOpportunity.qhelpMissed opportunity to use FirstOrDefaultProgrammers 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 RecommendationThis pattern is directly available as the ExampleIn this example the method searches a list of operations for the first operation with a matching identifier, returning 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; }
}The LINQ 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));
}
}The following examples should not use 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;
}
}References |
| va = is.getCondition().getAChildExpr*() | ||
| ) and | ||
| not is.getCondition().getAChildExpr*() instanceof AwaitExpr and | ||
| exists(ReturnStmt ret, ReturnStmt defaultRet, BlockStmt enclosingBlock, int i | |
there are already rules for missed where/all/oftype/select/cast opportunities. It only makes sense to have FirstOrDefault which is a very common use case. Related #22484