Recently I have introduced a subtle bug into my code. It all started when I was creating a few value object classes:
MethodCall class constructor, I wrongly assumed
IReadOnlyList<T> behaves like an immutable list.
In other words that its content never changes.
Due to this wrong assumption I did not create a defensive copy,
that I usually do for collection arguments,
but instead I just assigned
args parameter to
a readonly property named
MethodCall object was then used by another component
TypeArguments passed to it via
Then when someone calls
AddMethodCall method, it uses stored
TypeArguments and a value of
method parameter to construct
MethodCall object and adds it to
MethodCallSpy class worked perfectly, at least until I
returned to it a few days later to make some improvements.
Yes, I know, I know
“premature optimization is the root of all evil”
but this was my quick’n’dirty pet project and I just cannot resist:
Of course I also made a lot of other refactorings without running my tests (I had only a few integration tests). This was another mistake of mine. Looks like good practices help even if you are building quickly a Proof Of Concept solution.
When I finally ran my tests, they all failed. For some reason
MethodCall objects did not contain any
Strange, isn’t it…
After a quarter of debugging, I have found that the bug was
introduced by my wrong assumptions about
IReadOnlyCollection<T> interfaces and
ReadOnlyCollection<T> class where introduced
to protect owners of the collections, not the receivers.
For example if a method is declared like this:
We can be sure, that it will not attempt to modify the list that we are going to pass to it:
ProcessList method may be
implemented in an evil way:
But we can protect our lists from evil code by using
method, that returns a
wrapped in a
On the other hand receivers of
are not protected at all. Consider this short program:
It demonstrates that
IReadOnlyList<T> can change even
during duration of a single method!
So what should we do to void this class of bugs? Option one is to use old and proven defensive copy technique:
Remember not to use
ReadOnlyCollection<T> class to create
a defensive copy. This class is only a wrapper - it
does not copy the actual data.
The second option is to use truly immutable data structures, for example from System.Collections.Immutable package:
These types are truly immutable, they never change their contents after they are created.
One problem that I have with
is that immutable types have often different performance
characteristics than their mutable counterparts.
ImmutableList is implemented using,
guess what, a good old AVL tree
On the other hand
ImmutableArray that is backed by a
regular array, seems like a perfect candidate for making
defensive copies. You can read more about
in this MSDN article.
It is never wrong to take a look at the collection source code before using it (all links to GitHub):