Merge pull request github#13885 from michaelnebel/csharp/linqforeach

C#: LINQ recommendation queries.

Author: michaelnebel

csharp/ql/lib/Linq/Helpers.qll

@@ -3,7 +3,9 @@
  * Provides helper classes and methods related to LINQ.
  */
 
-import csharp
+private import csharp
+private import semmle.code.csharp.frameworks.system.collections.Generic as GenericCollections
+private import semmle.code.csharp.frameworks.system.Collections as Collections
 
 //#################### PREDICATES ####################
 private Stmt firstStmt(ForeachStmt fes) {
@@ -29,13 +31,40 @@ predicate isIEnumerableType(ValueOrRefType t) {
   )
 }
 
+/**
+ * A class of foreach statements where the iterable expression
+ * supports the use of the LINQ extension methods on `IEnumerable<T>`.
+ */
+class ForeachStmtGenericEnumerable extends ForeachStmt {
+  ForeachStmtGenericEnumerable() {
+    exists(ValueOrRefType t | t = this.getIterableExpr().getType() |
+      t.getABaseType*().getUnboundDeclaration() instanceof
+        GenericCollections::SystemCollectionsGenericIEnumerableTInterface or
+      t.(ArrayType).getRank() = 1
+    )
+  }
+}
+
+/**
+ * A class of foreach statements where the iterable expression
+ * supports the use of the LINQ extension methods on `IEnumerable`.
+ */
+class ForeachStmtEnumerable extends ForeachStmt {
+  ForeachStmtEnumerable() {
+    exists(ValueOrRefType t | t = this.getIterableExpr().getType() |
+      t.getABaseType*() instanceof Collections::SystemCollectionsIEnumerableInterface or
+      t.(ArrayType).getRank() = 1
+    )
+  }
+}
+
 /**
  * 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(ForeachStmt fes) {
+predicate missedAllOpportunity(ForeachStmtGenericEnumerable fes) {
   exists(IfStmt is |
     // The loop contains an if statement with no else case, and nothing else.
     is = firstStmt(fes) and
@@ -54,12 +83,12 @@ predicate missedAllOpportunity(ForeachStmt fes) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to a `.Cast()` call.
+ * Holds if the `foreach` statement `fes` can be converted to a `.Cast()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is a cast. The first statement needs to be a
- * `LocalVariableDeclStmt`.
+ * block, the access is a cast, and the first statement is a
+ * local variable declaration statement `s`.
  */
