What am I missing in this chain of predicates?

Question

NOTE: Right before posting this question it occurred to me there's a better way of doing what I was trying to accomplish (and I feel pretty stupid about it):

IEnumerable<string> checkedItems = ProductTypesList.CheckedItems.Cast<string>();
filter = p => checkedItems.Contains(p.ProductType);

So OK, yes, I already realize this. However, I'm posting the question anyway, because I still don't quite get why what I was (stupidly) trying to do wasn't working.

---

I thought this would be extremely easy. Turns out it is giving me quite a headache.

The basic idea: display all the items whose ProductType property value is checked in a CheckedListBox.

The implementation:

private Func<Product, bool> GetProductTypeFilter() {
    // if nothing is checked, display nothing
    Func<Product, bool> filter = p => false;

    foreach (string pt in ProductTypesList.CheckedItems.Cast<string>()) {
        Func<Product, bool> prevFilter = filter;
        filter = p => (prevFilter(p) || p.ProductType == pt);
    }

    return filter;
}

However, say the items "Equity" and "ETF" are both checked in ProductTypesList (a CheckedListBox). Then for some reason, the following code only returns products of type "ETF":

var filter = GetProductTypeFilter();
IEnumerable<Product> filteredProducts = allProducts.Where(filter);

I guessed it might have had something to do with some self-referencing messiness where filter is set to, essentially, itself or something else. And I thought that maybe using ...

filter = new Func<Product, bool>(p => (prevFilter(p) || p.ProductType == pt));

...would do the trick, but no such luck. Can anybody see what I am missing here?

Answer 1

I believe you have a modified closure problem here. The pt parameter is bound into the lambda expression but changes as the loop progresses. It's important to realize the when a variable is referenced in a lambda it is the variable that is captured, not the value of the variable.

In loops this has a very significant ramification - because the loop variable is changing, not being redefined. By creating a variable inside the loop, you are creating a new variable for each iteration - which then alows the lambda to capture each independently.

The desired implementation would be:

foreach (string pt in ProductTypesList.CheckedItems.Cast<string>()) {
    string ptCheck = pt;
    Func<Product, bool> prevFilter = filter;
    filter = p => (prevFilter(p) || p.ProductType == ptCheck);
}

Eric Lippert has written about this specific situation:

• http://blogs.msdn.com/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx
• http://blogs.msdn.com/ericlippert/archive/2009/11/16/closing-over-the-loop-variable-part-two.aspx

Also, see the question Access to Modified Closure (2) for a good explanation of what happens with closure variables. There's also an series of articles on the blog The Old New Thing that has an interesting perspective on this:

• http://blogs.msdn.com/oldnewthing/archive/2006/08/02/686456.aspx
• http://blogs.msdn.com/oldnewthing/archive/2006/08/03/687529.aspx
• http://blogs.msdn.com/oldnewthing/archive/2006/08/04/688527.aspx

Answer 2

It has to do with closures. The variable pt will always refer to the last value of the for loop.

Consider the following example where the output is the one expected because it's using a variable that is scoped inside the for loop.

public static void Main(string[] args)
{
    var countries = new List<string>() { "pt", "en", "sp" };

    var filter = GetFilter();

    Console.WriteLine(String.Join(", ", countries.Where(filter).ToArray()));
}

private static Func<string, bool> GetFilter()
{
    Func<string, bool> filter = p => false;

    foreach (string pt in new string[] { "pt", "en" })
    {
        Func<string, bool> prevFilter = filter;

        string name = pt;

        filter = p => (prevFilter(p) || p == name);
    }

    return filter;
}

Answer 3

Since you're looping and setting the filter type to itself, you're setting the product type to the last pt in each case. It's a modified closure and since it's delay bound, you need to copy it on each loop, like this:

foreach (string pt in ProductTypesList.CheckedItems.Cast<string>()) {
    var mypt = pt;
    Func<Product, bool> prevFilter = filter;
    filter = p => (prevFilter(p) || p.ProductType == mypt);
}

This should result in the right result, otherwise the last pt is used for all equality checks.