API Proposal: Expose a base collection that prevents null items
- Dominant language
- C#
- Stars
- 4.9k
- Forks
- 1.1k
- Avg merge
- 1d 13m
- Merged PRs (30d)
- 85
Description
We have a number of collections in WinForms that are currently untyped but also attempt to disallow nulls (either in the collections themselves or in the consumers).
We have an internal base collection we're using that we should consider making public and consider deriving from in our public collections.
``` C#
namespace System.Windows.Forms
{
///
/// Collection that protects against inserting null objects.
///
internal abstract class NonNullCollection
: IList, ICollection, IEnumerable, IEnumerable, IList, ICollection, IReadOnlyList, IReadOnlyCollection
where T : class
{
public NonNullCollection();
public NonNullCollection(IEnumerable items);
// These would match the behavior of `System.Collections.CollectionBase` as we frequently use it and would mitigate replacing
// that base class on unsealed Windows Forms collections. We could have both typed and untyped (original) or go with only
// one or the other.
protected virtual void OnClear();
protected virtual void OnClearComplete();
protected virtual void OnInsert(int index, T value);
protected virtual void OnInsertComplete(int index, T value);
protected virtual void OnRemove(int index, T value);
protected virtual void OnRemoveComplete(int index, T value);
protected virtual void OnSet(int index, T oldValue, T newValue);
protected virtual void OnSetComplete(int index, T oldValue, T newValue);
protected virtual void OnValidate(T value);
// The rest of the methods are all for the inherited interfaces and add null validation on manipulation of the internal List
public T this[int index];
public int Count;
public bool IsReadOnly;
public void Add(T item);
public void Clear();
public bool Contains(T item);
public void CopyTo(T[] array, int arrayIndex);
public IEnumerator GetEnumerator();
public int IndexOf(T item);
public void Insert(int index, T item);
public bool Remove(T item);
public void RemoveAt(int index);
IEnumerator IEnumerable.GetEnumerator();
public void AddRange(IEnumerable items);
int IList.Add(object? value);
bool IList.Contains(object? value);
int IList.IndexOf(object? value);
void IList.Insert(int index, object? value);
void IList.Remove(object? value);
object? IList.this[int index];
void ICollection.CopyTo(Array array, int index);
private string DebuggerDisplay;
bool IList.IsFixedSize;
object ICollection.SyncRoot;
bool ICollection.IsSynchronized;
}
}
```
Any place we just implement `IList` would move to this collection (presuming they don't like nulls), such as the following:
``` diff
namespace System.Windows.Forms
{
public partial class ListViewItem
{
- public class ListViewSubItemCollection : IList
+ public class ListViewSubItemCollection : NonNullCollection
}
}
```
We should consider moving class that derive from `CollectionBase` to this. It is a breaking change if you're casting directly to CollectionBase. It is also a breaking change if you derive from these. We could mitigate the derivation case by providing the same virtuals `CollectionBase` has.
We can also consider introducing an adapter `CollectionBase` that would still allow casting the collections against `CollectionBase`. If we did that we'd depreciate the cast operator out of the gate to discourage using it (as it would have to copy the collection into the `ArrayList` in `CollectionBase` and sync the two). I'm not fond of this and think we should just take the breaking change.
One other possibility is to extend this proposal to include modifications to `CollectionBase` to allow providing your own backing collection, but as the current one is exposed via `protected ArrayList CollectionBase.InnerList { get; }` that probably won't fly.
For example:
``` diff
namespace System.Windows.Forms.Design.Behavior
{
- public sealed class BehaviorServiceAdornerCollection : CollectionBase
+ public sealed class BehaviorServiceAdornerCollection : NonNullCollection
- public class GlyphCollection : CollectionBase
+ public class GlyphCollection : NonNullCollection
}
```
This is just a start on this to get initial thoughts down and start gathering feedback.
Contributor guide
Assessment
This issue has not been assessed yet.