Extension methods for both IDictionary and IReadOnlyDictionary

Question

My question is similar to the previous one, but the answer to that question is inapplicable to this one.

Well, I want to write an extension method for both IDictionary and IReadOnlyDictionary interfaces:

public static TValue? GetNullable<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key)
    where TValue : struct
{
    return dictionary.ContainsKey(key)
        ? (TValue?)dictionary[key]
        : null;
}

public static TValue? GetNullable<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
    where TValue : struct
{
    return dictionary.ContainsKey(key)
        ? (TValue?)dictionary[key]
        : null;
}

But when I use it with classes implementing both interfaces (e.g. Dictionary<Tkey, TValue>), I get the ‘ambiguous invocation’. I don't want to type var value = myDic.GetNullable<IReadOnlyDictionary<MyKeyType, MyValueType>>(key), I want it to be just var value = myDic.GetNullable(key).

Is this possible?

Twirp

Why do you not keep only the IDictionary version? – Maxie

@mircea: unfortunately, IReadOnlyDictionary<TKey, TValue> doesn't inherit from IDictionary<TKey, TValue>. So if the OP wants to use the same extension method on both types of objects, he needs to write the method using a type that both objects inherit from, in this case, IEnumerable<KeyValuePair<TKey, TValue>>. – Trolley

Because some classes can implement IDictionry only. – Twirp

Answer 1

You only need one extension method. Redefine it as follows:

public static TValue? GetNullableKey<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> dictionary, TKey, key) where TValue : struct
{
    // Your code here.
    // Per @nmclean: to reap performance benefits of dictionary lookup, try to cast
    //               dictionary to the expected dictionary type, e.g. IDictionary<K, V> or
    //               IReadOnlyDictionary<K, V>. Thanks for that nmclean!
}

Both IDictionary<TKey, TValue> and IReadOnlyDictionary<TKey, TValue> inherit from IEnumerable<KeyValuePair<TKey, TValue>>.

HTH.

UPDATE: To answer your comment question

If you don't need to call methods that only appear on one interface or the other (i.e. you're only calling methods that exist on IEnumerable<KeyValuePair<TKey, TValue>>), then no, you don't need that code.

If you do need to call methods on either the IDictionary<TKey, TValue> or IReadOnlyDictionary<TKey, TValue> interface that don't exist on the common base interface, then yes, you'll need to see if your object is one or the other to know which methods are valid to be called.

UPDATE: You probably (most likely) shouldn't use this solution

So, technically, this solution is correct in answering the OP's question, as stated. However, this is really not a good idea, as others have commented below, and as I have acknowledged as correct those comments.

For one, the whole point of a dictionary/map is O(1) (constant time) lookup. By using the solution above, you've now turned an O(1) operation into an O(n) (linear time) operation. If you have 1,000,000 items in your dictionary, it takes up to 1 million times longer to look up the key value (if you're really unlucky). That could be a significant performance impact.

What about a small dictionary? Well, then the question becomes, do you really need a dictionary? Would you be better off with a list? Or perhaps an even better question: where do you start to notice the performance implications of using an O(n) over O(1) lookup and how often do you expect to be performing such a lookup?

