Synchronize collections: ReSharper "Implicitly captured closure"

Question

I implemented this extension method to synchronize a collection from another that can be of different type:

public static void Synchronize<TFirst, TSecond>(
    this ICollection<TFirst> first,
    IEnumerable<TSecond> second,
    Func<TFirst, TSecond, bool> match,
    Func<TSecond, TFirst> create)
{
    var secondCollection = second.ToArray();

    var toAdd = secondCollection.Where(item => !first.Any(x => match(x, item))).Select(create);

    foreach (var item in toAdd)
    {
        first.Add(item);
    }

    var toRemove = first.Where(item => !secondCollection.Any(x => match(item, x))).ToArray();

    foreach (var item in toRemove)
    {
        first.Remove(item);
    }
}

ReSharper give me two "Implicitly captured closure", one on the first Where and one on the second, is there a way to fix it? I cannot find one.

[Update]

Based on the observation of Eric, I wrote a version that is faster than the one with the equals function, using instead the hash:

public static void Synchronize<TFirst, TSecond>(
    this ICollection<TFirst> first,
    IEnumerable<TSecond> second,
    Func<TSecond, TFirst> convert,
    Func<TFirst, int> firstHash = null,
    Func<TSecond, int> secondHash = null)
{
    if (firstHash == null)
    {
        firstHash = x => x.GetHashCode();
    }

    if (secondHash == null)
    {
        secondHash = x => x.GetHashCode();
    }

    var firstCollection = first.ToDictionary(x => firstHash(x), x => x);
    var secondCollection = second.ToDictionary(x => secondHash(x), x => x);

    var toAdd = secondCollection.Where(item => firstCollection.All(x => x.Key != item.Key)).Select(x => convert(x.Value));

    foreach (var item in toAdd)
    {
        first.Add(item);
    }

    var toRemove = firstCollection.Where(item => secondCollection.All(x => x.Key != item.Key));

    foreach (var item in toRemove)
    {
        first.Remove(item.Value);
    }
}

I wrote an answer some time ago: [Read more here][1] [1]: http://stackoverflow.com/questions/13633617/why-does-resharper-tell-me-implicitly-captured-closure/15843306#15843306 — quadroid, Jun 18 '14 at 20:58

score 16 · Accepted Answer · answered Jun 13 '13 at 19:29

First let me characterize the problem that Resharper is attempting to alert you to here. Suppose you have:

Action M(Expensive expensive, Cheap cheap)
{
    Action shortLived = ()=>{DoSomething(expensive);};
    DoSomethingElse(shortLived);
    Action longLived = ()=>{DoAnotherThing(cheap);};
    return longLived;
}

The issue here is that in C# (and VB, and JScript and many other languages), the lifetime of both cheap and expensive is extended to match the lifetime of both shortLived and longLived. So what happens is even though longLived does not use expensive, the expensive resource is never garbage collected until longLived dies.

Your program matches this pattern; you make two delegates out of two lambdas; one of them uses first and the other does not. Therefore first will survive as long as the longer of the two delegates.

Second let me criticize Resharper here. This is clearly a false positive. In order for this to be a true positive one of the delegates has to be long lived. In this case both delegates will be eligible for collection when the method returns; both are short lived and therefore there is no actual bug here. Resharper could track the query objects returned by Where and notice that they do not survive the method.

Third, what should you do about it? I would be inclined to do nothing about it; if the code is working as you like it then keep it working. I would not be concerned about the Resharper warning; I would be far, far more concerned about the fact that your code is extremely inefficient for large collections. If the two collections have n and m items then this program does O(nm) operations.

So Eric, in this case ReSharper "cannot" know if the warning is a false positive and I can ignore it. Have you a more efficient alternative to my implementation? — Matteo Migliore, Jun 13 '13 at 22:08
@MatteoMigliore: The expensive part of your implementation is that you're running a matching predicate against all pairs. If you can supply a *hashing* function that is *consistent* with the matching predicate -- that is, two matching elements have the same hash code -- and has *good distribution* of hash values -- then you can build a hash table that is O(1), which will reduce your cost from O(nm) to O(n+m). — Eric Lippert, Jun 13 '13 at 22:34

score -1 · Answer 2 · answered Jun 13 '13 at 16:50

-1

Secondcollection variable is used in both the places which is the actual cause. Make the first expression to use variable 'second' instead of secondcollection. Problem is better explained in http://sparethought.wordpress.com/2012/08/17/implicit-closures-in-c-lambdas/

answered Jun 13 '13 at 16:50

srsyogesh

609
5
17

No, because I even obtain the warning but on the "first" variable and I also obtain the "possible multiple enumeration" on the "second" variable. – Matteo Migliore Jun 13 '13 at 17:51

Synchronize collections: ReSharper "Implicitly captured closure"

2 Answers2