[Bug]: `ReferenceEquals` breaks value types

[erri120]

Describe the bug 🐞

DynamicData has many instances of using ReferenceEquals which breaks observable streams silently for value types.

The issue I ran into specifically was using MergeManyChangeSets for IChangeSet<TValue, TKey> where both the value and key are value types. When an item upstream is getting removed, the item was never removed from the cache because the equality check uses ReferenceEquals:

DynamicData/src/DynamicData/Cache/Internal/ChangeSetMergeTracker.cs

Lines 270 to 271 in afbbb47

	private bool CheckEquality(TObject left, TObject right) =>
	ReferenceEquals(left, right) || (equalityComparer?.Equals(left, right) ?? false);

The temporary workaround is to add a custom equality comparer, but a proper solution should use EqualityComparer<T>.Default or the custom equality comparer instead of ReferenceEquals.

Step to reproduce

var source1 = new SourceCache<int, int>(x => x);
var source2 = new SourceCache<bool, int>(_ => throw new NotSupportedException());

source1.Connect().MergeManyChangeSets(_ => source2.Connect()).Do(changeSet =>
{
    foreach (var change in changeSet)
    {
        Console.WriteLine($"{change.Reason}: {change.Current}");
    }
}).Subscribe();

source1.Edit(updater => updater.AddOrUpdate(1, key: 1));

// produces console log
source2.Edit(updater => updater.AddOrUpdate(false, key: 1));

// doesn't produce console log
source2.Edit(updater => updater.Remove(key: 1));

Expected behavior

Value types shouldn't break DynamicData silently or loudly with exceptions, as seen in #925.

DynamicData Version

9.0.4

[JakenVeina]

I would agree. I can't imagine a scenario where EqualityComparer<T>.Default isn't the proper choice, but I also have never had a use-case for this operator, so I'll defer to @dwcullop for a second-opinion. He's the one who actually wrote this, and has use-cases for it all the time.

[dwcullop]

Clearly a bug. Will fix and also check for other uses of ReferenceEquals.

[dwcullop]

I would agree. I can't imagine a scenario where EqualityComparer<T>.Default isn't the proper choice

There is a scenario where we need ReferenceEquals. When two merged changesets each contain an entry with the same key, only one will be emitted downstream. If either of the source changesets emits "Refresh" for that key, we have to re-evaluate to see if we're still emitting the best choice. If the best choice is now different, then downstream should observe an "Update" to the new, better value. However, if the current one is still the best choice, then the downstream should observe the "Refresh" event IF AND ONLY IF it came from the item that is being observed downstream (the best choice). If it came from the other one, then the downstream shouldn't see anything because they shouldn't see a Refresh for an entry that they haven't actually seen. So "ReferenceEquals" is being used to make sure the updated value is the same as the downstream value.

Not sure how to make this work in the context of a value type.

[erri120]

There is a scenario where we need ReferenceEquals. When two merged changesets each contain an entry with the same key, only one will be emitted downstream. If either of the source changesets emits "Refresh" for that key, we have to re-evaluate to see if we're still emitting the best choice. If the best choice is now different, then downstream should observe an "Update" to the new, better value. However, if the current one is still the best choice, then the downstream should observe the "Refresh" event IF AND ONLY IF it came from the item that is being observed downstream (the best choice). If it came from the other one, then the downstream shouldn't see anything because they shouldn't see a Refresh for an entry that they haven't actually seen. So "ReferenceEquals" is being used to make sure the updated value is the same as the downstream value.

Not sure how to make this work in the context of a value type.

I'm not sure why this requires ReferenceEquals. The definition of equality should be consistent across all operations in a stream. If all other operators use the equality operator, it should also be used here to guarantee consistent behavior.

[JakenVeina]

Not sure how to make this work in the context of a value type.

Pretty sure EqualityComparer<T>.Default covers this, as it just uses reference-equality for reference types that don't implement IEquatable<T>.

If the best choice is now different, then downstream should observe an "Update" to the new, better value.

It all hinges on your definition of "different" here.

If the type is a value type, "different" should mean value-inequality. EqualityComparer<T>.Default gives you that.

If the type is a reference type not implementing IEquatable<T>, "different" should mean reference-inequality, which is the scenario you're describing. EqualityComparer<T>.Default gives you that.

If the type is a reference type implementing IEquatable<T>, "different" should mean IEquatable<T>.Equals(). EqualityComparer<T>.Default gives you that.

Honestly, I'd say if these operators actually need an additional enhancement. Any operator that functionally relies on equality checking, beyond just key-equality, should really have an option/parameter for injecting a custom IEqualityComparer<T>.

I also see an ENTIRELY different possible approach here, which doesn't require equality checking at all. Your approach for merging these conflicting Refresh/Update changes was to try and determine which one is optimal to forward, which requires equality checking. The alternative approach is to just NOT bother being optimal, and always forward the Update. An Update change won't actually BREAK anything if the old and new items are the same, it's just sub-optimal. Maybe it's worth the simplicity to not have to deal with equality checking.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.