diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index 052b5fe6c2e6..99ec505c697a 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/csharp/ql/lib/Linq/Helpers.qll @@ -116,6 +116,55 @@ class ForEachStmtEnumerable extends ForEachStmt { } } +private predicate accessInOutOrRefParameter(Expr e) { + exists(ParameterAccess pa, Parameter p | p = pa.getTarget() | + pa = e.getAChildExpr*() and + (p.isOutOrRef() or p.isIn() or p.isReadonlyRef()) + ) +} + +private signature predicate linqCandidateSig(Stmt s, Expr e); + +private module LinqFilterOpportunity { + predicate missed(ForEachStmtGenericEnumerable fes, Stmt s) { + s = firstStmt(fes) and + // The linq candidate expression accesses the loop variable, and the + // candidate doesn't access an in, out, or ref parameter. + exists(Expr candidate | linqCandidate(s, candidate) | + fes.getVariable().getAnAccess() = candidate.getAChildExpr*() and + not accessInOutOrRefParameter(candidate) + ) + } +} + +private module LinqMapOpportunity { + predicate missed(ForEachStmt fes, Stmt s) { + s = firstStmt(fes) and + // The linq candidate (and only the candidate) expression accesses the loop variable and the + // candidate doesn't access an in, out, or ref parameter. + exists(Expr candidate | linqCandidate(s, candidate) | + forex(VariableAccess va | va = fes.getVariable().getAnAccess() | + va = candidate.getAChildExpr*() + ) and + not accessInOutOrRefParameter(candidate) + ) + } +} + +private predicate linqAllCandidate(Stmt s, Expr e) { + s = + any(IfStmt is | + e = is.getCondition() and + not exists(is.getElse()) and // The then case of the if assigns false to something and breaks out of the loop. + exists(Assignment a, BoolLiteral bl | + a = is.getThen().getAChild*() and + bl = a.getRightOperand() and + bl.toString() = "false" + ) and + is.getThen().getAChild*() instanceof BreakStmt + ) +} + /** * Holds if `foreach` statement `fes` could be converted to a `.All()` call. * That is, the `ForEachStmt` contains a single `if` with a condition that @@ -123,21 +172,20 @@ class ForEachStmtEnumerable extends ForEachStmt { * and `break`s out of the `foreach`. */ predicate missedAllOpportunity(ForEachStmtGenericEnumerable fes) { - exists(IfStmt is | - // The loop contains an if statement with no else case, and nothing else. - is = firstStmt(fes) and - numStmts(fes) = 1 and - not exists(is.getElse()) and - // The if statement accesses the loop variable. - is.getCondition().getAChildExpr*() = fes.getVariable().getAnAccess() and - // The then case of the if assigns false to something and breaks out of the loop. - exists(Assignment a, BoolLiteral bl | - a = is.getThen().getAChild*() and - bl = a.getRightOperand() and - bl.toString() = "false" - ) and - is.getThen().getAChild*() instanceof BreakStmt - ) + // The loop contains an if statement with no else case, and nothing else. + LinqFilterOpportunity::missed(fes, _) and + numStmts(fes) = 1 +} + +private predicate linqCastCandidate(Stmt s, Expr e) { + s = + any(LocalVariableDeclStmt lvds | + exists(CastExpr ce | + ce = lvds.getAVariableDeclExpr().getInitializer() and + e = ce.getExpr() and + e instanceof VariableAccess + ) + ) } /** @@ -147,14 +195,18 @@ predicate missedAllOpportunity(ForEachStmtGenericEnumerable fes) { * local variable declaration statement `s`. */ predicate missedCastOpportunity(ForEachStmtEnumerable fes, LocalVariableDeclStmt s) { - s = firstStmt(fes) and - forex(VariableAccess va | va = fes.getVariable().getAnAccess() | - va = s.getAVariableDeclExpr().getAChildExpr*() - ) and - exists(CastExpr ce | - ce = s.getAVariableDeclExpr().getInitializer() and - ce.getExpr() = fes.getVariable().getAnAccess() - ) + LinqMapOpportunity::missed(fes, s) +} + +private predicate linqOfTypeCandidate(Stmt s, Expr e) { + s = + any(LocalVariableDeclStmt lvds | + exists(AsExpr ae | + ae = lvds.getAVariableDeclExpr().getInitializer() and + e = ae.getExpr() and + e instanceof VariableAccess + ) + ) } /** @@ -164,14 +216,16 @@ predicate missedCastOpportunity(ForEachStmtEnumerable fes, LocalVariableDeclStmt * is a local variable declaration statement `s`. */ predicate missedOfTypeOpportunity(ForEachStmtEnumerable fes, LocalVariableDeclStmt s) { - s = firstStmt(fes) and - forex(VariableAccess va | va = fes.getVariable().getAnAccess() | - va = s.getAVariableDeclExpr().getAChildExpr*() - ) and - exists(AsExpr ae | - ae = s.getAVariableDeclExpr().getInitializer() and - ae.getExpr() = fes.getVariable().getAnAccess() - ) + LinqMapOpportunity::missed(fes, s) +} + +private predicate linqSelectCandidate(Stmt s, Expr e) { + s = + any(LocalVariableDeclStmt lvds | + e = lvds.getAVariableDeclExpr().getInitializer() and + not e instanceof Cast and + not e.getAChildExpr*() instanceof AwaitExpr + ) } /** @@ -182,12 +236,24 @@ predicate missedOfTypeOpportunity(ForEachStmtEnumerable fes, LocalVariableDeclSt * contain an `await` expression (since `Select` does not support async lambdas). */ predicate missedSelectOpportunity(ForEachStmtGenericEnumerable fes, LocalVariableDeclStmt s) { - s = firstStmt(fes) and - forex(VariableAccess va | va = fes.getVariable().getAnAccess() | - va = s.getAVariableDeclExpr().getAChildExpr*() - ) and - not s.getAVariableDeclExpr().getInitializer() instanceof Cast and - not s.getAVariableDeclExpr().getInitializer().getAChildExpr*() instanceof AwaitExpr + LinqMapOpportunity::missed(fes, s) +} + +private predicate linqWhereCandidateCase1(Stmt s, Expr e) { + s = + any(IfStmt is | + e = is.getCondition() and + is.getThen() instanceof ContinueStmt + ) +} + +private predicate linqWhereCandidateCase2(Stmt s, Expr e) { + s = + any(IfStmt is | + e = is.getCondition() and + not exists(is.getElse()) and + not terminatesCallable(is.getThen()) + ) } /** @@ -197,20 +263,21 @@ predicate missedSelectOpportunity(ForEachStmtGenericEnumerable fes, LocalVariabl * else in the loop than the `if`. */ predicate missedWhereOpportunity(ForEachStmtGenericEnumerable fes, IfStmt is) { - // The very first thing the foreach loop does is test its iteration variable. - is = firstStmt(fes) and - exists(VariableAccess va | - va.getTarget() = fes.getVariable() and - va = is.getCondition().getAChildExpr*() - ) and - // It then either (a) continues, or (b) performs the entire body of the loop within the condition. - ( - is.getThen() instanceof ContinueStmt - or - not exists(is.getElse()) and - numStmts(fes) = 1 and - not terminatesCallable(is.getThen()) - ) + // The body of the `if` is a continue. + LinqFilterOpportunity::missed(fes, is) + or + // There's nothing else in the loop than the `if`. + LinqFilterOpportunity::missed(fes, is) and + numStmts(fes) = 1 +} + +private predicate linqFirstOrDefaultCandidate(Stmt s, Expr e) { + s = + any(IfStmt is | + e = is.getCondition() and + not exists(is.getElse()) and + not e.getAChildExpr*() instanceof AwaitExpr + ) } /** @@ -220,15 +287,8 @@ predicate missedWhereOpportunity(ForEachStmtGenericEnumerable fes, IfStmt is) { */ 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 + LinqFilterOpportunity::missed(fes, is) 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 not fes.isAsync() and not fes.getVariable().isCaptured() and returnsLoopVariable(fes, is.getThen()) and diff --git a/csharp/ql/src/change-notes/2026-09-10-missed-linq-inoutref.md b/csharp/ql/src/change-notes/2026-09-10-missed-linq-inoutref.md new file mode 100644 index 000000000000..400e0bcd2ed0 --- /dev/null +++ b/csharp/ql/src/change-notes/2026-09-10-missed-linq-inoutref.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `cs/linq/missed-*` queries no longer suggest rewrites that would capture `in`, `out`, or `ref` parameters in a lambda, fixing false-positive results for transformations that would not compile. diff --git a/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.cs new file mode 100644 index 000000000000..4b09d8bd6409 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.cs @@ -0,0 +1,53 @@ +using System; +using System.Linq; +using System.Collections.Generic; + +class MissedAllOpportunity +{ + public void M1(List lst) + { + // BAD: Can be replaced with lst.All(e => e % 2 == 0) + var allEven = true; + foreach (int i in lst) + { + if (i % 2 != 0) + { + allEven = false; + break; + } + } // $ Alert + } + + public void M2(NonEnumerableClass nec) + { + // GOOD: Linq can't be used here. + var allEven = true; + foreach (int i in nec) + { + if (i % 2 != 0) + { + allEven = false; + break; + } + } + } + + public void M3(List lst, ref int x) + { + // GOOD: Linq can't be used here because the condition uses a ref parameter. + var allEven = true; + foreach (int i in lst) + { + if (i % 2 != x) + { + allEven = false; + break; + } + } + } + + public class NonEnumerableClass + { + public IEnumerator GetEnumerator() => throw null; + } +} diff --git a/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.expected new file mode 100644 index 000000000000..b4300f8caf07 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.expected @@ -0,0 +1 @@ +| MissedAllOpportunity.cs:11:9:18:9 | foreach (... ... in ...) ... | This foreach loop looks as if it might be testing whether every sequence element satisfies a predicate - consider using '.All(...)'. | diff --git a/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.qlref b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.qlref new file mode 100644 index 000000000000..689d5fbb60a4 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.qlref @@ -0,0 +1,2 @@ +query: Linq/MissedAllOpportunity.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/options b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/options new file mode 100644 index 000000000000..75c39b4541ba --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedAllOpportunity/options @@ -0,0 +1,2 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj diff --git a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs index 8120a70414e5..84824efee4bc 100644 --- a/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs @@ -179,6 +179,20 @@ public Operation M14(IEnumerable operations, Func[] pre return null; } + public int M15(IEnumerable values, ref readonly int x) + { + // GOOD: FirstOrDefault does not support a predicate that captures a ref parameter. + foreach (var value in values) + { + if (value > x) + { + return value; + } + } + + return default; + } + 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/MissedSelectOpportunity/MissedSelectOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedSelectOpportunity/MissedSelectOpportunity.cs index 9655a5a0fa9c..0a958d3e50d8 100644 --- a/csharp/ql/test/query-tests/Linq/MissedSelectOpportunity/MissedSelectOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedSelectOpportunity/MissedSelectOpportunity.cs @@ -25,6 +25,17 @@ public async Task M2(IEnumerable counters) } } + public void M3(List lst, out int x) + { + // GOOD: Linq can't be used here as the Select would capture an out parameter. + x = 2; + foreach (int i in lst) + { + int j = i * x; + Console.WriteLine(j); + } + } + public interface ICounter { Task CountAsync(); diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs index 7b9d35821299..b3575473eab9 100644 --- a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -174,6 +174,18 @@ public void M12(IEnumerable elements) } } + public void M13(List lst, in int x) + { + // GOOD: Linq can't be used here because the condition uses an in parameter. + foreach (int i in lst) + { + if (i % 2 != x) + continue; + Console.WriteLine(i); + Console.WriteLine((i / 2)); + } + } + public class NonEnumerableClass { public IEnumerator GetEnumerator() => throw null;