Merge pull request #22541 from michaelnebel/csharp/linqinoutret

C#: Don't suggest LINQ re-writes that would capture `in`, `out`, or `ref` parameters in a lambda.

Author: michaelnebel

csharp/ql/lib/Linq/Helpers.qll

@@ -116,28 +116,79 @@ class ForEachStmtEnumerable extends ForEachStmt {
   }
 }
 
+bindingset[e]
+private predicate acceptableForLinqCapture(Expr e) {
+  not exists(ParameterAccess pa, Parameter p |
+    p = pa.getTarget() and
+    pa = e.getAChildExpr*()
+  |
+    p.isOutOrRef() or p.isIn() or p.isReadonlyRef()
+  )
+}
+
+private signature predicate linqCandidateSig(Stmt s, Expr e);
+
+private module LinqFilterOpportunity<linqCandidateSig/2 linqCandidate> {
+  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
+      acceptableForLinqCapture(candidate)
+    )
+  }
+}
+
+private module LinqMapOpportunity<linqCandidateSig/2 linqCandidate> {
+  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
+      acceptableForLinqCapture(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
  * accesses the loop variable and with a body that assigns `false` to a variable
  * 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<linqAllCandidate/2>::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 LocalVariableAccess
+      )
+    )
 }
 
 /**
@@ -147,14 +198,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<linqCastCandidate/2>::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 LocalVariableAccess
+      )
+    )
 }
 
 /**
@@ -164,14 +219,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<linqOfTypeCandidate/2>::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 +239,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<linqSelectCandidate/2>::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 +266,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<linqWhereCandidateCase1/2>::missed(fes, is)
+  or
+  // There's nothing else in the loop than the `if`.
+  LinqFilterOpportunity<linqWhereCandidateCase2/2>::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 +290,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<linqFirstOrDefaultCandidate/2>::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

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.

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<int> 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<int> 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<int> GetEnumerator() => throw null;
+    }
+}

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(...)'. |

csharp/ql/test/query-tests/Linq/MissedAllOpportunity/MissedAllOpportunity.qlref

@@ -0,0 +1,2 @@
+query: Linq/MissedAllOpportunity.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

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

csharp/ql/test/query-tests/Linq/MissedFirstOrDefaultOpportunity/MissedFirstOrDefaultOpportunity.cs

@@ -179,6 +179,20 @@ public Operation M14(IEnumerable<Operation> operations, Func<string, bool>[] pre
         return null;
     }
 
+    public int M15(IEnumerable<int> 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<bool> IsMatch(Operation operation, string operationId) =>
         Task.FromResult(string.Equals(operation.OperationId, operationId, StringComparison.Ordinal));
 }

csharp/ql/test/query-tests/Linq/MissedSelectOpportunity/MissedSelectOpportunity.cs

@@ -25,6 +25,17 @@ public async Task M2(IEnumerable<ICounter> counters)
         }
     }
 
+    public void M3(List<int> 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<int> CountAsync();

csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs

@@ -174,6 +174,18 @@ public void M12(IEnumerable<int> elements)
         }
     }
 
+    public void M13(List<int> 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<int> GetEnumerator() => throw null;