[New analyzer] SA1143 Use implicitly assignable foreach variable type

maxkoshevoi commented 3 years ago

Describe the problem you are trying to solve

As described here and here foreach can throw InvalidCastException at runtime because of hidden explicit cast that is does under the hood.

This is very dangerous behavior for statically typed language to have since programmers don't expect successfully compiled code to throw InvalidCastException when they didn't do any casting. I came across this behavior when I changed type of a collection to more generic one and then got this error at runtime.

This behavior was needed until generics appeared in C# 2, but it's still here with us 15 years after it's introduction.

Describe suggestions on how to achieve the rule

While solution could be as easy as "always use var for foreach items", I'm not a fan of this approach. var makes code less readable in this scenario, and forcing someone to use var is not in the spirit of this keyword anyway (it was made to be optional).

I'm proposing analyzer that would detect that instance of type T from IEnumerable<T> that is passed to foreach can be cast to type specified for item of the foreach.

Something like this, but using IsAssignableFrom:

List<A> aList = new() { new() };
Foreach<B, A>(aList);

List<B> bList = new() { new() };
Foreach<A, B>(bList);

void Foreach<TItem, TCollectionValue>(IEnumerable<TCollectionValue> collection) where TCollectionValue : TItem
{
    foreach (TItem item in collection)
    {
    }
}

class A { }
class B : A { }

Additional context

Here is code to reproduce the issue:

List<A> aList = new() { new() };
foreach (B item in aList)
{
}

class A { }
class B : A { }

maxkoshevoi commented 3 years ago

Regarding what is and isn't a false positive, there are too two groups so far:

object collections (like ArrayList). A lot of code is already written for them and it expects foreach to cast them to some type, so there shouldn't be any surprise that it performs explicit cast in their case. I excluded such collections from this analyzer, so this is no longer a false positive. I'm not sure if generic collections with object as type argument (List<object>) should be included in this bucket though. Would like to hear any thoughts on this.
Collections that always return one type. For example:
```
foreach (string item in GenerateSequence())
{
}

IEnumerable<IComparable> GenerateSequence()
{
}
```
Here's an example from dotnet/runtime. Here explicit cast is performed since Instantiation returns TypeDesc and GenericParameterDesc is derived from TypeDesc. Should it be flagged with this analyzer to manually perform the cast? Also would like to hear any thoughts on this. Here is another example (schema.Includes is XmlSchemaObject)

As for other false positives, I didn't find any so far (at least in dotnet/runtime nothing is flagged if I exclude everything mentioned above).

sharwell commented 3 years ago

This is more a usage rule than a style rule. I have no objection to someone implementing the analyzer in another repository, but it doesn't really fit the goals of StyleCop Analyzers.

DotNetAnalyzers / StyleCopAnalyzers