tags:

views:

403

answers:

7

In the Design Guidelines for Developing Class Libraries, Microsoft say:

Do not assign instances of mutable types to read-only fields.

The objects created using a mutable type can be modified after they are created. For example, arrays and most collections are mutable types while Int32, Uri, and String are immutable types. For fields that hold a mutable reference type, the read-only modifier prevents the field value from being overwritten but does not protect the mutable type from modification.

This simply restates the behaviour of readonly without explaining why it's bad to use readonly. The implication appears to be that many people do not understand what "readonly" does and will wrongly expect readonly fields to be deeply immutable. In effect it advises using "readonly" as code documentation indicating deep immutability - despite the fact that the compiler has no way to enforce this - and disallows its use for its normal function: to ensure that the value of the field doesn't change after the object has been constructed.

I feel uneasy with this recommendation to use "readonly" to indicate something other than its normal meaning understood by the compiler. I feel that it encourages people to misunderstand the meaning of "readonly", and furthermore to expect it to mean something that the author of the code might not intend. I feel that it precludes using it in places it could be useful - e.g. to show that some relationship between two mutable objects remains unchanged for the lifetime of one of those objects. The notion of assuming that readers do not understand the meaning of "readonly" also appears to be in contradiction to other advice from Microsoft, such as FxCop's "Do not initialize unnecessarily" rule, which assumes readers of your code to be experts in the language and should know that (for example) bool fields are automatically initialised to false, and stops you from providing the redundancy that shows "yes, this has been consciously set to false; I didn't just forget to initialize it".

So, first and foremost, why do Microsoft advise against use of readonly for references to mutable types? I'd also be interested to know:

  • Do you follow this Design Guideline in all your code?
  • What do you expect when you see "readonly" in a piece of code you didn't write?
+20  A: 

It seems natural that if a field is readonly, you would expect to not be able to change the value or anything having to do with it. If I knew that Bar was a readonly field of Foo, I could obviously not say

Foo foo = new Foo();
foo.Bar = new Baz();

But I can get away with saying

foo.Bar.Name = "Blah";

If the object backing Bar is, in fact, mutable. Microsoft is simply recommending against that subtle, counterintuitive behavior by suggesting that readonly fields be backed by immutable objects.

Anthony Pegram
You say that seems natural, but it does not seem so to me. It might seem *desirable* that there be some means of specifying deep immutability, but it does not seem *natural* that readonly should be it, and even that does not obviously lead to the conclusion that we should behave *as if* that were readonly's actual meaning when it is not.
Weeble
A: 

If you put a mutable type into a readonly field, you have a fundamental contradiction. It is expected that the contents of a readonly field will be immutable. So, the following could lead to improper use:

public Customer OrderCustomer
{
   get;
}

There is no setter, yet the Customer object is mutable. If a developer interprets the readonly nature of the property to mean that the Customer object is immutable, that assumption could lead to problems. The code will compile and the system may run, but if a framework depends on mutability and other code expects immutability, you could have race conditions, deadlocks, and other resource contention issues.

Dave Swersky
If you know that the *symbol* `readonly` and the English language concept of "read only" don't actually mean the same thing, then there is no contradiction. Maybe `readonly` should be changed to `unassignable except in constructor` or something similar to prevent people from accidentally mapping the English language meaning onto the C# symbol.
dss539
+2  A: 

Microsoft has a few such peculiar advices. The other one that immediately springs to mind is not to nest generic types in public members, like List<List<int>>. I try to avoid these constructs where easily possible, but ignore the newbie-friendly advice when I feel the use is justified.

As for readonly fields - I try to avoid public fields as such, instead going for properties. I think there was a piece of advice about that too, but more importantly there are cases now and then when a field doesn't work while a property does (mostly it has to do with databinding and/or visual designers). By making all public fields properties I avoid any potential problems.

Vilx-
You raise a good point - do the design guidelines apply only to non-private members? The guidelines already say not to use public or protected fields. I suppose they are focused on the public API of a library, on which private fields obviously have no bearing. I certainly don't feel a pressing need to have *public* readonly mutable fields, since I rarely feel a need to have public fields at all.
Weeble
+1  A: 

In the end they are just guidelines. I know for a fact that the people at Microsoft often don't follow all of the guidelines.

tster
+7  A: 

I agree with you completely, and I do sometimes use readonly in my code for mutable reference types.

As an example: I might have some private or protected member -- say, a List<T> -- which I use within a class's methods in all its mutable glory (calling Add, Remove, etc.). I may simply want to put a safeguard in place to ensure that, no matter what, I am always dealing with the same object. This protects both me and other developers from doing something stupid: namely, assigning the member to a new object.

To me, this is often a preferable alternative to using a property with a private set method. Why? Because readonly means the value cannot be changed after instantiation, even by the base class.

In other words, if I had this:

protected List<T> InternalList { get; private set; }

Then I could still set InternalList = new List<T>(); at any arbitrary point in code in my base class. (This would require a very foolish error on my part, yes; but it would still be possible.)

On the other hand, this:

protected readonly List<T> _internalList;

Makes it unmistakably clear that _internalList can only ever refer to one particular object (the one to which _internalList is set in the constructor).

So I am on your side. The idea that one should refrain from using readonly on a mutable reference type is frustrating to me personally, as it basically presupposes a misunderstanding of the readonly keyword.

Dan Tao
+1, perfectly agree, private + readonly = less worries in code.
csharptest.net
+4  A: 
stakx
While I sort of understand your point, one thing I definitely *don't* wish for is the horrible C++-style `const`.
JSBangs
_@JS Bangs:_ Of course this comes down to personal preferences. C's type declaration syntax is indeed... well, "special", but offers a lot of flexibility. C's `const` would probably not fit well into the relative simplicity of C#/.NET's type system... yet I sometimes miss it. `const` is a more powerful tool than many people think.
stakx
Upvoted, but I do disagree on the one point that readonly should make referenced objects immutable. I can't see that working without a system even more complex than C++'s const: when you access an object through a readonly reference, does that make all its properties and fields return readonly references? What about its method return values? Do you need a way to explicitly declare methods const/readonly? How does it all work when the object is of a type declared in another assembly?
Weeble
+1  A: 

The syntax you are looking for is supported by the C++/CLI language:

const Example^ const obj;

The first const makes the referenced object immutable, the 2nd makes the reference immutable. The latter is equivalent to the readonly keyword in C#. Attempts to evade it produce a compile error:

Test^ t = gcnew Test();
t->obj = gcnew Example();   // Error C3892
t->obj->field = 42;         // Error C3892
Example^ another = t->obj;  // Error C2440
another->field = 42; 

It is however smoke and mirrors. The immutability is verified by the compiler, not by the CLR. Another managed language could modify both. Which is the root of the problem, the CLR just doesn't have support for it.

Hans Passant