ReadOnly does not mean Immutable

Recently I have introduced a subtle bug into my code. It all started when I was creating a few value object classes:

public class Method {
    public string Name { get; }
    public Method(string name) {
        Name = name;
    }
    // Equals, GetHashCode, ToString skipped
    // to save space.
}
public class TypedArgument {
    public Type Type { get; }
    public object Value { get; }
    public TypedArgument(Type type, object value) {
        Type = type;
        Value = value;
    }
}
public class MethodCall {
    public Method CalledMethod { get; }
    public IReadOnlyList<TypedArgument> PassedArguments { get; }
    public MethodCall(
        Method calledMethod,
        IReadOnlyList<TypedArgument> args)
    {
        CalledMethod = calledMethod;
        // NOTICE: No defensive copy.
        PassedArguments = args;
    }
}

While implementing MethodCall class constructor, I wrongly assumed that 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. Instead I just assigned args parameter to a readonly property named PassedArguments.

MethodCall object was then used by another component called MethodCallSpy:

public class MethodCallSpy {
    private readonly List<MethodCall> _methodCalls
        = new List<MethodCall>();

    public IReadOnlyList<MethodCall> MethodCalls
        => _methodCalls;

    private List<TypedArgument> _currentArguments
        = new List<TypedArgument>();

    public void AddArgument(TypedArgument arg) {
        _currentArguments.Add(arg);
    }

    public void AddMethodCall(Method method) {
        var methodCall = 
            new MethodCall(method, _currentArguments);
        _methodCalls.Add(methodCall);

        // NOTICE: Old List<T> is not modified.
        _currentArguments = new List<TypedArgument>();
    }
}

MethodCallSpy gathers TypedArguments passed to it via AddArgument calls in _currentArguments list. Then when someone calls AddMethodCall method, it uses stored TypedArguments and a value of method parameter to construct a new MethodCall object and adds it to _methodCalls list.

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:

// readonly added.
private readonly List<TypedArgument> _currentArguments
    = new List<TypedArgument>();
public void AddMethodCall_AfterRefactoring(Method method) {
    // Assumes that MethodCall will make a defensive copy
    // of _currentArguments list.
    var methodCall = 
        new MethodCall(method, _currentArguments);

    _methodCalls.Add(methodCall);

    // NOTICE: We use clear instead of creating a new list
    _currentArguments.Clear();
}

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 programming practices help even if you are building quickly a Proof Of Concept solution.

When I finally ran my tests, they all have failed. For some reason MethodCall objects did not contain any TypedArguments. Strange, isn’t it…

After a quarter of debugging, I have found that the bug was introduced by my wrong assumptions about IReadOnlyList<T> interface.

IReadOnlyList<T>, 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:

void ProcessList<T>(IReadOnlyList<T> list);

We can be sure, that it will not attempt to modify the list that we are going to pass to it:

var list = new List<string> { "foo", "bar" };
ProcessList(list);
// still: list == new List<string> { "foo", "bar" }

Of course ProcessList method may be implemented in an evil way:

private void ProcessList<T>(IReadOnlyList<T> list) {
    (list as IList<T>)[0] = default(T);
}

But we can protect our lists from evil code by using AsReadOnly method, that returns a List<T> instance wrapped in a ReadOnlyCollection<T> object (source code).

var list = new List<string> { "foo", "bar" };
ProcessList(list.AsReadOnly());
// Our list can be modified now
// only by using reflection
// to get access to private fields
// of ReadOnlyCollection<T> class.

On the other hand receivers of IReadOnlyList<T> arguments are not protected at all. Consider this short program:

[Fact]
public void ReadonlyButNotForYou() {
    var list = new List<int> { 1, 2, 3 };

    int extraInt() {
        list[0] = 42;
        return list[0];
    }

    ProcessList(list, extraInt);
}
private void ProcessList(IReadOnlyList<int> ints, Func<int> extraInt) {
    int first = ints.First();
    int tmp = extraInt();
    int firstAgain = ints.First();

    Assert.Equal(first, firstAgain); // fails
}

It demonstrates that IReadOnlyList<T> can change even during duration of a single method!

So what should we do to avoid this class of bugs? Option one is to use old and proven defensive copy technique:

public MethodCall(
    Method calledMethod, 
    IReadOnlyList<TypedArgument> args)
{
    CalledMethod = calledMethod;
    PassedArguments = 
        new List<TypedArgument>(args);
}

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:

public MethodCall(
    Method calledMethod, 
    IImmutableList<TypedArgument> args)
{
    CalledMethod = calledMethod;
    PassedArguments = args;
}

These types are truly immutable, they never change their contents after they are created.

One problem that I have with System.Collections.Immutable is that immutable types have often different performance characteristics than their mutable counterparts. For example ImmutableList is implemented using, guess what, a good old AVL tree (source code).

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 ImmutableArray in this MSDN article.

It is never wrong to take a look at the collection source code before using it (all links to GitHub):

Menu