ecsharp icon indicating copy to clipboard operation
ecsharp copied to clipboard

Remove support for IList and ICollection in various adapters and extension methods?

Open qwertie opened this issue 6 years ago • 3 comments

It used to be that things like IReadOnlyList<T> didn't exist, so I implemented my own equivalent versions (such as IListSource<T> and its write-only cousin IListSink<T>).

I implemented a whole bunch of adapters on top of these interfaces, but I also wanted to support standard types like List<T> and HashSet<T> and Dictionary<T>, so I would also make adapters and extension methods that support IList<T> and ICollection<T>.

This caused ambiguity errors in C#, because my collections would implement both IList<T> and IListSource<T>. To solve this I introduced IListAndListSource<T>:

public interface IListAndListSource<T> : IList<T>, IListSource<T> {}

Then to avoid potential ambiguity errors, if I had overloads that accepted IList and IListSource, I wrote a third overload that took IListAndListSource. Problem solved.

Later, when .NET added their own similar interfaces, I made my interfaces derive from them (example: IListSource<T> contains a superset of the functionality in IReadOnlyList<T> and its name is more accurate, since a IReadOnlyList<T> isn't necessarily read-only.) And I introduced similar helper interfaces like ICollectionAndReadOnly:

public interface ICollectionAndReadOnly<T> : ICollection<T>, IReadOnlyCollection<T> { }
public interface IListAndListSource<T> : IList<T>, IListSource<T>, ICollectionAndReadOnly<T> { }

Unfortunately when Microsoft added new interfaces like IReadOnlyList<T>, they didn't change IList<T> to derive from the new interface. However, List<T> implements both! So if I made overloads that accept both IReadOnlyList<T> and IList<T>, boom, a new ambiguity error would appear and I would have to write a special overload just for List<T>, or just for arrays, or just for HashSet<T>. This kind of problem has persisted until now because (1) I didn't see the way out and (2) my libraries still support .NET 3.5 and .NET 4.0 (IReadOnlyList is new in .NET 4.5).

I think I see a way out now:

  1. Drop support for IList and ICollection in all extension methods (and other methods) that use collections in a read-only way. Then, disambiguation overloads taking IListAndListSource et al can be removed.
  2. I could either drop support for .NET < 4.5, or preserve support with a bunch of #if directives everywhere. Dropping support sounds easier, but on the other hand I would feel obliged to strip out all the existing #if directives everywhere for supporting old framework versions.
  3. Assuming I drop .NET < 4.5, I could also drop the adapters corresponding to the extension methods. This isn't necessary, but LoycCore has few users so I'm not too worried about people really needing them.
  4. Optionally, I could also drop several adapters for IList<T> that use the list in a read-only way.

Note: I will keep interfaces like ICollectionAndReadOnly around as they could still be necessary in rare circumstances (e.g. you want to write an overloaded method that handles a writable collection differently than a read-only collection.)

qwertie avatar May 16 '19 18:05 qwertie

I just realized this isn't (always) a satisfying answer for dictionaries.

Today I added a class called SelectDictionaryFromKeys for adapting the interface of a dictionary on the fly (AsReadOnlyDictionary extension method). It's useful, for example, when you need to implement an interface that provides a dictionary of values, but your data is in a different format than the interface demands. You don't want to convert the entire dictionary, since the caller might only need to look up one item from it. Example:

interface ICompany
{
	IReadOnlyDictionary<long, string> Employees { get; }
	...
}
class Company : ICompany
{
	Dictionary<int, Person> _employees = new Dictionary<int, Person>();

	public IReadOnlyDictionary<long, string> Employees => 
		LinqToLists.Select(_employees.Keys, k => (long)k)
		.AsReadOnlyDictionary(k => {
		    var v = _employees.TryGetValue((int)k);
		    return v.HasValue ? (Maybe<string>)v.Value.ToString() : Maybe<string>.NoValue;
		});
	...
}
class Person
{
	string FirstName, LastName;
	public override string ToString() => FirstName + " " + LastName;
}

This works great as long as LinqToLists.Select accepts an ICollection, since that's all Dictionary.Keys provides. (My own dictionaries provide a richer KeyCollection that implements ICollectionAndReadOnly<TKey> -- both IReadOnlyCollection and ICollection). So it seems I need, for this case at least, overloads that take ICollection in addition to the read-only equivalent, IReadOnlyCollection. Guess I'll have to deal with the inevitable so-called "ambiguity" on a case-by-case basis...

And by the way, IReadOnlyCollection's Keys and Values properties return IEnumerable but AsReadOnlyDictionary needs an IReadOnlyCollection so it knows how big the dictionary is. Pft. Looks like I'll need to add something here because right now readOnlyDict.Keys.AsReadOnlyDictionary(...) won't work.

qwertie avatar May 16 '19 23:05 qwertie

Plan B: don't remove support IList and ICollection extension methods outright, but put them in another namespace so they don't cause "ambiguity" every time I want to use an extension method on an array or List<T>.

qwertie avatar Feb 09 '20 16:02 qwertie

I've committed some changes in this direction, mostly moving extension methods on ICollection and IList to a new namespace called Loyc.Collections.MutableListExtensionMethods.

TODO: after dropping support for .NET 3.5/4, do another pass to remove superfluous methods like TryGet<T>(this T[] list, int index) that exist only because arrays and List<T> did not implement IReadOnlyList<T> in those versions of .NET. Edit: this task looks done.

qwertie avatar Feb 09 '20 17:02 qwertie