Resharper Warning - Access to Modified Closure

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.

Access to modified closure: ReSharper

I would suggest avoid using a ref parameter for this in the first place - it seems needlessly complicated to me. I'd rewrite DoAction as:

static string DoAction(string data, Action<string> action)
{
    data = data == null ? "Initialized Data" : data + "|";
    action(data);
    return data;
}

Then you can have:

data = DoAction(data, Console.WriteLine);

or if you want to use a lambda expression:

data = DoAction(data, txt => Console.WriteLine(txt));

You can make DoAction a void method if you don't actually need the result afterwards. (It's not clear why you need the result to be returned and a delegate to execute in DoAction, but presumably that makes more sense in your wider context.)

How to mitigate Access to modified closure in cases where the delegate is called directly

Install the JetBrains.Annotations package with NuGet: https://www.nuget.org/packages/JetBrains.Annotations

Mark the passed in delegate with the InstantHandle attribute.

private void ConsumeConsume([InstantHandle] Consume c)
{
    c();
}

From InstantHandle's description:

Tells code analysis engine if the parameter is completely handled when the invoked method is on stack. If the parameter is a delegate, indicates that delegate is executed while the method is executed. If the parameter is an enumerable, indicates that it is enumerated while the method is executed.

Source: https://www.jetbrains.com/help/resharper/Reference__Code_Annotation_Attributes.html

If you don't want to add the whole package to your project, it's enough to just add the attribute yourself, although it's hacky in my opinion.

namespace JetBrains.Annotations
{
    [AttributeUsage(AttributeTargets.Parameter)]
    public class InstantHandleAttribute : Attribute { }
}

Access to Modified Closure

In this case, it's okay, since you are actually executing the delegate within the loop.

If you were saving the delegate and using it later, however, you'd find that all of the delegates would throw exceptions when trying to access files[i] - they're capturing the variable i rather than its value at the time of the delegates creation.

In short, it's something to be aware of as a potential trap, but in this case it doesn't hurt you.

See the bottom of this page for a more complex example where the results are counterintuitive.

Does ToList() remove need to address the Access to Modified Closure resharper warning?

That is exactly why "Access to modified closure" is just a warning, not an error. Basically, as long as that closure (any reference to it) cannot escape one iteration of the loop body, you are fine.

And since, as you mentioned, .ToList() evaluates the IEnumerable holding the closure, you're fine and this warning is, indeed, harmless and you can safely suppress it with a comment.

In order for ReSharper to know when this warning is serious, not only would it have to perform escape analysis on the closure, it would also have to know how both Find<T> and .ToList() behave with respect to keeping a hold of the closure. This will probably not happen anytime soon.

C# Access to modified closure

The problem is in:

e.Team.Where(partner => partner != member)

The variable member is a direct reference to the member variable in the outer scope. While you might not have a problem with this in the above code, it is problematic when you are running the code on multiple threads or if you aren't evaluating the lambda in the Where method right away (for example, using IQueryable instead of IEnumerable).

The reason this is a problem is that C# generates a method to then pass as a delegate to Where. That method needs direct access to member. If you were to assign the reference to another variable like this:

var m = member;
// ...
e.Team.Where(partner => partner != m);

Then C# can "capture" that value in a construct called a "closure" and pass it to the generated method. This will ensure that when member changes, the value you expect it to be when you pass it to Where isn't changed.

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>
}

Access to modified closure, is this a ReSharper bug?

Right, now you've modified the question, it makes complete sense. You're modifying a variable used within a closure - which can produce unexpected results:

var now = new DateTime(1970, 1, 1);
var dates = new List<DateTime>();
var query = dates.Where(d => d > now);
...
now = new DateTime(1990, 1, 1);
foreach (DateTime date in query)
{
    // This will only see dates after 1990, not after 1970
    // This would confuse many developers.
}

In fact, it's not just a matter of when the query starts - you could modify it while iterating over the results:

var now = new DateTime(1970, 1, 1);
var dates = new List<DateTime>();
var query = dates.Where(d => d > now);
...
foreach (DateTime date in query)
{
    now = date;
    Console.WriteLine(date);
}

That will give a strictly-increasing sequence of dates... again, somewhat confusing.

R# is absolutely right to warn about this, IMO. It can sometimes be useful - but should be used with great care.

---

Related Topics