Last week, a user reported a bug on NDepend (hopefully it happens in a very rare situation!). After mulling over a bit on the bug stack trace, it appears that the bug is the consequence of a call to ICollection<T>.Add() while the underlying collection is an array!
Here I felt bad, because really the bug is a consequence of a .NET Framework API design flaw. Or better said, to take our responsibility, the bug is the consequence of us, not providing a work around seriously enough on this .NET Framework API design flaw. Why the hell some read-only collection implements the Add() method? It is even getting worse if considering the documentation of Array.Add() (as a IList<T> explicit implementation method) and ReadOnlyCollection.ICollection<T>.Add().
This implementation always throws NotSupportedException. OUCH!
It appears that finally, a decade after .NET 1.0 has been release, 3 interfaces IReadOnlyCollection<T>, IReadOnlyList<T>, IReadOnlyDictionary<T> will be presented by the .NET v4.5. As commonly said, it is never too late to do things right, but in the case of an API with ascendant compatibility used by millions of developers world-wide, this proverb doesn’t fit that well.
The fact is that the brand new NDepend.API actually uses the interface ICollection<T> to return read-only lists. I wouldn’t like us or some users stumble on this API design flaw again! Hence a decision was taken to change the API policy, even though this will introduce breaking-change (the API is still young enough for that). First, I wrote the following CQLinq query to evaluate the problem:
let seqTypes = Types.Where(
t => t.Implement("System.Collections.Generic.IEnumerable<T>") &&
t.Name != "String" // We don't string as collection!
).ToHashSet()
from m in Assemblies.WithNameIn( "NDepend.API").ChildMethods()
where m.IsPubliclyVisible &&
m.ReturnType != null &&
seqTypes.Contains(m.ReturnType)
select new { m, m.ReturnType }
Here is the result, 40 methods (or property getters) returns something that implement IEnumerable<T> and many of them returns a ICollection<T>.
So what the API should return instead of ICollection<T>? Knowing that NDepend is still compatible with VS2008 and hence is compiled with .NET 3.5, it cannot use the brand new .NET v4.5 IReadOnlyList<T> (but it will certainly use these in several years, when we will compile against .NET v4.5 and above).
- Returning arrays instead of ICollection<T> could be an idea. The problem is that array is an implementation, and it is not read-only since the user can update each slot of the array.
- I never really liked the class ReadOnlyCollection<T> that wraps a IList<T>. First it is a class and not an interface. Second it has the property getter Items that can returns the wrapped IList<T>, so it is not that read-only and not a good wrapper at all! Third it implements IList<T>, and all mutable members are not supported, so it can lead to the same kind of confusion we are now faced with.
- Returning some IEnumerable<T> would be a better choice than the two above. It is not ideal through. While IEnumerable<T> conveys well the read-only idea, it has no Count nor an item access by index property. Hence it is not really suited to return a well-sized list. LINQ provides the adequate extension methods Count() and ElementAt(), but IEnumerable<T> has a strong taste of sequence, while we’d really like to provide users with proper lists!
So we choose to provide our own interfaces IReadOnlyCollection<T> and IReadOnlyList<T>! Same as the ones released soon in .NET v4.5, but defined in another namespace NDepend.Helpers, this will discard most naming collisions for clients. Thanks to this choice, the API is as clean as possible for now, and the transition will be simplified once NDepend will be compiled for .NET v4.5 and above, sooner or later.
As more and more often, I found inspiration in a stackoverflow.com response here. Find below the whole interfaces declarations and implementations. This is not an ideal solution, since List<T> and Array of course doesn’t implement our interfaces, but I found it to be a good design compromise.
Notice the four factory extension methods, that make a clear distinction at creation time, between Wrapped and Cloned read-only collection. Notice as well that the Cloned factories, take any sequence as argument, while the Wrapped factories, need collections and lists as argument:
- ToReadOnlyWrappedCollection(this ICollection<T>),
- ToReadOnlyClonedCollection(this IEnumerable<T>),
- ToReadOnlyWrappedList(this IList<T>),
- ToReadOnlyClonedList(this IEnumerable<T>),
Also, we’ve added the IndexOf() extension method over our two interfaces, because we needed it, and because the .NET Framework doesn’t provide IndexOf(this IEnumerable<T>) because a sequence is not necessarily ordered (like hashset or dictionary).
///<summary>
///Defines the property getter Count that defines a read-only collections.
///</summary>
///<remarks>A <see cref="IReadOnlyCollection{T}"/> object can be obtain through the extension methods <see cref="ExtensionMethodsEnumerable.ToReadOnlyWrappedCollection{T}"/> or <see cref="ExtensionMethodsEnumerable.ToReadOnlyClonedCollection{T}"/>.</remarks>
///<typeparam name="T">The type parameter of the items in the collection.</typeparam>
public interface IReadOnlyCollection<T> : IEnumerable<T> {
///<summary>
///Gets the number of elements contained in the collection.
///</summary>
int Count { get; }
}
///<summary>
///Represents a read-only collection of elements that can be accessed by index.
///</summary>
///<remarks>A <see cref="IReadOnlyList{T}"/> object can be obtain through the extension methods <see cref="ExtensionMethodsEnumerable.ToReadOnlyWrappedList{T}"/> or <see cref="ExtensionMethodsEnumerable.ToReadOnlyClonedList{T}"/>.</remarks>
///<typeparam name="T">The type parameter of the items in the collection.</typeparam>
public interface IReadOnlyList<T> : IReadOnlyCollection<T> {
///<summary>
///Gets the element at the specified index in the read-only list.
///</summary>
///<param name="index">The zero-based index of the element to get.</param>
T this[int index] { get; }
}
public static class ExtensionMethodsEnumerable {
/// <summary>
/// Creates a <see cref="IReadOnlyCollection{T}"/> wrapper collection around <paramref name="collection"/>.
/// </summary>
/// <typeparam name="T">The type parameter of the items in the collection.</typeparam>
/// <param name="collection">A collection object.</param>
public static IReadOnlyCollection<T> ToReadOnlyWrappedCollection<T>(this ICollection<T> collection) {
Contract.Requires(collection != null, "collection must not be null");
Contract.Ensures(Contract.Result<IReadOnlyCollection<T>>() != null, "returned read-only collection is not null");
Contract.Ensures(Contract.Result<IReadOnlyCollection<T>>().Count == collection.Count, "returned read-only collection has the same number of elements as collection");
return new CollectionReadOnlyWrapper<T>(collection);
}
/// <summary>
/// Creates a <see cref="IReadOnlyCollection{T}"/> cloned collection around <paramref name="collection"/>.
/// </summary>
/// <typeparam name="T">The type parameter of the items in the collection.</typeparam>
/// <param name="collection">A collection object.</param>
public static IReadOnlyCollection<T> ToReadOnlyClonedCollection<T>(this IEnumerable<T> collection) {
Contract.Requires(collection != null, "collection must not be null");
Contract.Ensures(Contract.Result<IReadOnlyCollection<T>>() != null, "returned read-only collection is not null");
Contract.Ensures(Contract.Result<IReadOnlyCollection<T>>().Count == collection.Count(), "returned read-only collection has the same number of elements as collection");
return new CollectionReadOnlyWrapper<T>(collection.ToArray());
}
/// <summary>
/// Creates a <see cref="IReadOnlyCollection{T}"/> wrapper collection around <paramref name="list"/>.
/// </summary>
/// <typeparam name="T">The type parameter of the items in the list.</typeparam>
/// <param name="list">A list object.</param>
public static IReadOnlyList<T> ToReadOnlyWrappedList<T>(this IList<T> list) {
Contract.Requires(list != null, "collection must not be null");
Contract.Ensures(Contract.Result<IReadOnlyList<T>>() != null, "returned read-only list is not null");
Contract.Ensures(Contract.Result<IReadOnlyList<T>>().Count == list.Count, "returned read-only list has the same number of elements as list");
return new ListReadOnlyWrapper<T>(list);
}
/// <summary>
/// Creates a <see cref="IReadOnlyCollection{T}"/> cloned collection around <paramref name="list"/>.
/// </summary>
/// <typeparam name="T">The type parameter of the items in the list.</typeparam>
/// <param name="list">A list object.</param>
public static IReadOnlyList<T> ToReadOnlyClonedList<T>(this IEnumerable<T> list) {
Contract.Requires(list != null, "collection must not be null");
Contract.Ensures(Contract.Result<IReadOnlyList<T>>() != null, "returned read-only list is not null");
Contract.Ensures(Contract.Result<IReadOnlyList<T>>().Count == list.Count(), "returned read-only list has the same number of elements as list");
return new ListReadOnlyWrapper<T>(list.ToArray());
}
private class CollectionReadOnlyWrapper<T> : IReadOnlyCollection<T> {
internal CollectionReadOnlyWrapper(ICollection<T> collection) {
Debug.Assert(collection != null);
m_Collection = collection;
}
readonly ICollection<T> m_Collection;
public int Count {
get { return m_Collection.Count; }
}
public IEnumerator<T> GetEnumerator() {
return m_Collection.GetEnumerator();
}
IEnumerator IEnumerable.GetEnumerator() {
return ((IEnumerable)m_Collection).GetEnumerator();
}
}
private class ListReadOnlyWrapper<T> : CollectionReadOnlyWrapper<T>, IReadOnlyList<T> {
internal ListReadOnlyWrapper(IList<T> list) : base(list) {
Debug.Assert(list != null);
m_List = list;
}
private readonly IList<T> m_List;
public T this[int index] {
get { return m_List[index]; }
}
}
///<summary>
///Determines the index of a specific item in <paramref name="readOnlyList"/>.
///</summary>
///<typeparam name="T">The type parameter of the items in the read-only list.</typeparam>
///<param name="readOnlyList"></param>
///<param name="value">The object to locate in <paramref name="readOnlyList"/>.</param>
///<returns>The index of value if found in the list; otherwise, -1.</returns>
///<remarks>This method uses the EqualityComparer$lt;T>.Default.Equals() method on <paramref name="value"/> to determine whether item exists</remarks>
public static int IndexOf<T>(this IReadOnlyList<T> readOnlyList, T value) {
Contract.Requires(readOnlyList != null, "readOnlyList must not be null");
var count = readOnlyList.Count;
var equalityComparer = EqualityComparer<T>.Default;
for(var i =0; i< count; i++) {
var current = readOnlyList[i];
if (equalityComparer.Equals(current, value)) { return i; }
}
return- 1;
}
}
[TestFixture]
public class Test_ReadOnlyCollectionList {
[Test]
public void Test_ReadOnlyCollection() {
var collection = new List<int> { 11, 21 };
var wrappedCollection = collection.ToReadOnlyWrappedCollection();
Assert.IsTrue(wrappedCollection.Count == 2);
Assert.IsTrue(wrappedCollection.ElementAt(0) == 11);
Assert.IsTrue(wrappedCollection.ElementAt(1) == 21);
var clonedCollection = collection.ToReadOnlyClonedCollection();
Assert.IsTrue(clonedCollection.Count == 2);
Assert.IsTrue(clonedCollection.ElementAt(0) == 11);
Assert.IsTrue(clonedCollection.ElementAt(1) == 21);
// Test Cloned/Wrapped
collection.Add(31);
Assert.IsTrue(wrappedCollection.Count == 3);
Assert.IsTrue(wrappedCollection.ElementAt(2) == 31);
Assert.IsTrue(clonedCollection.Count == 2);
// Force cover the IEnumerable.GetEnumerator()
IEnumerator enumerator = ((IEnumerable)wrappedCollection).GetEnumerator();
enumerator.MoveNext();
Assert.IsTrue((int)(enumerator.Current) == 11);
}
[Test]
public void Test_ReadOnlyList() {
var list = new List<int> { 11,21 };
var wrappedList = list.ToReadOnlyWrappedList();
Assert.IsTrue(wrappedList.Count == 2);
Assert.IsTrue(wrappedList[0] == 11);
Assert.IsTrue(wrappedList[1] == 21);
var clonedList = list.ToReadOnlyClonedList();
Assert.IsTrue(clonedList.Count == 2);
Assert.IsTrue(clonedList[0] == 11);
Assert.IsTrue(clonedList[1] == 21);
// Test Cloned/Wrapped
list.Add(31);
Assert.IsTrue(wrappedList.Count == 3);
Assert.IsTrue(wrappedList[2] == 31);
Assert.IsTrue(clonedList.Count == 2);
}
[Test]
public void Test_IndexOf() {
var ints = (new List<int> {11, 21, 31}).ToReadOnlyWrappedList();
Assert.IsTrue(ints.IndexOf(21) == 1);
Assert.IsTrue(ints.IndexOf(41) == -1);
var strings = (new List<string> {"11", "21", "31"}).ToReadOnlyWrappedList();
Assert.IsTrue(strings.IndexOf("21") == 1);
Assert.IsTrue(strings.IndexOf("41") == -1);
var obj = new object();
var objects = (new List<object> {new object(), obj, new object()}).ToReadOnlyWrappedList();
Assert.IsTrue(objects.IndexOf(obj) == 1);
Assert.IsTrue(objects.IndexOf(new object()) == -1);
}
}
And finally, I did a CQLinq rule to check that NDepend.API will never return anymore a ICollection<T>!
// <Name>Check enumerable types returned by NDepend.API</Name>
warnif count > 0
let seqTypes = Types.Where(
t => t.Implement("System.Collections.Generic.IEnumerable<T>") &&
// List of enumerable types that can be returned by a NDepend.API function.
!t.FullName.EqualsAny("System.String",
"System.Linq.ILookup<TKey,TElement>",
"NDepend.Helpers.IReadOnlyList<T>",
"NDepend.Helpers.IReadOnlyCollection<T>",
"System.Collections.Generic.HashSet<T>",
"NDepend.CodeModel.ICodeMetric<TCodeElement,TNumeric>")).ToHashSet()
from m in Assemblies.WithNameIn( "NDepend.API").ChildMethods() orderby m.NbLinesOfCode descending
where m.IsPubliclyVisible &&
m.ReturnType != null &&
seqTypes.Contains(m.ReturnType)
select new { m, m.ReturnType }
DIGITAL JUICE
No comments:
Post a Comment
Thank's!