FxCop: CA1033 - Microsoft's implementation of a ReadOnlyCollection violates this?

Question

If you look at the code for a read-only collection it does not have an "Add" method, but instead defines the ICollection<T>.Add(T Value) method (explicit interface implementation).

When I did something similar with my ReadOnlyDictionary class, FxCop 10 complains that I'm breaking CA1033.

public class ReadOnlyDictionary<TKey, TValue> : IDictionary<TKey, TValue>
{
    //CA1033 ERROR
    void IDictionary<TKey, TValue>.Add(TKey, TValue) { //Throw Exception }
}

public class ReadOnlyDictionary<TKey, TValue> : IDictionary<TKey, TValue>
{
    //NO CA1033 ERROR
    Add(TKey, TValue) { //Throw Exception }
}

ReadOnlyCollectionClass:

public class ReadOnlyCollection<T> : ICollection<T>
{
    void ICollection<T>.Add(T item) { //Throw Exception }
}

So, is this a false positive? Is Microsoft's base code bad? What gives?

Answer 1

A lot of Microsoft code "fails" FxCop and StyleCop. The primary reason is that these tools are new; a lot of the BCL was written by many programmers before anyone had any experience with .NET at all.

I'd say in this particular case it's a false positive. But it depends on what you mean by "false". I think the run-time nature of the collection interface is hokey at best. It could be argued that read-only collections are in violation of the LSP. But the explicit implementation acts as a "hint" that your class is not really a (full) collection.

Answer 2

That is a false positive here. In a read-only collection an Add is never going to be useful.

It is only needed to satisfy the interface, since there is no valid/inbuilt read-only IList[<T>] equivalent that provides read access by index.

In this case, tucking it out of sight makes perfect sense.

Answer 3

I'm in agreement with Marc on this one -- it's useless, so it might as well be hidden as much as possible. It's also worth considering that the ReadOnlyCollection might have a similar suppression for exactly the same reason. Unfortunately, given that that the release build of mscorlib does not contain SuppressMessage attributes, we have no easy way of knowing if that might be the case or not.

There are a few generic ReadOnlyDictionary implementations in the .NET Framework, although none of them are public in existing versions. At least one of them (System.Dynamic.Utils.ReadOnlyDictionary) uses explicit interface implementations for the "modification" methods.