dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

[Proposal] Warn/error when foreach iteration variable is explicitly typed differently from the T in the IEnumerable<T> #1494

Open Joe4evr opened 6 years ago

Joe4evr commented 6 years ago

This is something that's been on my mind since I found out about it last June, and I filed dotnet/roslyn#20314 for it.

Quick recap:

using System.Collections.Generic;
using System.Linq;

public class Foo
{
    // this was an int before the loop was written
    // but after that and a brief discussion
    // it was decided to make it a string instead
    public string Group { get; set; }
}

public class C
{
    public void M(IEnumerable<Foo> foos)
    {
        foreach (IGrouping<int, Foo> fooGroup in foos.GroupBy(p => p.Group))
        {
        }
    }
}

I expect that the IGrouping<string, Foo> that is being enumerated from the GroupBy() call isn't compatible with the type IGrouping<int, Foo>, but the compiler accepts this code and silently inserts a cast instead. This only happens in foreach loops, as far as I see, since any other way to attempt to assign something like this without visibly casting is blocked by the compiler. And although Cyrus gave an example in the linked issue of when the cast would succeed, I expect that to be a pretty extreme edge-case scenario.

I imagine it likely has to do with supporting foreach on the non-generic IEnumerable/IEnumerator interfaces (casting object up to the expected type) and those should probably be exempt from being warned on, but when just about everyone is using the generic versions, it's really something that should be brought to the developer's attention.

Like, "hey, it appears you don't like using var to prevent code like this from throwing an exception at runtime, but this statement will likely throw an exception at runtime". :trollface:

But seriously, the primary suggested action should be to correct the type of the iteration variable to the type of T that is returned from the method call. Just, if offering var is the secondary suggestion, it would maybe push some devs into embracing var a little more often.

jamesqo commented 6 years ago

I think this is a good idea, but it should not kick-in for nongeneric collections that only implement IEnumerable.

Youssef1313 commented 1 year ago

@Evangelink Should we close this as fixed in https://github.com/dotnet/roslyn/pull/60120 ?