Home > C#, delegates, foreach, lambdas > The classic delegate/foreach interaction bug (and a solution)

The classic delegate/foreach interaction bug (and a solution)

Update (2012-10-05): This whole issue has now been fixed in C# 5! The compiler just copies the loop variable for you, if you capture it in a closure. This is actually a breaking change, but a perfectly sensible one. Note that this only affects foreach. The plain for loop is unaffected.

Update (2011-06-17): Since I posted this I’ve regularly been told that there’s another solution where you declare a local variable inside the loop and copy the loop variable into it. Thanks, but I know that already! That’s the first solution everyone learns. The idea here was to suggest an alternative solution that works more elegantly, without adding extra clutter.

If you’ve used C# anonymous delegates or lambdas for a while (or anonymous functions in JavaScript) you will have encountered this problem. You make a delegate for each item in a list, and they all seem to wind up referring to the last item on the list. FUME!

It’s just an unfortunate side effect of the way foreach works. It reuses the same loop variable each time around the loop, and so that loop variable is created outside the scope of the loop, and so all the delegates you create inside the loop are actually looking at the same variable, not their own local copies.

This demonstrates the problem, and also gives a neat solution:

static class Extensions
{
 public static void ForEach<T>(this IEnumerable<T> on,
                               Action<T> action)
 {
     foreach (T item in on)
         action(item);
 }
}

class Program
{
 static void Main(string[] args)
 {
     // A list of actions to execute later
     List<Action> actions = new List<Action>();

     // Numbers 0 to 9
     IEnumerable<int> numbers = Enumerable.Range(0, 10);

     // Store an action that prints each number (WRONG!)
     foreach (int number in numbers)
         actions.Add(() => Console.WriteLine(number));

     // Run the actions, we actually print 10 copies of "9"
     foreach (Action action in actions)
         action();

     // So try again
     actions.Clear();

     // Store an action that prints each number (RIGHT!)
     numbers.ForEach(number =>
         actions.Add(() => Console.WriteLine(number)));

     // Run the actions
     foreach (Action action in actions)
         action();
 }
}

By using ForEach (an extension method) instead of using foreach directly, we make the problem disappear, because now each time around the loop we have a unique number variable which is properly captured by the delegates we created.

Notes:

  • The ForEach extension method I define above is actually already available as a method in the List<T> class, so if you’re looping over a List<T>, you don’t need that extension method.
  • That extension method ought to be added to the System.Linq.Enumerable class, oughtn’t it?
  • One situation this can’t help with is yield return. If you are looping through a list of items, and then you want to yield return each item, you’re out of luck with this solution.
Advertisements
  1. Anupheaus
    July 4, 2012 at 2:12 pm

    Hi,

    I only just discovered your blog and I’ve already been pretty blown away with some of the stuff I’ve found and I’m going to make this a regular stop when I trawl the web for stuff in future, but I’m wondering if this can be extension method could be enhanced; for example:

    public static IEnumerable ForEach(this IEnumerable on, Action action) {
    foreach(T item in on) {
    action(item);
    }
    return on;
    }

    So that you can continue using the enumerable even after you’ve performed some action on each item? Since I’m no expert, I’m wondering if you can see any any issues with doing this?

    • earwicker
      July 10, 2012 at 7:38 pm

      Thanks, glad you’re enjoying it. I think it’s important to distinguish between methods that have side-effects and methods that are pure computations. At the extreme this produces the principle of CSQ. It doesn’t apply in every situation, e.g. a thread-safe queue MUST have an operation that pops from the queue AND returns the popped item in a single atomic operation. But where it makes sense, it’s a good idea to distinguish imperative state-modifying code from functional, expression evaluating code.

      The key to “LINQ” style is not just chaining methods. It’s that each step in the chain produces a new object, rather than having side-effects on other objects. Mixing the two is a recipe for confusion and unintended side-effects.

      Also I never get bitten by the bug I described here, because I use Resharper, which tells me if I write a lambda that depends on variable that might change in this way. So for all these reasons I use foreach as it is.

  1. No trackbacks yet.

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

%d bloggers like this: