Is Access to Modified Closure warnings valid for string variable?

Question

I have a warning with access to modified closure for string variable.

foreach (string s in splits)
{
    regexes.Where(x => x.Pattern.Contains(s));
}

Is it safe to leave the code the way it is? 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.

Thanks

Answer 1

In this case it doesn't hurt you because you aren't using the delegate at a place outside the loop. Of course, you aren't really doing anything useful in the example, so it is unclear if this "stands" for your actual code.

Threads and deferred callbacks would suffer though, so yes it is (in a way) valid. If unsure, fix it:

foreach (string s in splits)
{ 
    var clone = s;
    regexes.Where(x => x.Pattern.Contains(clone));
}

To clarify, this problem only hurts when the captured variable is used outside of the normal flow of the foreach loop. When used directly inside the loop, and on the same thread (no Parallel), the captured variable will always be at the expected value. Anything that breaks that will see random data; typically (but not always) the last value.

Note in particular that this "deferred delegate" includes things like building up Where filters on a query, as you'll find they all use the last value. The following would be bad:

var query = ...
foreach (string s in splits)
{ // BAD CODE!
    query = query.Where(x => x.Foo.Contains(s));
}

---

Re any "better way" (comments)... well, the cloned variable trick has the advantage of working, but here's a cute trick - fixes the above "BAD CODE!":

query = splits.Aggregate(query, (tmp, s) => tmp.Where(x => x.Foo.Contains(s)));

Personally I'd find the following easier to grok though:

foreach (string s in splits)
{
    var tmp = s;
    query = query.Where(x => x.Foo.Contains(tmp));
}

Answer 2

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.

Answer 3

Doesn't look safe, and I generally try to avoid warnings when possible since they often indicate potential problems.

Googling "access to modified closure" brings up a number of seemingly relevant hits, including as #2 Linq: Beware of the 'Access to modified closure' demon which you definitely want to look closely at.

Answer 4

Here is a quick example that proves that it's this is not safe for strings, immutable or not.

List<Func<int>> lambdas = new List<Func<int>>();
List<string> strings = new List<string> 
{ "first", "second", "supercalifragilisticexpialidocious" };

foreach (string s in strings)
{
  lambdas.Add(new Func<int>(() => s.Length));
}

Console.WriteLine(lambdas[0]()); //34
Console.WriteLine(lambdas[1]()); //34
Console.WriteLine(lambdas[2]()); //34

However, aside from getting bitten by LINQ's defered execution, I don't really think it's a problem to use lambdas in this way, since mostly the entire lifespan of the lambda is within the scope of the foreach.