API Proposal: Expose a base collection that prevents null items

[JeremyKuhne]

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.

namespace System.Windows.Forms
{
    /// <summary>
    ///  Collection that protects against inserting null objects.
    /// </summary>
    internal abstract class NonNullCollection<T>
        : IList<T>, ICollection<T>, IEnumerable<T>, IEnumerable, IList, ICollection, IReadOnlyList<T>, IReadOnlyCollection<T>
        where T : class
    {
        public NonNullCollection();
        public NonNullCollection(IEnumerable<T> 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<T>
        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<T> 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<T> 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:

namespace System.Windows.Forms
{
    public partial class ListViewItem
    {
-        public class ListViewSubItemCollection : IList
+        public class ListViewSubItemCollection : NonNullCollection<ListViewSubItem>
    }
}

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:

namespace System.Windows.Forms.Design.Behavior
{
-    public sealed class BehaviorServiceAdornerCollection : CollectionBase
+    public sealed class BehaviorServiceAdornerCollection : NonNullCollection<Adorner>
-    public class GlyphCollection : CollectionBase
+    public class GlyphCollection : NonNullCollection<Glyph>
}

This is just a start on this to get initial thoughts down and start gathering feedback.

[RussKie]

This is related to #2644

/cc: @weltkante

[weltkante]

I canceled my work on that because the desired compatibility requirement of executing code against newer frameworks without recompiling were too high. You'll likely run into similar issues by introducing a base class, since existing code will have been linked against the "old" public API of the subclass. Changing the API surface will cause runtime exceptions like MissingMethodException. You have to go through weird contortions to keep this compatible, by keeping the old public API binary compatible and not replacing it with strongly typed methods. The base class basically will not be visible this way since the subclass needs to keep its API and will shadow the base class methods.

Collection classes which are subclassable make this even more complicated, because the subclassing contract can't be changed and is based on an untyped list.

If the compatibility requirements were dropped and you're willing to introduce breaking changes (most importantly, code from previous .NET versions no longer running without recompiling) I may take a second look at #2644, but maybe lets keep the discussion focused on this first, because its much more limited in scope.