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?

FxCop is used to detect *possible* problems with your code. There are often good reasons to ignore what it says. — dlev, Jul 11 '11 at 20:59
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. — James Michael Hare, Jul 11 '11 at 21:12

Stephen Cleary · Accepted Answer · 2011-07-11T21:08:24.190

5

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.

edited Jul 11 '11 at 21:08

answered Jul 11 '11 at 21:01

Stephen Cleary

437,863
77
675
810

2

The root cause is the bad design decision to not offer a `IReadOnlyList` interface when the collection interfaces where created in .net 2. – CodesInChaos Jul 11 '11 at 21:26
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. – BrainSlugs83 Sep 27 '13 at 02:31

score 1 · Answer 2 · answered Jul 11 '11 at 21:06

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.

score 1 · Answer 3 · answered Jul 12 '11 at 12:09

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.

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

3 Answers3