diff --git a/csharp/ql/lib/Linq/Helpers.qll b/csharp/ql/lib/Linq/Helpers.qll index a628c717277..541f7a380e6 100644 --- a/csharp/ql/lib/Linq/Helpers.qll +++ b/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`. + */ +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 | diff --git a/csharp/ql/src/Linq/MissedAllOpportunity.ql b/csharp/ql/src/Linq/MissedAllOpportunity.ql index 54c2e7b0c46..54fe40faeec 100644 --- a/csharp/ql/src/Linq/MissedAllOpportunity.ql +++ b/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(...)'." diff --git a/csharp/ql/src/Linq/MissedCastOpportunity.ql b/csharp/ql/src/Linq/MissedCastOpportunity.ql index 24d1c87bd2a..8d3de1f31b4 100644 --- a/csharp/ql/src/Linq/MissedCastOpportunity.ql +++ b/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(...)'.", diff --git a/csharp/ql/src/Linq/MissedOfTypeOpportunity.ql b/csharp/ql/src/Linq/MissedOfTypeOpportunity.ql index 3a984d0cc3e..3d61acfa523 100644 --- a/csharp/ql/src/Linq/MissedOfTypeOpportunity.ql +++ b/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, diff --git a/csharp/ql/src/Linq/MissedSelectOpportunity.ql b/csharp/ql/src/Linq/MissedSelectOpportunity.ql index 1714aed81da..9f36c3de82b 100644 --- a/csharp/ql/src/Linq/MissedSelectOpportunity.ql +++ b/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) diff --git a/csharp/ql/src/Linq/MissedWhereOpportunity.ql b/csharp/ql/src/Linq/MissedWhereOpportunity.ql index 4c93b7ab6bf..401248d6176 100644 --- a/csharp/ql/src/Linq/MissedWhereOpportunity.ql +++ b/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) diff --git a/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.cs new file mode 100644 index 00000000000..fe5617c228a --- /dev/null +++ b/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 animals) + { + // BAD: Can be replaced with animals.Cast(). + 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(). + foreach (Animal animal in animals) + { + Dog d = (Dog)animal; + d.Woof(); + } + } + + public void M4(Array animals) + { + // BAD: Can be replaced with animals.Cast(). + foreach (Animal animal in animals) + { + Dog d = (Dog)animal; + d.Woof(); + } + } + + public void M5(IEnumerable animals) + { + // BAD: Can be replaced with animals.Cast(). + foreach (object animal in animals) + { + Dog d = (Dog)animal; + d.Woof(); + } + } + + public class NonEnumerableClass + { + public IEnumerator 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 + "."); + } + } +} \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.expected new file mode 100644 index 00000000000..7e08d734acc --- /dev/null +++ b/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 | diff --git a/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.qlref b/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.qlref new file mode 100644 index 00000000000..8d70f999503 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/MissedCastOpportunity.qlref @@ -0,0 +1 @@ +Linq/MissedCastOpportunity.ql diff --git a/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/options b/csharp/ql/test/query-tests/Linq/MissedCastOpportunity/options new file mode 100644 index 00000000000..75c39b4541b --- /dev/null +++ b/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 diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs new file mode 100644 index 00000000000..d1326c70ee2 --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.cs @@ -0,0 +1,83 @@ +using System; +using System.Linq; +using System.Collections.Generic; + +class MissedWhereOpportunity +{ + public void M1(List lst) + { + // BAD: Can be replaced with lst.Where(e => e % 2 == 0) + foreach (int i in lst) + { + if (i % 2 != 0) + continue; + Console.WriteLine(i); + Console.WriteLine((i / 2)); + } + + // BAD: Can be replaced with lst.Where(e => e % 2 == 0) + foreach (int i in lst) + { + if (i % 2 == 0) + { + Console.WriteLine(i); + Console.WriteLine((i / 2)); + } + } + } + + public void M2(NonEnumerableClass nec) + { + // GOOD: Linq can't be used here. + foreach (int i in nec) + { + if (i % 2 == 0) + { + Console.WriteLine(i); + Console.WriteLine((i / 2)); + } + } + } + + public void M3(int[] arr) + { + // BAD: Can be replaced with arr.Where(e => e % 2 == 0) + foreach (var n in arr) + { + if (n % 2 == 0) + { + Console.WriteLine(n); + Console.WriteLine((n / 2)); + } + } + } + + public void M4(Array arr) + { + // GOOD: Linq can't be used here. + foreach (var element in arr) + { + if (element.GetHashCode() % 2 == 0) + { + Console.WriteLine(element); + } + } + } + + public void M5(IEnumerable elements) + { + // BAD: Can be replaced with elements.Where(e => e.GetHashCode() % 2 == 0) + foreach (var element in elements) + { + if (element.GetHashCode() % 2 == 0) + { + Console.WriteLine(element); + } + } + } + + public class NonEnumerableClass + { + public IEnumerator GetEnumerator() => throw null; + } +} diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected new file mode 100644 index 00000000000..5efde9aebed --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.expected @@ -0,0 +1,4 @@ +| MissedWhereOpportunity.cs:10:9:16:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:12:17:12:26 | ... != ... | implicitly filters its target sequence | +| MissedWhereOpportunity.cs:19:9:26:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:21:17:21:26 | ... == ... | implicitly filters its target sequence | +| MissedWhereOpportunity.cs:45:9:52:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:47:17:47:26 | ... == ... | implicitly filters its target sequence | +| MissedWhereOpportunity.cs:70:9:76:9 | foreach (... ... in ...) ... | This foreach loop $@ - consider filtering the sequence explicitly using '.Where(...)'. | MissedWhereOpportunity.cs:72:17:72:46 | ... == ... | implicitly filters its target sequence | diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.qlref b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.qlref new file mode 100644 index 00000000000..4a08b459a6c --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/MissedWhereOpportunity.qlref @@ -0,0 +1 @@ +Linq/MissedWhereOpportunity.ql diff --git a/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/options b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/options new file mode 100644 index 00000000000..75c39b4541b --- /dev/null +++ b/csharp/ql/test/query-tests/Linq/MissedWhereOpportunity/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