Time to buy ReSharper?

Today I spent a few hours on mysterious error. One automated test failed, and since it was actually an integration test with complex setup, tracking down the failure was not easy. The problem that I’ve eventually found can be illustrated with a couple of tests:

[Test]
public void BadClosureTest()
{
    int[] numbers = new int[]
    {
        1, 2, 3
    };

    List> functions = new List>();
    foreach (int n in numbers)
    {
        functions.Add(new Func(() => n));
    }

    // Will fail on first assertion!
    Assert.That(functions[0].DynamicInvoke(), Is.EqualTo(1));
    Assert.That(functions[1].DynamicInvoke(), Is.EqualTo(2));
    Assert.That(functions[2].DynamicInvoke(), Is.EqualTo(3));
}

[Test]
public void GoodClosureTest()
{
    int[] numbers = new int[]
    {
        1, 2, 3
    };

    List> functions = new List>();
    foreach (int n in numbers)
    {
        int m = n;
        functions.Add(new Func(() => m));
    }

    Assert.That(functions[0].DynamicInvoke(), Is.EqualTo(1));
    Assert.That(functions[1].DynamicInvoke(), Is.EqualTo(2));
    Assert.That(functions[2].DynamicInvoke(), Is.EqualTo(3));
}

As you can see, the only difference between these tests is a line in the second test assigning iterator “n” to a variable “m” defined within the “foreach” scope. Why does this matter?

Well, for us, procedural languages veterans the difference may be not so obvious at first glance. But if you happen to have ReSharper, it will immediately give you a warning about the first test: “Access to modified closure”. And will suggest to copy “n” to a local variable. I didn’t have ReSharper, so I came to the same conclusion in a hard way.

A lambda-expression “() => n” is not just a delegate – it’s a closure. It uses a variable that is defined outside its scope. Such variables are always treated by reference, even though they may be of a value type. So as long as the variable “n” lives, any changes applied to it will affect every closure where it is referenced.

And apparently ReSharper can warn us about improper use of closures.

By the way, if instead of “foreach” test had a “for” loop with an index, the first test would fail with IndexOutOfRangeException. Because closures were accessed after the array elements are iterated and index would be set to 3.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s