Lastly, the OP's situation with an object that implements both IReadOnlyDictionary<TKey, TValue> and IDictionary<TKey, TValue> is, well, odd, as the behavior of IDictionary<TKey, TValue> is a superset of the behavior of IReadOnlyDictionary<TKey, TValue>. And the standard Dictionary<TKey, TValue> class already implements both of these interfaces. Therefore, if the OP's (I'm assuming, specialized) type had inherited from Dictionary<TKey, TValue> instead, then when read-only functionality was required, the type could've simply been cast to an IReadOnlyDictionary<TKey, TValue>, possibly avoiding this whole issue altogether. Even if the issue was not unavoidable (e.g. somewhere, a cast is made to one of the interfaces), it would still be better to have implemented two extension methods, one for each of the interfaces.

I think one more point of order is required before I finish this topic. Casting a Dictionary<TKey, TValue> to IReadOnlyDictionary<TKey, TValue> only ensures that whatever receives the casted value will not itself be able to mutate the underlying collection. That does not mean, however, that other references to the dictionary instance from which the casted reference was created won't mutate the underlying collection. This is one of the gripes behind the IReadOnly* collection interfaces—the collections referenced by those interfaces may not be truly "read-only" (or, as often intimated, immutable) collections, they merely prevent mutation of the collection by a particular reference (notwithstanding an actual instance of a concrete ReadOnly* class). This is one reason (among others) that the System.Collections.Immutable namespace of collection classes were created. The collections in this namespace represent truly immutable collections. "Mutations" of these collections result in an all-new collection being returned consisting of the mutated state, leaving the original collection unaffected.

Trolley

So should “your code here” look like if(dictionary is IReadonlyDictionary) ... else if(dictionary is IDictionary) ... else throw(new NotDictionaryException())? – Twirp

@Twirp You could definitely have code that specifically runs for IDictionary<K,V> and IReadOnlyDictionary<K,V>, but there's no reason not to implement something that works if it's just IEnumerable<KeyValuePair<K,V>> – Loper

@Twirp Like juharr says you could implement it solely based on IEnumerable<KeyValuePair>, but you would need to enumerate all the keys, defeating the performance benefit of dictionaries. It would be best to try to cast to IReadOnlyDictionary and IDictionary first so you can use their ContainsKey methods, then fall back to IEnumerable and Where if it didn't work. – Bloke

Why the down vote? This wasn't a question about performance (if that's what the down vote was for whoever down voted--and overall, the answer as it originally was was correct in and of itself. – Trolley

@Trolley Well, the downvote wasn't mine -- in fact I think it was already at 4 points before I commented about performance. – Bloke

No problem @nmclean, your comment, nonetheless, was on the mark! Thanks again! – Trolley

But upcasting shouldn’t be necessary to do something like this! – Gybe

@Gybe See @nmclean's comment above about the performance implications. The whole point of dictionaries is O(1) lookup. But, by converting to IEnumerable<KeyValuePair<TKey, TValue>>, we just made a key lookup an O(n) operation--very expensive compared to O(1). – Trolley

@Trolley But you shouldn’t have to use runtime casting. Shouldn’t the type system be helping you and not fighting you? Also, the idea of being able to fall back to a “table scan” if the upcast fails by enumerating the list of KeyValuePairs is sort of cool, but would require you to write code which accomplishes the same thing multiple times. If you are interested in seeing an alternative to upcasting, see my new answer where I adapt IDictionary<TKey, TValue> as an IReadOnlyDictionary<TKey, TValue>. – Gybe

@Gybe I see what you're getting at--with the three extension methods. Yes, that's correct, too. The OP wanted "one" method--but "one" isn't always the right answer ;). Thanks for sharing your answer!! – Trolley

I'm all for casting to IDictionary<TKey, TValue>/IReadOnlyDictionary<TKey, TValue>, but writing your own lookup code for IEnumerable<KeyValuePair<TKey, TValue>> can be tricky. Some implementations have additional lookup details, for example the System.Runtime.Caching.MemoryCache indexer checks for expired entries. In this specific case the enumerator also filters out expired entries, so it should work correctly. But there may be situations where a generic lookup cannot take such details into account. Whether that's a bug in that specific implementation or your code, is up for debate. – Winifredwinikka

@NielsvanderRest Yes, you are most certainly correct. I was merely answering the OP's question about "one method" to do this for the IDictionary<K,V> and IReadonlyDictionary<K,V> types. I've already commented that, while it's possible to do what the OP asked, it's not ideal. YMMV, depending on what you're trying to accomplish and your performance constraints. As my solution turns an O(1) operation into O(n)!! Not something you want to do lightly, otherwise, why even use a dictionary? Just use a list! – Trolley

Answer 2

After some experimenting, I remembered/found that C# is willing to resolve ambiguities by preferring subclasses over base classes. Thus, to do this with type safety, you need 3 extension methods rather than one. And if you have any other classes which, like Dictionary<TKey, TValue>, implement both IDictionary<TKey, TValue> and IReadOnlyDictionary<TKey, TValue>, you need to write more and more extension methods…

The following makes the compiler perfectly happy. It does not require casting to use the passed dictionary as a dictionary. My strategy is to implement my actual code for the lowest common denominator, IReadOnlyDictionary<TKey, TValue>, use a façade to adapt IDictionary<TKey, TValue> to that (because you could be passed an object which doesn’t implement IReadOnlyDictionary<TKey, TValue>), and, to avoid the method overload resolution ambiguity error, explicitly extend Dictionary<TKey, TValue> directly.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.IO;
using System.Linq;

// Proof that this makes the compiler happy.
class Program
{
    static void Main(string[] args)
    {
        // This is written to try to demonstrate an alternative,
        // type-clean way of handling https://mcmap.net/q/776655/-extension-methods-for-both-idictionary-and-ireadonlydictionary/429091
        var x = "blah".ToDictionary(
            c => c,
            c => (int)c);
        // This would be where the ambiguity error would be thrown,
        // but since we explicitly extend Dictionary<TKey, TValue> dirctly,
        // we can write this with no issue!
        x.WriteTable(Console.Out, "a");
        IDictionary<char, int> y = x;
        y.WriteTable(Console.Out, "b");
        IReadOnlyDictionary<char, int> z = x;
        z.WriteTable(Console.Out, "lah");
    }
}

// But getting compile-time type safety requires so much code duplication!
static class DictionaryExtensions
{
    // Actual implementation against lowest common denominator
    public static void WriteTable<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dict, TextWriter writer, IEnumerable<TKey> keys)
    {
        writer.WriteLine("-");
        foreach (var key in keys)
            // Access values by key lookup to prove that we’re interested
            // in the concept of an actual dictionary/map/lookup rather
            // than being content with iterating over everything.
            writer.WriteLine($"{key}:{dict[key]}");
    }

    // Use façade/adapter if provided IDictionary<TKey, TValue>
    public static void WriteTable<TKey, TValue>(this IDictionary<TKey, TValue> dict, TextWriter writer, IEnumerable<TKey> keys)
        => new ReadOnlyDictionary<TKey, TValue>(dict).StaticCast<IReadOnlyDictionary<TKey, TValue>>().WriteTable(writer, keys);

    // Use an interface cast (a static known-safe cast).
    public static void WriteTable<TKey, TValue>(this Dictionary<TKey, TValue> dict, TextWriter writer, IEnumerable<TKey> keys)
        => dict.StaticCast<IReadOnlyDictionary<TKey, TValue>>().WriteTable(writer, keys);

    // Require known compiletime-enforced static cast https://mcmap.net/q/456176/-how-to-do-a-static-cast-in-c/429091
    public static T StaticCast<T>(this T o) => o;
}

