Resharper removes yield from foreach. Why?

Question

I recently learned about yield and then created the following test console program:

    public static string Customers = "Paul,Fred,Doug,Mark,Josh";
    public static string Admins = "Paul,Doug,Mark";

    public static void Main()
    {
        var test = CreateEfficientObject();

        Console.WriteLine(test.Admins.FirstOrDefault());
        //Note that 'GetAllCustomers' never runs. 
    }

    public static IEnumerable<string> GetAllCustomers()
    {
        var databaseFetch = Customers.Split(',');
        foreach (var s in databaseFetch)
        {
            yield return s;
        }
    }

    public static IEnumerable<string> GetAllAdmins()
    {
        var databaseFetch = Admins.Split(',');
        foreach (var s in databaseFetch)
        {
            yield return s;
        }
    }

    static LoginEntitys CreateEfficientObject()
    {
        var returnObject = new LoginEntitys {};
        returnObject.Admins = GetAllAdmins();
        returnObject.Customers = GetAllCustomers();
        return returnObject;
    }
}
public class LoginEntitys
{
    public IEnumerable<String> Admins { get; set; }
    public IEnumerable<String> Customers { get; set; }
}

Yet I noticed Resharper wants to convert my foreach loops to :

public static IEnumerable<string> GetAllCustomers()
{
    var databaseFetch = Customers.Split(',');
    return databaseFetch;
}

Why does Resharper want to remove yield from this case? It changes the functionality completely as it will no longer lazy load without yield. I can only guess that either

• A) I am using yield incorrectly/in bad pratice
• B) It's a Resharper bug/suggestion that can just be ignored.

Any insight would be great.

Answer 1

You are correct that this proposed transformation changes the functionality of the code in subtle ways, preventing it from deferring the evaluation of the properties and performing the Split from being evaluated as early.

Perhaps those that implemented it were well aware that it was a change in functionality and felt that it was still a useful suggestion, one that could be ignored if the existing semantics were important, or if they actually failed to realize that the semantics were being altered. There's no good way for us to know, we can only guess. If those semantics are important for your program, then you are correct to not make the suggested transformation.

Answer 2

I think Resharper is being a bit dumb here, in the sense that its applying a standard "convert foreach to LINQ" transform without being aware of the context.

It doesn't suggest the same edits for a while loop:

public static IEnumerable<string> ReadLineFromFile(TextReader fileReader)
{
    using (fileReader)
    {
        string currentLine;
        while ((currentLine = fileReader.ReadLine()) != null)
        {
            yield return currentLine;
        }
    }
}

I guess the next iteration of Resharper which uses Roslyn will be much more context aware.

Thanks @servy for an engaging and refreshing discussion!

Answer 3

The code in your example is not calling the iterator on the IEnumerable you are returning. If you were using the result of GetAllAdmins() in a LINQ query for example the yield would be useful because execution of the expression could resume on each iteration.

I would imagine Resharper is just suggesting you remove unused code.