Search code examples
c#warningsresharper-6.0list-comprehension

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


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();
}

Solution

  • 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.