ReSharper Warning - Access to Modified Closure

Question

I have the following code:

string acctStatus = account.AccountStatus.ToString();
if (!SettableStatuses().Any(status => status == acctStatus))
    acctStatus = ACCOUNTSTATUS.Pending.ToString();

Note that account.AccountStatus is an enum of type ACCOUNTSTATUS. On the second line, ReSharper is giving me the warning "Access to Modified Closure" for acctStatus. When I do the recommended operation, Copy to local variable, it modifies the code to the following:

string acctStatus = realAccount.AccountStatus.ToString();
string s = acctStatus;
if (!SettableStatuses().Any(status => status == s))
    acctStatus = ACCOUNTSTATUS.Pending.ToString();

Why is this better or preferable to what I had originally?

EDIT

It also recommends Wrap local variable in array, which produces:

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

This seems downright wacky to me.

Solution

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.

Comments

[Ohad Schneider]

"The problem is that the closure will grab a reference to acctStatus, but each loop iteration will change that value" - actually it will always be account.AccountStatus[...].ToString() whenever the Any predicate evaluates it (since Any iterates over the enumerable before returning), so I'm not sure it's a good example. I believe what you proposed as better will have the same behavior.

[Godeke]

This would be incorrect. The reason for the difference is that in the first example only one variable is allocated: acctStatus. Resharper is concerned that this variable will change before Any() is actually evaluated. My suggestion doesn't change the action of the code at all (Any() is evaluated prior to any change happening) but makes explicit that we want a distinct instance of the variable acctStatus for each iteration of the loop. If you note my later example, the bug Resharper is worried about actually triggers because I don't immediately evaluate the lambda (minus the locals).