-predicate missedCastOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedCastOpportunity(ForeachStmtEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -71,12 +100,12 @@ predicate missedCastOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to an `.OfType()` call.
+ * Holds if `foreach` statement `fes` can be converted to an `.OfType()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is a cast with the `as` operator. The first statement
- * needs to be a `LocalVariableDeclStmt`.
+ * block, the access is a cast with the `as` operator, and the first statement
+ * is a local variable declaration statement `s`.
  */
-predicate missedOfTypeOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedOfTypeOpportunity(ForeachStmtEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -88,12 +117,12 @@ predicate missedOfTypeOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to a `.Select()` call.
+ * Holds if `foreach` statement `fes` can be converted to a `.Select()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is not a cast. The first statement needs to be a
- * `LocalVariableDeclStmt`.
+ * block, the access is not a cast, and the first statement is a
+ * local variable declaration statement `s`.
  */
-predicate missedSelectOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedSelectOpportunity(ForeachStmtGenericEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -107,7 +136,7 @@ predicate missedSelectOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
  * variable, and the body of the `if` is either a `continue` or there's nothing
  * else in the loop than the `if`.
  */
-predicate missedWhereOpportunity(ForeachStmt fes, IfStmt is) {
+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 |

csharp/ql/src/Linq/MissedAllOpportunity.ql

@@ -10,7 +10,6 @@
  *       language-features
  */
 
-import csharp
 import Linq.Helpers
 
 /*
@@ -31,7 +30,7 @@ import Linq.Helpers
  * bool allEven = lst.All(i => i % 2 == 0);
  */
 
-from ForeachStmt fes
+from ForeachStmtGenericEnumerable fes
 where missedAllOpportunity(fes)
 select fes,
   "This foreach loop looks as if it might be testing whether every sequence element satisfies a predicate - consider using '.All(...)'."

csharp/ql/src/Linq/MissedCastOpportunity.ql

@@ -13,7 +13,7 @@
 import csharp
 import Linq.Helpers
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtEnumerable fes, LocalVariableDeclStmt s
 where missedCastOpportunity(fes, s)
 select fes,
   "This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'.",

csharp/ql/src/Linq/MissedOfTypeOpportunity.ql

@@ -13,7 +13,7 @@
 import csharp
 import Linq.Helpers
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtEnumerable fes, LocalVariableDeclStmt s
 where missedOfTypeOpportunity(fes, s)
 select fes,
   "This foreach loop immediately uses 'as' to $@ - consider using '.OfType(...)' instead.", s,

csharp/ql/src/Linq/MissedSelectOpportunity.ql

@@ -20,7 +20,7 @@ predicate oversized(LocalVariableDeclStmt s) {
   )
 }
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtGenericEnumerable fes, LocalVariableDeclStmt s
 where
   missedSelectOpportunity(fes, s) and
   not oversized(s)

csharp/ql/src/Linq/MissedWhereOpportunity.ql

@@ -12,7 +12,7 @@
 import csharp
 import Linq.Helpers
 
-from ForeachStmt fes, IfStmt is
+from ForeachStmtGenericEnumerable fes, IfStmt is
 where
   missedWhereOpportunity(fes, is) and
   not missedAllOpportunity(fes)

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

@@ -0,0 +1,78 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+
+class MissedCastOpportunity
+{
+    public void M1(List<Animal> animals)
+    {
+        // BAD: Can be replaced with animals.Cast<Dog>().
+        foreach (Animal a in animals)
+        {
+            Dog d = (Dog)a;
+            d.Woof();
+        }
+    }
+
+    public void M2(NonEnumerableClass nec)
+    {
+        // GOOD: Not possible to use Linq here.
+        foreach (Animal a in nec)
+        {
+            Dog d = (Dog)a;
+            d.Woof();
+        }
+    }
+
+    public void M3(Animal[] animals)
+    {
+        // BAD: Can be replaced with animals.Cast<Dog>().
+        foreach (Animal animal in animals)
+        {
+            Dog d = (Dog)animal;
+            d.Woof();
+        }
+    }
+
+    public void M4(Array animals)
+    {
+        // BAD: Can be replaced with animals.Cast<Dog>().
+        foreach (Animal animal in animals)
+        {
+            Dog d = (Dog)animal;
+            d.Woof();
+        }
+    }
+
+    public void M5(IEnumerable animals)
+    {
+        // BAD: Can be replaced with animals.Cast<Dog>().
+        foreach (object animal in animals)
+        {
+            Dog d = (Dog)animal;
+            d.Woof();
+        }
+    }
+
+    public class NonEnumerableClass
+    {
+        public IEnumerator<Animal> GetEnumerator() => throw null;
+    }
+
+    public class Animal { }
+
+    class Dog : Animal
+    {
+        private string name;
+
+        public Dog(string name)
+        {
+            this.name = name;
+        }
+
+        public void Woof()
+        {
+            Console.WriteLine("Woof! My name is " + name + ".");
+        }
+    }
+}

csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.expected

@@ -0,0 +1,4 @@
+| MissedCastOpportunity.cs:10:9:14:9 | foreach (... ... in ...) ... | This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'. | MissedCastOpportunity.cs:12:13:12:27 | ... ...; | casts its iteration variable to another type |
+| MissedCastOpportunity.cs:30:9:34:9 | foreach (... ... in ...) ... | This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'. | MissedCastOpportunity.cs:32:13:32:32 | ... ...; | casts its iteration variable to another type |
+| MissedCastOpportunity.cs:40:9:44:9 | foreach (... ... in ...) ... | This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'. | MissedCastOpportunity.cs:42:13:42:32 | ... ...; | casts its iteration variable to another type |
+| MissedCastOpportunity.cs:50:9:54:9 | foreach (... ... in ...) ... | This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'. | MissedCastOpportunity.cs:52:13:52:32 | ... ...; | casts its iteration variable to another type |

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

@@ -0,0 +1 @@
+Linq/MissedCastOpportunity.ql

csharp/ql/test/query-tests/Linq/MissedCastOpportunity/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