IDE0059 - Remove redundant assignment triggered for variable used from closure

[bartdesmet]

Version Used: VS 16.0 Preview 2.0

Steps to Reproduce:

int i = 1;
Action a = () => Console.WriteLine(i);

Expected Behavior:

IDE0059 is not fired, and no suggested fix to remove a redundant assignment is made.

Actual Behavior:

IDE0059 fires and suggests to remove the redundant assignment, which results in:

int i;
Action a = () => Console.WriteLine(i);

which doesn't compile. Adding a use site outside the lambda/delegate body makes IDE0059 go away.

Context:

The bigger repro for this issue is a place where we cache delegates that have a reference to a loop variable, which is copied to a local inside the body of a for loop, like this:

Action[] GetCachedDelegates(int n)
{
    var res = new Action[n];

    for (var i = 0; i < n; i++)
    {
        var j = i;
        res[i] = () => DoSomething(j); // closure over `this` and `j`
    }

    return res;
}

[mavasani]

Thanks for the repro cases. The original repro case is by design, hopefully the below explains why:

class C
{
    void M()
    {
        int i = 1;
        Action a = () => Console.WriteLine(i);
        // Commenting out the below code leads to assignments to 'a' and 'i' being unused.
        a();
    }
}

However, the repro case provided in Context is a false positive, our flow analysis has multiple bail out cases for lambdas/delegates escaping the method (such as assignment to fields/properties, return value, assignment to out/ref params, etc.), but it is not treating res[i] = () => DoSomething(j); as an escape, leading to the false result. I will fix the analyzer to handle such escapes.

[bartdesmet]

The first case makes sense given that there are no observable side effects to constructing a delegate and its closure. However, removing i and not a as well in a quick fix seems unexpected to me. IIRC, that's exactly what I saw happen and the use site of i became invalid after taking away i.