Runtime: KeyValuePair<TKey, TValue> should implement IEquatable<T>

Created on 29 Apr 2020  路  8Comments  路  Source: dotnet/runtime

The KeyValuePair class should implement IEquatable<T>.

Consider the sample below. It doesn't compile due to the constraint.

```c#
class Program
{
class Foo : IEquatable
{
public bool Equals(Foo other) => false;
}

static void Bar<T>(IEnumerable<T> items) where T : IEquatable<T>
{
    // Code that relies on IEquatable<T>
}

static void Main(string[] args)
{
    var dic = new Dictionary<string, Foo>();

    Bar(dic);        // CS0315
    Bar(dic.Values); // ok, but produces more garbage
}

}

In our performance critical application, reading the `Values` property is not an option. Now we faced the above problem, where our `IEquatable<T>` aware APIs cannot be used in this context.


**Current declaration:**
```c#
public readonly struct KeyValuePair<TKey, TValue>

Should be:
```c#
public readonly struct KeyValuePair : IEquatable>

Current **workaround** involves **allocating** an intermediate enumerator:

```c#
private static IEnumerable<TValue> GetItems<TKey, TValue>(Dictionary<TKey, TValue> dic)
    where TValue : IEquatable<TValue>
{
    foreach (var kvp in dic)
    {
        yield return kvp.Value;
    }
}
api-ready-for-review area-System.Collections

Most helpful comment

This is non pay-for-play API. It will make static footprint of all existing Dictionary instantiations more expensive. I do not think that this is a good API to add.

All 8 comments

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

What are you imagining the implementation being? EqualityComparer.Default on each of the members?

Also, it's not clear how your workaround is related to this. You would only be testing the values for equality there, not the full KeyValuePair. If you're only interested in enumerating values in the dictionary, you can already use Dictionary<TKey,TValue>.Values to get an IEnumerable<TValue>.

I could see it implementing all 3 - IEquatable<KeyValuePair<TKey, TValue>>, IEquatable<TKey>, and IEquatable<TValue>

Well, this is an error that should have been caught by your unit tests. All value types should implement IEquatable<T> as stated on MSDN.

You effectively have a well-used public type that a developer can unintentionally misuse, ending up with unnecessary boxing.

If you're only interested in enumerating values in the dictionary, you can already use Dictionary<TKey,TValue>.Values to get an IEnumerable<TValue>.

No. That's exactly what I'm trying to avoid here. Reading the Values property is very costly (at least as of .NET 4.7.2). Put it on a hot path and you are in GC trouble. So my workaround is highly relevant. Memory profiling a hot path in our code pointed at the Values collection, and using a regular foreach loop fixes that.

I agree with @john-h-k. For the strongly typed KeyValuePair<..> why not enforce strongly typed equability? As of 2020, it makes no sense to write a class without implementing this interface. Even more so for struct.

Given KVP is a readonly struct, I think this would be a reasonable addition. There is precedent in the ValueType implementations. Note that Object.Equals() and Object.GetHashCode() would need to be overridden explicitly as well.

This is non pay-for-play API. It will make static footprint of all existing Dictionary instantiations more expensive. I do not think that this is a good API to add.

Interesting. Care to elaborate? Which static footprint are you referring to?

The memory occupied by the runtime data structures that are backing the type.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

chunseoklee picture chunseoklee  路  3Comments

sahithreddyk picture sahithreddyk  路  3Comments

bencz picture bencz  路  3Comments

aggieben picture aggieben  路  3Comments

iCodeWebApps picture iCodeWebApps  路  3Comments