Why is a temporary variable required in foreach to be included in lambda expression?

Question

I was reading C# 4 in a Nutshell and I've come to this piece of code:

IQueryable<Product> SearchProducts (params string[] keywords)
{
  IQueryable<Product> query = dataContext.Products;

  foreach (string keyword in keywords)
  {
    string temp = keyword;
    query = query.Where (p => p.Description.Contains (temp));
  }
  return query;
}

Right after the code there's a 'warning' that goes like this:

The temporary variable in the loop is required to avoid the outer variable trap, where the same variable is captured for each iteration of the foreach loop.

I don't get it, I don't understand why the temp variable is necessary. What is the outter variable trap?

From: http://www.albahari.com/nutshell/predicatebuilder.aspx

Can anybody please clarify this?

You definitely want to read [this article](http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx). — David Schwartz, Mar 24 '12 at 22:03
See this: http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx — Phil, Mar 24 '12 at 22:03
Everybody suggested the same blog post, how come I've never heard of Eric before? thanks all — sebagomez, Mar 24 '12 at 22:08
@sebastian: We can't know why you haven't heard of him before, but you've got a lot of catching up to do. Eric's blog is a *treasure trove*. Have an enjoyable few months reading it :) — Jon Skeet, Mar 24 '12 at 22:09
Related: http://stackoverflow.com/questions/7133816 (the culprit is `foreach`) — , Mar 24 '12 at 22:11
I've written a FxCop rule for that (it's in fxcopcontrib on codeplex) but it isn't fully polished yet. — jessehouwing, Mar 24 '12 at 23:13

score 5 · Accepted Answer · 2012-03-24T22:16:57.387

5

Because there is only one variable called keyword that is closed over. The temporary variable, however, is different each iteration.

Thus, without the temporary variable, when the lambda is executed later, keyword evaluates to the last value it was assigned in the loop.

Another solution, in some cases, is to force the evaluation (and end up with a List or such).

edited Mar 24 '12 at 22:16

answered Mar 24 '12 at 22:02

`later`, that's the keyword! I didn't see that, Thanks! – sebagomez Mar 24 '12 at 22:05

David Schwartz · Answer 2 · 2012-03-24T22:23:11.927

The problem is that keyword is changing. The => delegate closes on the current value of the variable, not the value it had back in the past when the delegate was created. There's a detailed explanation in Eric Lippert's blog post.

This classic C bug is the same mistake:

#include <stdio.h> 
#include <pthread.h>

void * MyThreadFunction(void *x)
{
   printf("I am thread %d\n", * (int *) x);
   return NULL;
}

int main(void)
{
   int i;
   pthread_t t[10];
   void *ret;
   for(i=0; i<10; i++)
       pthread_create(&t[i], NULL, MyThreadFunction, (void *) &i);
   for(i=0; i<10; i++)
       pthread_join(t[i], &ret);
}

The * (int *) x gets the current value of i, not the value when the thread was created.

That's a nice article, but it annoys me there is not a small excerpt / summary that can be used with minimal context in it :( — , Mar 24 '12 at 22:08

Why is a temporary variable required in foreach to be included in lambda expression?

2 Answers2

Linked