ListViewItemCollection doesn't have an AddRange that takes an `IE…

…numerable<ListViewItem>`, which necessitates unnecessary array allocations when adding from a collection.

It also doesn't implement a generic `IList<T>` which makes typed usage and LINQ usage difficult. This adds that.

Also add `IEnumerable<Control>` to `ControlCollection` to address the LINQ scenario. Control has `IList`, but indexed setting throws. I don't want to add `IList<Control>` until we've had time to evaluate the implications of implicitly doing the public steps with the collection that replicate what those setters should be doing.

Also tweak `ArrangedElementCollection` so the static empty collection can't be written to.

This doesn't fix the ambiguity of `AddRange([..])` with `ListViewItemCollection`, but it does avoid the need for it in some cases.

src/System.Windows.Forms/src/PublicAPI.Unshipped.txt

@@ -0,0 +1,2 @@
+System.Windows.Forms.ListView.ListViewItemCollection.AddRange(System.Collections.Generic.IEnumerable<System.Windows.Forms.ListViewItem!>! collection) -> void
+System.Windows.Forms.ListView.ListViewItemCollection.CopyTo(System.Windows.Forms.ListViewItem![]! array, int arrayIndex) -> void

src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs

@@ -10,10 +10,10 @@ namespace System.Windows.Forms;
 public partial class Control
 {
     /// <summary>
-    ///  Collection of controls...
+    ///  Collection of controls.
     /// </summary>
     [ListBindable(false)]
-    public partial class ControlCollection : ArrangedElementCollection, IList, ICloneable
+    public partial class ControlCollection : ArrangedElementCollection, IList, IEnumerable<Control>, ICloneable
     {
         ///  A caching mechanism for key accessor
         ///  We use an index here rather than control so that we don't have lifetime
@@ -29,10 +29,7 @@ public ControlCollection(Control owner)
         /// <summary>
         ///  Returns true if the collection contains an item with the specified key, false otherwise.
         /// </summary>
-        public virtual bool ContainsKey(string? key)
-        {
-            return IsValidIndex(IndexOfKey(key));
-        }
+        public virtual bool ContainsKey(string? key) => IsValidIndex(IndexOfKey(key));
 
         /// <summary>
         ///  Adds a child control to this control. The control becomes the last control in
@@ -156,11 +153,23 @@ object ICloneable.Clone()
         {
             // Use CreateControlInstance so we get the same type of ControlCollection, but whack the
             // owner so adding controls to this new collection does not affect the control we cloned from.
-            ControlCollection ccOther = Owner.CreateControlsInstance();
+            ControlCollection clone = Owner.CreateControlsInstance();
 
             // We add using InnerList to prevent unnecessary parent cycle checks, etc.
-            ccOther.InnerList.AddRange(InnerList);
-            return ccOther;
+            if (clone.InnerList is List<IArrangedElement> list)
+            {
+                list.AddRange(InnerList);
+            }
+            else
+            {
+                Debug.Fail("Unexpected InnerList type");
+                foreach (Control c in this)
+                {
+                    clone.Add(c);
+                }
+            }
+
+            return clone;
         }
 
         public bool Contains(Control? control) => ((IList)InnerList).Contains(control);
@@ -224,10 +233,7 @@ private static void FindInternal(string key, bool searchAllChildren, ControlColl
             }
         }
 
-        public override IEnumerator GetEnumerator()
-        {
-            return new ControlCollectionEnumerator(this);
-        }
+        public override IEnumerator GetEnumerator() => new ControlCollectionEnumerator(this);
 
         public int IndexOf(Control? control) => ((IList)InnerList).IndexOf(control);
 
@@ -269,10 +275,7 @@ public virtual int IndexOfKey(string? key)
         /// <summary>
         ///  Determines if the index is valid for the collection.
         /// </summary>
-        private bool IsValidIndex(int index)
-        {
-            return ((index >= 0) && (index < Count));
-        }
+        private bool IsValidIndex(int index) => (index >= 0) && (index < Count);
 
         /// <summary>
         ///  Who owns this control collection.
@@ -285,10 +288,9 @@ private bool IsValidIndex(int index)
         /// </summary>
         public virtual void Remove(Control? value)
         {
-            // Sanity check parameter
             if (value is null)
             {
-                return;     // Don't do anything
+                return;
             }
 
             if (value.ParentInternal == Owner)
@@ -317,10 +319,7 @@ void IList.Remove(object? control)
             }
         }
 
-        public void RemoveAt(int index)
-        {
-            Remove(this[index]);
-        }
+        public void RemoveAt(int index) => Remove(this[index]);
 
         /// <summary>
         ///  Removes the child control with the specified key.
@@ -394,7 +393,7 @@ public virtual void Clear()
         ///  Retrieves the index of the specified child control in this array. An ArgumentException
         ///  is thrown if child is not parented to this Control.
         /// </summary>
-        public int GetChildIndex(Control child) => GetChildIndex(child, true);
+        public int GetChildIndex(Control child) => GetChildIndex(child, throwException: true);
 
         /// <summary>
         ///  Retrieves the index of the specified child control in this array. An ArgumentException
@@ -444,5 +443,13 @@ internal virtual void SetChildIndexInternal(Control child, int newIndex)
         /// </summary>
         public virtual void SetChildIndex(Control child, int newIndex) =>
             SetChildIndexInternal(child, newIndex);
+
+        IEnumerator<Control> IEnumerable<Control>.GetEnumerator()
+        {
+            foreach (Control control in InnerList)
+            {
+                yield return control;
+            }
+        }
     }
 }

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

@@ -4695,20 +4695,14 @@ public bool Contains([NotNullWhen(true)] Control? ctl)
     ///  should not call base.CreateAccessibilityObject.
     /// </summary>
     [EditorBrowsable(EditorBrowsableState.Advanced)]
-    protected virtual AccessibleObject CreateAccessibilityInstance()
-    {
-        return new ControlAccessibleObject(this);
-    }
+    protected virtual AccessibleObject CreateAccessibilityInstance() => new ControlAccessibleObject(this);
 
     /// <summary>
     ///  Constructs the new instance of the Controls collection objects. Subclasses
     ///  should not call base.CreateControlsInstance.
     /// </summary>
     [EditorBrowsable(EditorBrowsableState.Advanced)]
-    protected virtual ControlCollection CreateControlsInstance()
-    {
-        return new ControlCollection(this);
-    }
+    protected virtual ControlCollection CreateControlsInstance() => new ControlCollection(this);
 
     /// <summary>
     ///  Creates a Graphics for this control. The control's brush, font, foreground

src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewItemCollection.cs

@@ -12,7 +12,7 @@ public partial class ListView
     ///  Represents the collection of items in a ListView or ListViewGroup
     /// </summary>
     [ListBindable(false)]
-    public partial class ListViewItemCollection : IList
+    public partial class ListViewItemCollection : IList, IList<ListViewItem>
     {
         ///  A caching mechanism for key accessor
         ///  We use an index here rather than control so that we don't have lifetime
@@ -75,10 +75,7 @@ public virtual ListViewItem this[int index]
 
         object? IList.this[int index]
         {
-            get
-            {
-                return this[index];
-            }
+            get => this[index];
             set
             {
                 this[index] = value is ListViewItem item
@@ -116,10 +113,7 @@ public virtual ListViewItem? this[string key]
         ///  the correct sorted position, or, if no sorting is set, at the end
         ///  of the list.
         /// </summary>
-        public virtual ListViewItem Add(string? text)
-        {
-            return Add(text, -1);
-        }
+        public virtual ListViewItem Add(string? text) => Add(text, -1);
 
         int IList.Add(object? item)
         {
@@ -159,8 +153,6 @@ public virtual ListViewItem Add(ListViewItem value)
             return value;
         }
 
-        // <-- NEW ADD OVERLOADS IN WHIDBEY
-
         /// <summary>
         ///  Add an item to the ListView.  The item will be inserted either in
         ///  the correct sorted position, or, if no sorting is set, at the end
@@ -203,8 +195,6 @@ public virtual ListViewItem Add(string? key, string? text, int imageIndex)
             return item;
         }
 
-        // END - NEW ADD OVERLOADS IN WHIDBEY  -->
-
         public void AddRange(params ListViewItem[] items)
         {
             ArgumentNullException.ThrowIfNull(items);
@@ -224,10 +214,7 @@ public void AddRange(ListViewItemCollection items)
         /// <summary>
         ///  Removes all items from the list view.
         /// </summary>
-        public virtual void Clear()
-        {
-            InnerList.Clear();
-        }
+        public virtual void Clear() => InnerList.Clear();
 
         public bool Contains(ListViewItem item)
         {
@@ -335,7 +322,7 @@ public virtual int IndexOfKey(string? key)
             // step 1 - check the last cached item
             if (IsValidIndex(_lastAccessedIndex))
             {
-                if (WindowsFormsUtils.SafeCompareStrings(this[_lastAccessedIndex].Name, key, /* ignoreCase = */ true))
+                if (WindowsFormsUtils.SafeCompareStrings(this[_lastAccessedIndex].Name, key, ignoreCase: true))
                 {
                     return _lastAccessedIndex;
                 }
@@ -344,7 +331,7 @@ public virtual int IndexOfKey(string? key)
             // step 2 - search for the item
             for (int i = 0; i < Count; i++)
             {
-                if (WindowsFormsUtils.SafeCompareStrings(this[i].Name, key, /* ignoreCase = */ true))
+                if (WindowsFormsUtils.SafeCompareStrings(this[i].Name, key, ignoreCase: true))
                 {
                     _lastAccessedIndex = i;
                     return i;
@@ -395,8 +382,6 @@ void IList.Insert(int index, object? item)
             }
         }
 
-        // <-- NEW INSERT OVERLOADS IN WHIDBEY
-
         public ListViewItem Insert(int index, string? text, string? imageKey)
             => Insert(index, new ListViewItem(text, imageKey));
 
@@ -412,8 +397,6 @@ public virtual ListViewItem Insert(int index, string? key, string? text, int ima
                 Name = key
             });
 
-        // END - NEW INSERT OVERLOADS IN WHIDBEY -->
-
         /// <summary>
         ///  Removes an item from the ListView
         /// </summary>
@@ -452,5 +435,42 @@ void IList.Remove(object? item)
                 Remove(listViewItem);
             }
         }
+
+        void IList<ListViewItem>.Insert(int index, ListViewItem item) => Insert(index, item);
+
+        void ICollection<ListViewItem>.Add(ListViewItem item) => Add(item);
+
+        public void CopyTo(ListViewItem[] array, int arrayIndex) => CopyTo((Array)array, arrayIndex);
+
+        bool ICollection<ListViewItem>.Remove(ListViewItem item)
+        {
+            if (Contains(item))
+            {
+                Remove(item);
+                return true;
+            }
+
+            return false;
+        }
+
+        IEnumerator<ListViewItem> IEnumerable<ListViewItem>.GetEnumerator()
+        {
+            var enumerator = GetEnumerator();
+            while (enumerator.MoveNext())
+            {
+                if (enumerator.Current is ListViewItem item)
+                {
+                    yield return item;
+                }
+            }
+        }
+
+        public void AddRange(IEnumerable<ListViewItem> collection)
+        {
+            foreach (ListViewItem item in collection)
+            {
+                Add(item);
+            }
+        }
     }
 }

src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripPanel.ToolStripPanelControlCollection.cs

@@ -34,14 +34,13 @@ internal override void AddInternal(Control? value)
 
         internal void Sort()
         {
-            if (_owner.Orientation == Orientation.Horizontal)
+            if (InnerList is not List<IArrangedElement> list)
             {
-                InnerList.Sort(new YXComparer());
-            }
-            else
-            {
-                InnerList.Sort(new XYComparer());
+                Debug.Fail("InnerList is not a List<IArrangedElement>?");
+                return;
             }
+
+            list.Sort(_owner.Orientation == Orientation.Horizontal ? new YXComparer() : new XYComparer());
         }
     }
 }

src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs

@@ -7,17 +7,17 @@ namespace System.Windows.Forms.Layout;
 
 public class ArrangedElementCollection : IList
 {
-    internal static ArrangedElementCollection Empty { get; } = new(0);
+    internal static ArrangedElementCollection Empty { get; } = new(new List<IArrangedElement>().AsReadOnly());
 
     internal ArrangedElementCollection() : this(4)
     {
     }
 
-    internal ArrangedElementCollection(List<IArrangedElement> innerList) => InnerList = innerList;
+    internal ArrangedElementCollection(IList<IArrangedElement> innerList) => InnerList = innerList;
 
     private ArrangedElementCollection(int size) => InnerList = new List<IArrangedElement>(size);
 
-    private protected List<IArrangedElement> InnerList { get; }
+    private protected IList<IArrangedElement> InnerList { get; }
 
     internal virtual IArrangedElement this[int index] => InnerList[index];
 
@@ -93,7 +93,12 @@ private protected void MoveElement(IArrangedElement element, int fromIndex, int
         InnerList[toIndex] = element;
     }
 
-    private static void Copy(ArrangedElementCollection sourceList, int sourceIndex, ArrangedElementCollection destinationList, int destinationIndex, int length)
+    private static void Copy(
+        ArrangedElementCollection sourceList,
+        int sourceIndex,
+        ArrangedElementCollection destinationList,
+        int destinationIndex,
+        int length)
     {
         if (sourceIndex < destinationIndex)
         {

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs

@@ -146,7 +146,7 @@ static void ValidateResult(string blob)
             Assert.Equal(HorizontalAlignment.Center, result.HeaderAlignment);
             Assert.Equal("Tag", result.Tag);
             Assert.Equal("GroupName", result.Name);
-            var item = Assert.Single(result.Items) as ListViewItem;
+            var item = Assert.Single(result.Items);
             Assert.NotNull(item);
             Assert.Equal("Item", item.Text);
         }