The most annoying thing with this approach is that you have to write so much boilerplate—two thin wrapper extension methods and one actual implementation. However, I think this can be justified because code using the extensions can be kept simple and clear. The ugliness is contained in the the extensions class. Also, because the extension implementation is contained in one method, you do not need to worry about updating the overloads when the IReadOnlyDictionary<TKey, TValue> variant is updated. The compiler will even remind you to update the overloads if you change the parameter types unless you’re adding new parameters with default values.

Gybe

I agree, the most annoying part is all the boilerplate code. Other than that, it should work as expected and can be used as one would expect to use such a method. You get a vote from me :) – Trolley

It needs also the creation of an Extra ReadOnlyDictionary object each time an IDictionary is used due to the façade – Weimer

@Weimer Yes. Hopefully it should be low impact and never leave the first generation heap. But .net has no way to avoid that while keeping code simple—even if you make a value type, it gets boxed the moment you cast it to an interface. Though actually you could you use generic constraints like this. Without that, you could do something like make an IDictionaryReader<TDictionary, TKey, TValue>. – Gybe

Answer 3

To get the desired behavior in your case, see fourpastmidnight's answer.

---

To directly answer your question though, no, it isn't possible to make the myDic.GetNullable syntax work when there are two extension methods. The error message you got was correct in saying it's ambiguous, because it has no way of knowing which one to call. If your IReadOnlyDictionary method happened to do something different, should it do that or not for Dictionary<TKey, TValue>?

It's for this reason that the AsEnumerable method exists. Extension methods are defined for both IEnumerable and IQueryable with the same names, so you sometimes need to cast objects that implement both to ensure that a specific extension method will be called.

Supposing your two methods did have different implementations, you could include similar methods like this:

public static IReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(this IDictionary<TKey, TValue> dictionary) {
    return (IReadOnlyDictionary<TKey, TValue>)dictionary;
}

public static IDictionary<TKey, TValue> AsReadWrite<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary) {
    return (IDictionary<TKey, TValue>)dictionary;
}

Then call the methods like this:

dictionary.AsReadOnly().GetNullable(key);
dictionary.AsReadWrite().GetNullable(key);

(Technically, a method named AsReadOnly should return a real ReadOnlyDictionary, but this is just for demonstration)

Bloke

That's not why the AsEnumerable and AsQueryable methods exist. IQueryable<T> extends IEnumerable<T>. It's not possible to have an IQueryable that's not IEnumearable. In that case it's not ambiguous, because of the inheritance chain. The more-derived type takes precedence, so an IQueryable object uses the IQueryable method. If you want to use the IEnumerable method, even though the object implements IQueryable, you need to use AsEnumerable. Another use is because instance methods have priority over extensions; if there is an instance method called Select... – Vadose

@Vadose Right, IQueryable implementors are always IEnumerable implementors as well, but that doesn't solve the issue. If you assign an IQueryable to a field that is declared as IEnumerable, you must cast it to call IQueryable extension methods. If these were classes, this would be solved with polymorphism, but with interface extension methods there is ambiguity that must be explicitly resolved by casting. – Bloke

I'm not saying there isn't a use for those methods, I'm saying the use you mentioned in this answer isn't a valid one. Also, the case you just mentioned in comments isn't really the designed use of those methods. They're built more for upcasting than downcasting. – Vadose

@Vadose Actually, you're right that AsQueryable isn't meant for downcasting. I assumed it was complementary to AsEnumerable, but it's actually meant for creating an IQueryable that calls IEnumerable extension methods. However, this is exactly the designed use of AsEnumerable, so I don't know why you're lumping it in. – Bloke

Note that either of those AsReadOnly or AsReadWrite will throw a runtime cast error when passed something that doesn’t implement both interfaces. Also, you’re forcing a runtime/dynamic cast. Why not just have both of those functions accept Dictionary<TKey, TValue> instead of the opposing interface? At least that can be verified to work at compiletime. – Gybe

Also, regarding your comment about ReadOnlyDictionary, the interface IReadOnlyDictionary<TKey, TValue> does not imply that the underlying dictionary is immutable. – Gybe

@Gybe 1) Actually I should have had them each accept their own return type, in the same way that AsEnumerable is designed... I'm not sure what I was thinking having it reversed. 2) I didn't mean to imply that IReadOnlyDictionary<TKey, TValue> must be read-only, it's just that per naming convention I think an AsReadOnly method should return an immutable view, like List<T>.AsReadOnly for instance. – Bloke