C# CA2104 - Automated Code Analysis dislikes static readonly mutable types

Question

I've got a code like this:

public abstract class Base
{
    // is going to be used in deriving classes
    // let's assume foo is threadsafe
    protected static readonly Foo StaticFoo = new Foo();
}

Visual Studio 2008's Code Analysis pops up this message:

CA2104 : Microsoft.Security : Remove the read-only designation from 'Base.StaticFoo' or change the field to one that is an immutable reference type. If the reference type 'Foo' is, in fact, immutable, exclude this message.

Is my design instrinsically flawed, or can i add a [SuppressMessage] in the source?

Answer 1

The warning is telling you, that the readonly modifier only applies to the reference itself. If Foo is a reference type the state of your Foo instance may still be modified unless you make sure Foo is immutable.

On another node, statics like that may end up giving you all sorts of problems when it comes to unit testing the code, so if that is important to you, you may want to look at other ways to achieve what you're trying to do.

Answer 2

Is the Foo type immutable?

If not, is your intention to use only this one Foo object, but to be able to alter its properties? If this is the case, then the warning is simply saying that the readonly keyword is misleading. There is no compilation error - the reference to the object is readonly, and that is what you have declared to the compiler. However, what you have declared to other developers is that StaticFoo is readonly which implies that it will never change.

So, you have a choice, as it says. To eliminate this warning, either remove the readonly keywoard, or add a SuppressMessage attribute. Alternatively, look at the design of your code. Would implementing Foo as a singleton pattern be more appropriate for example?

Answer 3

The problem is that it gives the wrong impression - it makes it look like the value can't change, when actually it's only the field value that can't change, rather than the object. I think the code analysis is being overly cautious here - there are plenty of places where you might want a mutable type stored in a readonly field, particularly if it's private, initialized in a static initializer and then never mutated within your code.

While the field is protected, however, there's more of a risk of derived classes abusing it. Personally I like all my fields to be private. On the other hand, I don't know your code - it could be that you've weighed that up against the possibilities and decided it's the right thing to do.

If Foo really is thread-safe, there's less to worry about, other than the general impression of immutability given by the fact that the field is readonly. I'd disable the warning for that line, and add a comment to emphasize that the object is mutable even though the field is readonly.