C# object property not accessible in Enumerable.Where within a foreach

Question

Found a c# head-scratcher. In the foreach loop, using the parent.Id property directly in a Enumerable.Where does not work. Putting it in a variable first works. There is no issue with directly accessing parent.Id in the Select statement.

    List<Person> people = new List<Person>() { 
        new Person() { Id = 1, name = "John", parentId = null },
        new Person() { Id = 2, name = "Sarah", parentId = null },
        new Person() { Id = 3, name = "Daniel", parentId = 1 },
        new Person() { Id = 4, name = "Peter", parentId = 1 }
    };

    List<object> peopleTree = new List<object>();

    var parents = people.Where(p => !p.parentId.HasValue);
    foreach (Person parent in parents)
    {
        int parentId = parent.Id;
        var children = people
            //.Where(p => p.parentId.Equals(parentId)) //This works, is able to find the children
            .Where(p => p.parentId.Equals(parent.Id)) //This does not work, no children for John
            .Select(p => new { Id = p.Id, Name = p.name, pId = parent.Id }); //pId set correctly

        peopleTree.Add(new
        {
            Id = parent.Id,
            Name = parent.name,
            Children = children
        });
    }

Alternatively, if I use a for loop and put parent in a variable first, I can access the parent.Id property directly in the Where statement.

var parents = people.Where(p => !p.parentId.HasValue).ToArray();
for (int idx = 0; idx < parents.Count(); idx++)
{
    var parent = parents[idx];
...

I could not find an answer to why it behaves like this. Can anyone explain it?

Answer 1

This problem is created by the deferred execution of the children. Essentially, the value of parent at the time when children is evaluated is different. Geekspeak for this is Accessing Modified Closure.

You can fix it by introducing a temporary the way you did, or by forcing evaluation to happen while the foreach loop is still in the current iteration:

var children = people
    .Where(p => p.parentId.Equals(parent.Id))
    .Select(p => new { Id = p.Id, Name = p.name, pId = parent.Id })
    .ToList();

Answer 2

This is caused by the lazy nature of linq queries. Linq queries will "materialize" as late as possible to avoid doing potentially unnecessary work.

children is a non-materialized IEnumerable<T>. It won't be actually filled with elements. There is a significant difference between parent and parentId, used in your two .Where() calls. parent is only declared once, but parentId is scoped inside the loop, so effectively is declared multiple times. At the time children is eventually materialized parent has changed values. It will refer to the last element in parents, which is not what you intended.

You can force eager evaluation like this.

    var children = people
        .Where(p => p.parentId.Equals(parent.Id)) 
        .Select(p => new { Id = p.Id, Name = p.name, pId = parent.Id })
        .ToArray();  <---- this forces materialization

Answer 3

The problem is on the statement that begins like this:

var children = people ...

This statement does not result it in a collection that actually stores values... it results in an IEnumerable object that knows how to iterate over a collection. The instructions used by this object happen to reference the parent variable from the loop. That variable is captured for the Enumerable into something called a closure. Later on, when you actually use the Enumerable object to access items, it looks back at that parent variable.

Here's the trick: there is one parent variable that mutates for each iteration through the original loop. At the end of the loop, all of the items in your parents collection are using the same parent object. Copying the parent.Id value to a variable inside the loop fixes the problem because you're now dealing with a new variable for the closure with each iteration through the loop.

You can also fix this by using a .ToList() call at the end of the statement indicated earlier to evaluate the Enumerable object while still inside the loop. However, I prefer your existing solution, because it's more memory efficient if you never need to expand all of those Children at the same time.

The good news is that this problem is fixed for C# 5.