Is "Access to modified closure" resolved by comprehension syntax?

Question

ReSharper 6.0 gives me the "Access to modified closure" warning for the dr identifier in the first code snippet.

private IEnumerable<string> GetTheDataTableStrings(DataTable dt) {
    foreach (DataRow dr in dt.Rows) {
        yield return GetStringFuncOutput(() => dr.ToString());
    }
}

I think I have a basic understanding of what this warning is trying to protect me from: dr changes several times before GetTheDataTableStrings's output is interrogated, and so the caller might not get the output/behavior I expect.

But R# doesn't give me any warning for the second code snippet.

private IEnumerable<string> GetTheDataTableStrings(DataTable dt) {
    return from DataRow dr in dt.Rows select GetStringFuncOutput(dr.ToString);
}

Is it safe for me to discard this warning/concern when using the comprehension syntax?

Other code:

string GetStringFuncOutput(Func<string> stringFunc) {
    return stringFunc();
}

Palanquin

I've had to scrub/simplify this code before presenting it. Let me know if something about the code prevents you from discussing the question. – Palanquin

Answer 1

First off, you are correct to be concerned about the first version. Each delegate created by that lambda is closed over the same variable and therefore as that variable changes, the meaning of the query changes.

Second, FYI we are highly likely to fix this in the next version of C#; this is a major pain point for developers.

(UPDATE: This answer was written in 2011. We did in fact take the fix described below in C# 5.)

In the next version each time you run through the "foreach" loop we will generate a new loop variable rather than closing over the same variable every time. This is a "breaking" change but in the vast majority of cases the "break" will be fixing rather than causing bugs.

The "for" loop will not be changed.

See http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/ for details.

Third, there is no problem with the query comprehension version because there is no closed-over variable that is being modified. The query comprehension form is the same as if you'd said:

return dt.Rows.Select(dr=>GetStringFuncOutput(dr.ToString));

The lambda is not closed over any outer variable, so there is no variable to be modified accidentally.

Heurlin

Cool answer and good news about fixes in c#. What version do you expect to get it in -- C# 5 or 6? – Amalgamate

@the_joric: No C# 6 product has been announced. We expect to put this fix in C# 5. (We had to rejigger the closure rewriting code anyways to make async/await work, so figured might as well get this fixed at the same time.) – Heurlin

@EricLippert does the change require a change in the C# specification? – Sika

@phoog, yes, absolutely. It will change how foreach is implemented by the compiler. Looks like it's already updated. msdn.microsoft.com/en-us/library/aa664754(v=vs.71).aspx See also: blogs.msdn.com/b/ericlippert/archive/2009/11/12/… – Isogamy

@Isogamy Searching the C#4 spec for various forms of the word "close" and "closure", I was unable to find anything that describes the process of closing over loop variables. I don't see how the VS 2003 specification of foreach loops (your first link) is relevant. – Sika

@SolutionYogi: Unfortunately that documentation is wrong. You're pointing to the 2003 release of the documentation. Though it will magically become accurate when C# 5 ships, it is not accurate for the 2003 release; the loop variable should be declared outside. – Heurlin

Eric, that's what surprised me (I did not see VS 2003 note on the top). When I saw that the loop variable is declared inside, I thought the specification is already updated. And that is why I linked to your post where you discuss this potential change in foreach implementation. – Isogamy

@phoog, you will not see anything about closure in foreach specification but the implementation does affect how lambdas work. You should read that Eric's post which I linked to understand the reason behind the change. – Isogamy

@Isogamy I found the relevant text by searching for "captured" rather than "closure"; the section is headed "captured outer variables" – Sika

Answer 2

The issue that Resharper is warning about has been resolved in both C# 5.0 and VB.Net 11.0. The following are extracts from the language specifications. Note that the specifications can be found in the following paths by default on a machine with Visual Studio 2012 installed.

• C:\Program Files (x86)\Microsoft Visual Studio 11.0\VB\Specifications\1033\Visual Basic Language Specification.docx
• C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC#\Specifications\1033\CSharp Language Specification.docx

C# Language Specification Version 5.0

8.8.4 The foreach statement

The placement of v inside the while loop is important for how it is captured by any anonymous function occurring in the embedded-statement.

For example:

int[] values = { 7, 9, 13 };
Action f = null;
foreach (var value in values)
{
    if (f == null) f = () => Console.WriteLine("First value: " + value);
}
f();

If v was declared outside of the while loop, it would be shared among all iterations, and its value after the for loop would be the final value, 13, which is what the invocation of f would print. Instead, because each iteration has its own variable v, the one captured by f in the first iteration will continue to hold the value 7, which is what will be printed. (Note: earlier versions of C# declared v outside of the while loop.)

The Microsoft Visual Basic Language Specification Version 11.0

10.9.3 For Each...Next Statements (Annotation)

There is a slight change in behavior between version 10.0 and 11.0 of the language. Prior to 11.0, a fresh iteration variable was not created for each iteration of the loop. This difference is observable only if the iteration variable is captured by a lambda or a LINQ expression which is then invoked after the loop.

Dim lambdas As New List(Of Action)
For Each x In {1,2,3}
   lambdas.Add(Sub() Console.WriteLine(x)
Next
lambdas(0).Invoke()
lambdas(1).Invoke()
lambdas(2).Invoke()

Up to Visual Basic 10.0, this produced a warning at compile-time and printed "3" three times. That was because there was only a single variable "x" shared by all iterations of the loop, and all three lambdas captured the same "x", and by the time the lambdas were executed it then held the number 3. As of Visual Basic 11.0, it prints "1, 2, 3". That is because each lambda captures a different variable "x".

Petr