Access to Foreach Variable in Closure Warning

Access to foreach variable in closure warning

There are two parts to this warning. The first is...

Access to foreach variable in closure

...which is not invalid per se but it is counter-intuitive at first glance. It's also very hard to do right. (So much so that the article I link to below describes this as "harmful".)

Take your query, noting that the code you've excerpted is basically an expanded form of what the C# compiler (before C# 5) generates for foreach¹:

I [don't] understand why [the following is] not valid:

string s; while (enumerator.MoveNext()) { s = enumerator.Current; ...

Well, it is valid syntactically. And if all you're doing in your loop is using the value of s then everything is good. But closing over s will lead to counter-intuitive behaviour. Take a look at the following code:

var countingActions = new List<Action>();

var numbers = from n in Enumerable.Range(1, 5)
              select n.ToString(CultureInfo.InvariantCulture);

using (var enumerator = numbers.GetEnumerator())
{
    string s;

    while (enumerator.MoveNext())
    {
        s = enumerator.Current;

        Console.WriteLine("Creating an action where s == {0}", s);
        Action action = () => Console.WriteLine("s == {0}", s);

        countingActions.Add(action);
    }
}

If you run this code, you'll get the following console output:

Creating an action where s == 1
Creating an action where s == 2
Creating an action where s == 3
Creating an action where s == 4
Creating an action where s == 5

This is what you expect.

To see something you probably don't expect, run the following code immediately after the above code:

foreach (var action in countingActions)
    action();

You'll get the following console output:

s == 5
s == 5
s == 5
s == 5
s == 5

Why? Because we created five functions that all do the exact same thing: print the value of s (which we've closed over). In reality, they're the same function ("Print s", "Print s", "Print s"...).

At the point at which we go to use them, they do exactly what we ask: print the value of s. If you look at the last known value of s, you'll see that it's 5. So we get s == 5 printed five times to the console.

Which is exactly what we asked for, but probably not what we want.

The second part of the warning...

May have different behaviour when compiled with different versions of compiler.

...is what it is. Starting with C# 5, the compiler generates different code that "prevents" this from happening via foreach.

Thus the following code will produce different results under different versions of the compiler:

foreach (var n in numbers)
{
    Action action = () => Console.WriteLine("n == {0}", n);
    countingActions.Add(action);
}

Consequently, it will also produce the R# warning :)

My first code snippet, above, will exhibit the same behaviour in all versions of the compiler, since I'm not using foreach (rather, I've expanded it out the way pre-C# 5 compilers do).

Is this for CLR version?

I'm not quite sure what you're asking here.

Eric Lippert's post says the change happens "in C# 5". So presumably you have to target .NET 4.5 or later with a C# 5 or later compiler to get the new behaviour, and everything before that gets the old behaviour.

But to be clear, it's a function of the compiler and not the .NET Framework version.

Is there relevance with IL?

Different code produces different IL so in that sense there's consequences for the IL generated.

¹ foreach is a much more common construct than the code you've posted in your comment. The issue typically arises through use of foreach, not through manual enumeration. That's why the changes to foreach in C# 5 help prevent this issue, but not completely.

How do I fix: Access to foreach variable in closure resharper warning?

A block scoped variable should resolve the warning.

@foreach(var item in Model)
{
    var myItem = item;
    <div>@Html.DisplayBooleanFor(modelItem => myItem.BooleanField)</div>
}

Foreach variable in closure

.Net 4.0 is irrelevant here. Only thing is the c# compiler. Starting from C# 5.0 behavior is changed. I presume you're using C# 5.0 compiler.

This means that even in .Net 2.0 this code will work if you're using Visual studio 2012 (given that default C# compiler version is 5.0)

If you're using Visual studio 2012 or newer version by default C#5.0 compiler will be used and hence you don't see the bug.

Foreach Variable in Closure. Why Results Differ for These Snippets?

Resharper gives this description: "Access to foreach variable in closure. May have different behaviour when compiled with different versions of compiler." Why may it have a different behaviour?

There was a breaking change between C# 4 and C# 5 due to the way the loop variable in foreach was impacted by closures, notably since the introduction of lambda expressions in C# 3. Resharper is warning you of this, in case you might depend or otherwise have come to expect the former semantics.

The quick upshot is that in C# 4, the loop variable was shared between each iteration of the loop, and closures capture the variable, so it led to unexpected results for most people when they closed over the loop variable.

In C# 5, each iteration of the loop gets its own variable, so closures in one iteration do not close over the same variable as other iterations, leading to more expected outcomes (for most people).

That gets us to the heart of your problem:

In the first snippet all tasks start with their own message, while in the second one all tasks start with the same message?

In your first snippet, you are creating a copy of the loop variable inside your loop and the closure is occuring over the inner variable. In the second, you close over the loop variable directly. Presumably, you are running under C# 4, so the former semantics apply. If running in C# 5, the loop outputs from both versions should be consistent. This is the change Resharper refers to, and it should also let you understand how to structure your code in C# 4 (namely, use the first version you have written).

As Justin Pihony points out in the comments, Eric Lippert has written a very useful blog article on the former semantics that also alludes to the change for C# 5.

Is Access to Modified Closure warnings valid for string variable?

I presume delegate created with lambda will receive string by value since strings are immutable and each new "s" will be poiting to different memory.

Creating a delegate does not evaluate the variables. That's the whole point about closures: They close over the variables. So, all your lambdas will capture the same variable s, which might not be what you want. If it affects your code or not depends on when the lambdas are evaluated: If the call of Where already "executes" the lambda (and throws it away afterwards), you should be fine. If your regexes object stores the lambdas and uses them later, you're in trouble.

Thus, to be on the safe side, copy the value to a variable local to the inside of the loop first.

Parallel.ForEach - Access To Modified Closure Applies?

You are accessing a modified closure, so it does apply. But, you are not changing its value while you are using it, so assuming you are not changing the values inside UpdateUsageStats you don't have a problem here.

Parallel.Foreach waits for the execution to end, and only then are you changing the values in startTime and endTime.

ReSharper Warning - Access to Modified Closure

The reason for the warning is that inside a loop you might be accessing a variable that is changing. However, the "fix" isn't really doing anything for you in this non-loop context.

Imagine that you had a FOR loop and the if was inside it and the string declaration was outside it. In that case the error would be correctly identifying the problem of grabbing a reference to something unstable.

An example of what you don't want:

string acctStatus

foreach(...)
{
  acctStatus = account.AccountStatus[...].ToString();
  if (!SettableStatuses().Any(status => status == acctStatus))
      acctStatus = ACCOUNTSTATUS.Pending.ToString();
}

The problem is that the closure will grab a reference to acctStatus, but each loop iteration will change that value. In that case it would be better:

foreach(...)
{
  string acctStatus = account.AccountStatus[...].ToString();
  if (!SettableStatuses().Any(status => status == acctStatus))
      acctStatus = ACCOUNTSTATUS.Pending.ToString();
}

As the variable's context is the loop, a new instance will be created each time because we have moved the variable inside the local context (the for loop).

The recommendation sounds like a bug in Resharper's parsing of that code. However, in many cases this is a valid concern (such as the first example, where the reference is changing despite its capture in a closure).

My rule of thumb is, when in doubt make a local.

Here is a real world example I was bitten by:

        menu.MenuItems.Clear();
        HistoryItem[] crumbs = policyTree.Crumbs.GetCrumbs(nodeType);

        for (int i = crumbs.Length - 1; i > -1; i--) //Run through items backwards.
        {
            HistoryItem crumb = crumbs[i];
            NodeType type = nodeType; //Local to capture type.
            MenuItem menuItem = new MenuItem(crumb.MenuText);
            menuItem.Click += (s, e) => NavigateToRecord(crumb.ItemGuid, type);
            menu.MenuItems.Add(menuItem);
        }

Note that I capture the NodeType type local, note nodeType, and HistoryItem crumb.ItemGuid, not crumbs[i].ItemGuid. This ensures that my closure will not have references to items that will change.

Prior to using the locals, the events would trigger with the current values, not the captured values I expected.

---

Related Topics