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?

Suisse

FxCop is used to detect possible problems with your code. There are often good reasons to ignore what it says. – Ethno

Yep, I'd just live with it since you're obviously modeling it after the ReadOnlyCollection and just add a [SuppressMessage(...)] attribute to keep it from barking in FxCop. – Fridlund

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.

Mv

The root cause is the bad design decision to not offer a IReadOnlyList<T> interface when the collection interfaces where created in .net 2. – Polygamous

Why not supply the "Add" method and upon invocation, have it throw an InvalidOperationException? That's similar to what read-only streams, etc. do. Also, you could implement a protected method with the same signature that calls ((IDictionary)this).Add(key, value); -- that will also make the warning go away. – Tat

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.

Taproom

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.

Dovetail