Should all .NET Collections Implement all .NET Collection Interfaces? That was the first of several important API questions asked in the .NET Core API Review for January the 14th. This video contains a recording of the discussions about ten change requests for the .NET’s base class library.
[Video] GitHub Issue: #316: Implement IList<T>, IReadOnlyList<T>, and IList on Regex Collections
In .NET, most properties and methods that return a collection use a strongly typed collection. Not just IEnumerable<Foo> or IList<Foo>, but an actual FooCollection. This is better for backwards compatibility because new methods can be added to the class in a way that doesn’t rely on unsafe casting.
But that’s not the only reason this was done. In .NET 1.0, generics didn’t exist yet. This meant that we didn’t have interfaces such as IList<T> and type safety pretty much required creating custom classes.
When .NET 2.0 was released, many of these strongly typed classes were never upgraded to support the new generic collection interfaces. So the first question on the table is should Microsoft go ahead and make the change for the RegEx Collections.
The second question is whether or not to support interfaces such as IList<T> for read-only collections. Had read-only collection interfaces been available since .NET 2.0 this would be a no-brainer and IList<T> would only be used for mutable collections. But since they are fairly new, many APIs expect a IList<T> and just document that they won’t modify the collection. The ones that do modify the collection are supposed to check the IsReadOnly property, but it is up to both the library developer and the application developer to remember this check.
So back to the question. Should the read-only RegEx collections implement IList<T> so that they can be used in older APIs that don’t expect IReadOnlyList<T>? Or should they skip the IList<T> interface and reduce the risk that they would accidentally be used where a mutable list is expected.
Another contributing factor is that data binding in Windows Forms is based on IList, so adding that interface is necessary for using these collections in the UI.
Conclusion: Follow today’s guidelines and implement all of the interfaces. Long-term, consider changing the guidelines.
Another question raised was whether or not to just change the classes to inherit from ReadOnlyCollection<T>. While this would remove a lot of boiler plate code, there are some backwards compatibility issues:
- Overload resolution would change (this is also a problem when adding new interfaces)
- The results of using Reflection would change.
Conclusion: In the future always derive new read-only collection classes from ReadOnlyCollection<T>. For existing classes, leave unchanged.
Serialization is another issue. Because of old bugs in the serialization libraries, it is possible that adding IList or another interface to the class will cause the serializer to fail where it used to succeed.
Conclusion: If serialization does break, the changes have to be reverted.
[Video] Git\Hub Issue: #110: Add async document/element loading for XLinq
The first question raised on this review was whether to use overloads or optional parameters. For this API the question was specifically about the cancellation token, but it appears in many other scenarios.
The argument against optional parameters is that they don’t version well. It is easier to break overload resolution when adding a new overload with additional parameters when using optional parameters.
Conclusion: Optional parameters are worth using, but more guidance and code analysis rules are needed to ensure they version appropriately.
The second question was whether or not to support a URI version of the LoadAsync function. This synchronous version of the function was much maligned by the team because of the dependencies it introduces into the XLinq library.
Conclusion: If the synchronous version of the Load(string uri) can be deprecated, then the asynchronous version can be omitted. Otherwise it should be implemented for the sake of parity and ease of moving to async.
The third question is, “Should all of the overloads for the Load functions be represented by the LoadAsync function?”. This would increase the number of functions from 4 to 8, assuming that the cancellation token is still required. If that is made optional by means of overloading, then we’re looking at 16 versions of LoadAsync, 12 of which being just redirects to the four wide versions.
Conclusion: Start with the widest method signature and consider adding overloads, or optional parameters, later.
[Video] GitHub Issue: #400: Add Cast<T> and CastFrom<TDerived> to ImmutableArray<T>
For immutable arrays, there are three casting scenarios that they would like to support:
- Static casting to a base type, which will always succeed. ImmutableArray<Dog> to ImmutableArray<Animal>
- Dynamic casting, which may fail. ImmutableArray<Animal> to ImmutableArray<Dog>
- Conditionally casting to a derived type, also known as “as casting”.
Currently only the last version is fully supported. Static casting is supported via an overload of ImmutableArray.Create, but this is hard to find and it isn’t obvious that it won’t allocate memory.
The proposal is to remove the non-intuitive Create function. Then add a Cast and CastFrom function to cover the static and dynamic casting operations.
The first question for this API is whether or not the name Cast can be used. The problem with this name is that LINQ uses it for lazily casting elements. The ideal solution would be to rename the LINQ method to CaseElements<T>, but that isn’t an option.
A related question is in regards to the As method’s name. Traditionally .NET used the ToXxx for conversions that allocated memory and AsXxx for casts that did not. Now the immutable array is using As to refer to the C# ‘as’ operator instead of the traditional definition.
Another wrinkle is Visual Basic. Unlike C#, VB allows one to invoke static methods via instance variables. Though this is frowned upon, it is part of the language so static methods such as CastFrom can be unclear as to what’s being cast.
Conclusion: The names can be improved, but the proposal is otherwise solid.
[Video] GitHub Issue: #394: Add GetOrAdd and AddOrUpdate overloads to ConcurrentDictionary<TKey, TValue> that take a TArg factoryArgument
Next up is a pull request for adding new overloads to the ConcurrentDictionary. To understand the purpose of these overloads you first must understand how closures work. Whenever a closure is created, memory needs to be allocated to hold the variables it refers to. When done in a tight loop this can result in a lot of memory churn.
By contrast, the delegate that points to an anonymous function that doesn’t capture any local variables can be cached by the compiler and reused. This makes the function call free from any memory allocation.
These new overloads allow you to pass in an additional value to the factory method used in the GetOrAdd and GetOrUpdate methods. Usually this value will be enough to eliminate the need for a closure, thus reducing the memory footprint.
These methods can be created as extension methods with nearly as good performance as they would have if they were a normal method. So the question is whether or not the performance gain and convenience of having it built-in is worth it making the class larger.
A broader question is whether or not this pattern should be used across the .NET platform. Pretty much any method that takes a delegate could require a closure that would be eliminated by adopting this pattern.
Conclusion: The new methods will be added if they can be proven to significantly improve performance.
We will continue our analysis of the API review tomorrow.