Linq query built in foreach loop always takes parameter value from last iteration

Question

I have a List containing several keywords. I foreach through them building my linq query with them like so (boiled down to remove the code noise):

List<string> keys = FillKeys()
foreach (string key in keys){
    q = q.Where(c => c.Company.Name.Contains(key));
}

When I now make my keys contain 2 keys that return results seperatly, but can never occure together (every item in q is either "xyz" or "123", never "123" AND "xyz"), I still get results. The resultset is then the same as the last string it got to.

I had a look at the linq query and it appears it creates the correct sql, but it replaces @p1 AND @p2 both by the same (last itterated) value.

What am I doing wrong?

Solution 1

You're reusing the same variable (key) in your lambda expression.

See my article on anonymous methods for more details, and there are a number of related SO questions too:

• LINQ to SQL bug (or very strange feature)...
• Local variables with delegates
• C# captured variable in a loop
• C# gotcha answer
• Building a LINQ query programmatically without local variables tricking me

The simple fix is to copy the variable first:

List<string> keys = FillKeys()
foreach (string key in keys){
    string copy = key;
    q = q.Where(c => c.Company.Name.Contains(copy));
}

Solution 2

Possibly a captured variable issue; try adding:

List<string> keys = FillKeys()
foreach (string key in keys){
    string tmp = key;
    q = q.Where(c => c.Company.Name.Contains(tmp));
}

Solution 3

it’s been fixed in C# 5.0, and the example above in C# 5.0 works but fails in earlier versions of C#.

But be careful, it does not concern a for loop

  static void Main()
        {
            IEnumerable<char> query = "aaa bbb ccc";
            string lettersToRemove = "ab";

            Console.WriteLine("\nOK with foreach:");
            foreach (var item in lettersToRemove)
            {
                query = query.Where(c => c != item);   
            }   
            foreach (char c in query) Console.Write(c);

            //OK:
            Console.WriteLine("\nOK with foreach and local temp variable:");
            query = "aaa bbb ccc";

            foreach (var item in lettersToRemove)
            {
                var tmp = item;
                query = query.Where(c => c != tmp);
            }            
            foreach (char c in query) Console.Write(c);


            /*
             An IndexOutOfRangeException is thrown because:
             firstly compiler iterates the for loop treating i as an outsite declared variable  
             when the query is finnaly invoked the same variable of i is captured (lettersToRemove[i] equals 3) which generates IndexOutOfRangeException

             The following program writes aaa ccc instead of writing ccc:
             Each iteration gets the same variable="C", i (last one frome abc). 
             */

            //Console.WriteLine("\nNOK with for loop and without temp variable:");
            //query = "aaa bbb ccc";

            //for (int i = 0; i <  lettersToRemove.Length; i++)
            //{
            //    query = query.Where(c => c != lettersToRemove[i]);
            //}
            //foreach (char c in query) Console.Write(c);

            /*
             OK
             The solution is to assign the iteration variable to a local variable scoped inside the loop
             This causes the closure to capture a different variable on each iteration.
            */
            Console.WriteLine("\nOK with for loop and with temp variable:");
            query = "aaa bbb ccc";

            for (int i = 0; i < lettersToRemove.Length; i++)
            {
                var tmp = lettersToRemove[i];
                query = query.Where(c => c != tmp);
            }
            foreach (char c in query) Console.Write(c);
        } 

Comments

[Boris Callens]

Hehe, found this one in the meantime: stackoverflow.com/questions/190227/… Easy (but earned) credits to you ;)

[Boris Callens]

Thanks. Basically same thing as skeet, but I can only assign one solution. Points though.

[Marc Gravell]

No problem. I find it reassuring that we gave the same reply ;-p