Twisted Oak Studios

Unfathomable Bugs #5: Readonly or not

posted by Craig Gidney on November 30, 2012

Normally I post on Tuesdays, but I already have the next two posts written and this issue is too short and sweet to wait two and a half weeks. I’m actually quite surprised I haven’t stumbled on it before.

Today’s bug comes courtesy of Microsoft. Thank you, Microsoft, this series wouldn’t exist without the generous support of entities like you.

—

I was writing an extension method that would expose an IList<T> as an IReadOnlyList<T> when I came across the problem.

Because of historical coincidence, IReadOnlyList<T> is not implemented by IList<T>. As a result, some classes that implement IList<T> don’t implement IReadOnlyList<T>. A somewhat related problem is that many classes that implement IReadOnlyList<T> are readable but not readonly (example error case: you pass a read only list to some dynamic binding code, which sees that it’s actually a mutable list, and end up with two-way binding instead of one-way). The extension method I was writing was for working around these issues, and looked like this:

public static IReadOnlyList<T> AsReadOnlyList(IList<T> list) {
    if (list == null) throw new ArgumentNullException("list");
    if (list.IsReadOnly) {
        var r = list as IReadOnlyList<T>;
        if (r != null) return r;
    }
    return new ReadOnlyCollection<T>(list);
}

Notice the special case that doesn’t wrap the input if it is already a truly readonly list. To confirm that it was working properly, I tested some cases where wrapping should and shouldn’t occur. This resulted in a very confusing failure:

var r = new int[5];
Assert.AreNotSame(r, r.AsReadOnlyList()); // fails

Arrays aren’t being wrapped! This is despite the fact that Array.IsReadOnly is documented as guaranteed to return false (even for 0-length arrays where it would be fine if it returned true). This, you might reasonably expect, would seem to guarantee that a branch conditioned on the IsReadOnly property would not be taken. I even confirmed that the documentation was correct:

var r = new int[5];
Assert.IsFalse(r.IsReadOnly); // passes
Assert.AreNotSame(r, r.AsReadOnlyList()); // still fails. what.

What the heck is going on?? It took some very confused experimenting, but the issue is that Array.IsReadOnly is always false, until you cast to an IList<T>, in which case it is always true. Despite the fact that the array-as-IList<T> still allows you to make changes:

var r = new int[5];
Assert.IsFalse(r.IsReadOnly); // passes
IList<T> c = r;
c[0] += 1; // works. clearly not readonly
Assert.IsFalse(c.IsReadOnly); // fails

How is this possible? Well, the array type has an explicit implementation of ICollection<T>.IsReadOnly, distinct from the public Array.IsReadOnly getter, that returns true instead of false. That’s right, an on-purpose differing explicit implementation of the “same” method. This bug is coming from inside the house.

But seriously, this “bug” is by design. Apparently when Microsoft decided to not keep the IsFixedSize property when making a generic variant of IList, they also decided to subtly change the meaning of IsReadOnly. However, since changing the result of the existing method would be a breaking change, they just decided to… have a little bit of both meanings. Ugh.

I’m not the first to notice this issue. I probably won’t be the last, either. Keep it in mind, because to write actually correct code you have to check for arrays in addition to checking IsReadOnly:

public static bool IsReallyReallyReadOnly<T>(this IList<T> list) {
    return list.IsReadOnly && !(list is T[]);
}

—

Discuss on Reddit

—

Twisted Oak Studios offers consulting and development on high-tech interactive projects. Check out our portfolio, or Give us a shout if you have anything you think some really rad engineers should help you with.

Unfathomable Bugs #5: Readonly or not

Discuss on Reddit

Older Posts

More interesting posts (18 of 45 articles)