Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Synchronize collections: ReSharper "Implicitly captured closure"

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);
    }
}
like image 244
Matteo Migliore Avatar asked Dec 06 '22 07:12

Matteo Migliore


1 Answers

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.

like image 56
Eric Lippert Avatar answered Dec 09 '22 14:12

Eric Lippert