Fix null values in AutoCompleteCustomSource (#10905)

elachlan · web-flow · commit 6bd932c0067c · 2024-02-23T09:31:34.000-08:00
* Refactoring to add ArgumentNullException/InvalidOperationException to AutoCompleteCustomSource and add tests to verify
Cleanup/Refactor of StringSource

* Moved tests to AutoCompleteStringCollectionTests

* Changes from review

* change from review

* Use Items property instead of private backing field
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/AutoCompleteStringCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/AutoCompleteStringCollection.cs
@@ -12,7 +12,7 @@ namespace System.Windows.Forms;
 public class AutoCompleteStringCollection : IList
 {
     private CollectionChangeEventHandler? _onCollectionChanged;
-    private readonly List<string> _data = new();
+    private readonly List<string> _data = [];
 
     public AutoCompleteStringCollection()
     {
@@ -57,8 +57,17 @@ public event CollectionChangeEventHandler? CollectionChanged
     ///  Adds a string with the specified value to the
     ///  <see cref="AutoCompleteStringCollection"/> .
     /// </summary>
+    /// <returns>
+    ///  The position into which the new element was inserted, or -1 to indicate that
+    ///  the item was not inserted into the collection.
+    /// </returns>
     public int Add(string value)
     {
+        if (value is null)
+        {
+            return -1;
+        }
+
         int index = ((IList)_data).Add(value);
         OnCollectionChanged(new CollectionChangeEventArgs(CollectionChangeAction.Add, value));
         return index;
@@ -71,7 +80,21 @@ public void AddRange(params string[] value)
     {
         ArgumentNullException.ThrowIfNull(value);
 
-        _data.AddRange(value);
+        List<string> nonNullItems = [];
+        foreach (string item in value)
+        {
+            if (item is not null)
+            {
+                nonNullItems.Add(item);
+            }
+        }
+
+        if (nonNullItems.Count <= 0)
+        {
+            return;
+        }
+
+        _data.AddRange(nonNullItems);
         OnCollectionChanged(new CollectionChangeEventArgs(CollectionChangeAction.Refresh, null));
     }
 
@@ -110,8 +133,14 @@ public void Clear()
     /// </summary>
     public void Insert(int index, string value)
     {
-        _data.Insert(index, value);
-        OnCollectionChanged(new CollectionChangeEventArgs(CollectionChangeAction.Add, value));
+        ArgumentOutOfRangeException.ThrowIfNegative(index);
+        ArgumentOutOfRangeException.ThrowIfGreaterThan(index, _data.Count);
+
+        if (value is not null)
+        {
+            _data.Insert(index, value);
+            OnCollectionChanged(new CollectionChangeEventArgs(CollectionChangeAction.Add, value));
+        }
     }
 
     /// <summary>
@@ -168,4 +197,6 @@ public void RemoveAt(int index)
     void ICollection.CopyTo(Array array, int index) => ((ICollection)_data).CopyTo(array, index);
 
     public IEnumerator GetEnumerator() => _data.GetEnumerator();
+
+    internal string[] ToArray() => [.. _data];
 }
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs
@@ -915,30 +915,20 @@ private int GetComboHeight()
         return cyCombo;
     }
 
-    private string[] GetStringsForAutoComplete(IList collection)
+    private string[] GetStringsForAutoComplete()
     {
-        if (collection is AutoCompleteStringCollection)
+        if (Items is not null)
         {
-            string[] strings = new string[AutoCompleteCustomSource.Count];
-            for (int i = 0; i < AutoCompleteCustomSource.Count; i++)
-            {
-                strings[i] = AutoCompleteCustomSource[i];
-            }
-
-            return strings;
-        }
-        else if (collection is ObjectCollection && _itemsCollection is not null)
-        {
-            string[] strings = new string[_itemsCollection.Count];
-            for (int i = 0; i < _itemsCollection.Count; i++)
+            string[] strings = new string[Items.Count];
+            for (int i = 0; i < Items.Count; i++)
             {
-                strings[i] = GetItemText(_itemsCollection[i])!;
+                strings[i] = GetItemText(Items[i])!;
             }
 
             return strings;
         }
 
-        return Array.Empty<string>();
+        return [];
     }
 
     /// <summary>
@@ -3283,15 +3273,15 @@ private void SetAutoComplete(bool reset, bool recreate)
 
             if (_stringSource is null)
             {
-                _stringSource = new StringSource(GetStringsForAutoComplete(AutoCompleteCustomSource));
+                _stringSource = new StringSource(AutoCompleteCustomSource.ToArray());
                 if (!_stringSource.Bind(_childEdit, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))
                 {
                     throw new ArgumentException(SR.AutoCompleteFailure);
                 }
             }
             else
             {
-                _stringSource.RefreshList(GetStringsForAutoComplete(AutoCompleteCustomSource));
+                _stringSource.RefreshList(AutoCompleteCustomSource.ToArray());
             }
 
             return;
@@ -3326,15 +3316,15 @@ private void SetAutoComplete(bool reset, bool recreate)
 
             if (_stringSource is null)
             {
-                _stringSource = new StringSource(GetStringsForAutoComplete(Items));
+                _stringSource = new StringSource(GetStringsForAutoComplete());
                 if (!_stringSource.Bind(_childEdit, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))
                 {
                     throw new ArgumentException(SR.AutoCompleteFailureListItems);
                 }
             }
             else
             {
-                _stringSource.RefreshList(GetStringsForAutoComplete(Items));
+                _stringSource.RefreshList(GetStringsForAutoComplete());
             }
 
             return;
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/TextBox/TextBox.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/TextBox/TextBox.cs
@@ -721,17 +721,6 @@ private protected override void SelectInternal(int start, int length, int textLe
         base.SelectInternal(start, length, textLen);
     }
 
-    private string[] GetStringsForAutoComplete()
-    {
-        string[] strings = new string[AutoCompleteCustomSource.Count];
-        for (int i = 0; i < AutoCompleteCustomSource.Count; i++)
-        {
-            strings[i] = AutoCompleteCustomSource[i];
-        }
-
-        return strings;
-    }
-
     /// <summary>
     ///  Sets the AutoComplete mode in TextBox.
     /// </summary>
@@ -767,15 +756,15 @@ private unsafe void SetAutoComplete(bool reset)
                     {
                         if (_stringSource is null)
                         {
-                            _stringSource = new StringSource(GetStringsForAutoComplete());
+                            _stringSource = new StringSource(AutoCompleteCustomSource.ToArray());
                             if (!_stringSource.Bind(this, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))
                             {
                                 throw new ArgumentException(SR.AutoCompleteFailure);
                             }
                         }
                         else
                         {
-                            _stringSource.RefreshList(GetStringsForAutoComplete());
+                            _stringSource.RefreshList(AutoCompleteCustomSource.ToArray());
                         }
                     }
                 }
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/StringSource.cs b/src/System.Windows.Forms/src/System/Windows/Forms/StringSource.cs
@@ -13,21 +13,18 @@ namespace System.Windows.Forms;
 /// </summary>
 internal unsafe class StringSource : IEnumString.Interface, IManagedWrapper<IEnumString>
 {
-    private string[] strings;
-    private int current;
-    private int size;
+    private string[] _strings;
+    private int _current;
+    private int _size;
     private IAutoComplete2* _autoComplete2;
 
     /// <summary>
     ///  Constructor.
     /// </summary>
     public StringSource(string[] strings)
     {
-        Array.Clear(strings, 0, size);
-        this.strings = strings;
-
-        current = 0;
-        size = strings.Length;
+        _strings = strings;
+        _size = strings.Length;
 
         PInvokeCore.CoCreateInstance(
             in CLSID.AutoComplete,
@@ -68,10 +65,10 @@ public void ReleaseAutoComplete()
 
     public void RefreshList(string[] newSource)
     {
-        Array.Clear(strings, 0, size);
-        strings = newSource;
-        current = 0;
-        size = strings.Length;
+        Array.Clear(_strings, 0, _size);
+        _strings = newSource;
+        _current = 0;
+        _size = _strings.Length;
     }
 
     public unsafe HRESULT Clone(IEnumString** ppenum)
@@ -81,7 +78,7 @@ public unsafe HRESULT Clone(IEnumString** ppenum)
             return HRESULT.E_POINTER;
         }
 
-        *ppenum = ComHelpers.GetComPointer<IEnumString>(new StringSource(strings) { current = current });
+        *ppenum = ComHelpers.GetComPointer<IEnumString>(new StringSource(_strings) { _current = _current });
         return HRESULT.S_OK;
     }
 
@@ -94,10 +91,10 @@ public unsafe HRESULT Next(uint celt, PWSTR* rgelt, [Optional] uint* pceltFetche
 
         uint fetched = 0;
 
-        while (current < size && celt > 0)
+        while (_current < _size && celt > 0)
         {
-            rgelt[fetched] = (char*)Marshal.StringToCoTaskMemUni(strings[current]);
-            current++;
+            rgelt[fetched] = (char*)Marshal.StringToCoTaskMemUni(_strings[_current]);
+            _current++;
             fetched++;
             celt--;
         }
@@ -112,19 +109,19 @@ public unsafe HRESULT Next(uint celt, PWSTR* rgelt, [Optional] uint* pceltFetche
 
     public HRESULT Skip(uint celt)
     {
-        int newCurrent = current + (int)celt;
-        if (newCurrent >= size)
+        int newCurrent = _current + (int)celt;
+        if (newCurrent >= _size)
         {
             return HRESULT.S_FALSE;
         }
 
-        current = newCurrent;
+        _current = newCurrent;
         return HRESULT.S_OK;
     }
 
     public HRESULT Reset()
     {
-        current = 0;
+        _current = 0;
         return HRESULT.S_OK;
     }
 }
diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/AutoCompleteStringCollectionTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/AutoCompleteStringCollectionTests.cs
@@ -35,6 +35,23 @@ public void AutoCompleteStringCollection_AddRange_NullValue_ThrowsArgumentNullEx
         Assert.Throws<ArgumentNullException>("value", () => collection.AddRange(null));
     }
 
+#nullable enable
+    [WinFormsFact]
+    public void AutoCompleteStringCollection_AddRange_NullValues_Nop()
+    {
+        AutoCompleteStringCollection collection = new();
+        collection.AddRange([null!]);
+        Assert.Empty(collection);
+    }
+
+    [WinFormsFact]
+    public void AutoCompleteStringCollection_Add_NullValue_ThrowsArgumentNullException()
+    {
+        AutoCompleteStringCollection collection = new();
+        Assert.Throws<ArgumentNullException>("value", () => collection.AddRange(null!));
+    }
+#nullable disable
+
     [WinFormsFact]
     public void AutoCompleteStringCollection_Contains_Invoke_ReturnsExpected()
     {
@@ -181,6 +198,16 @@ public void AutoCompleteStringCollection_IListInsert_InvalidIndex_ThrowsArgument
         Assert.Throws<ArgumentOutOfRangeException>("index", () => collection.Insert(index, "value"));
     }
 
+#nullable enable
+    [WinFormsFact]
+    public void AutoCompleteStringCollection_IListInsert_NullItem_Nop()
+    {
+        IList collection = new AutoCompleteStringCollection();
+        collection.Insert(0, null);
+        Assert.Empty(collection);
+    }
+#nullable disable
+
     [WinFormsFact]
     public void AutoCompleteStringCollection_Remove_String_Success()
     {

Original file line number	Diff line number	Diff line change
`@@ -915,30 +915,20 @@ private int GetComboHeight()`
`915`	`915`	`return cyCombo;`
`916`	`916`	`}`
`917`	`917`
`918`		`- private string[] GetStringsForAutoComplete(IList collection)`
	`918`	`+ private string[] GetStringsForAutoComplete()`
`919`	`919`	`{`
`920`		`- if (collection is AutoCompleteStringCollection)`
	`920`	`+ if (Items is not null)`
`921`	`921`	`{`
`922`		`- string[] strings = new string[AutoCompleteCustomSource.Count];`
`923`		`- for (int i = 0; i < AutoCompleteCustomSource.Count; i++)`
`924`		`- {`
`925`		`- strings[i] = AutoCompleteCustomSource[i];`
`926`		`- }`
`927`		`-`
`928`		`- return strings;`
`929`		`- }`
`930`		`- else if (collection is ObjectCollection && _itemsCollection is not null)`
`931`		`- {`
`932`		`- string[] strings = new string[_itemsCollection.Count];`
`933`		`- for (int i = 0; i < _itemsCollection.Count; i++)`
	`922`	`+ string[] strings = new string[Items.Count];`
	`923`	`+ for (int i = 0; i < Items.Count; i++)`
`934`	`924`	`{`
`935`		`- strings[i] = GetItemText(_itemsCollection[i])!;`
	`925`	`+ strings[i] = GetItemText(Items[i])!;`
`936`	`926`	`}`
`937`	`927`
`938`	`928`	`return strings;`
`939`	`929`	`}`
`940`	`930`
`941`		`- return Array.Empty<string>();`
	`931`	`+ return [];`
`942`	`932`	`}`
`943`	`933`
`944`	`934`	`/// <summary>`
`@@ -3283,15 +3273,15 @@ private void SetAutoComplete(bool reset, bool recreate)`
`3283`	`3273`
`3284`	`3274`	`if (_stringSource is null)`
`3285`	`3275`	`{`
`3286`		`- _stringSource = new StringSource(GetStringsForAutoComplete(AutoCompleteCustomSource));`
	`3276`	`+ _stringSource = new StringSource(AutoCompleteCustomSource.ToArray());`
`3287`	`3277`	`if (!_stringSource.Bind(_childEdit, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))`
`3288`	`3278`	`{`
`3289`	`3279`	`throw new ArgumentException(SR.AutoCompleteFailure);`
`3290`	`3280`	`}`
`3291`	`3281`	`}`
`3292`	`3282`	`else`
`3293`	`3283`	`{`
`3294`		`- _stringSource.RefreshList(GetStringsForAutoComplete(AutoCompleteCustomSource));`
	`3284`	`+ _stringSource.RefreshList(AutoCompleteCustomSource.ToArray());`
`3295`	`3285`	`}`
`3296`	`3286`
`3297`	`3287`	`return;`
`@@ -3326,15 +3316,15 @@ private void SetAutoComplete(bool reset, bool recreate)`
`3326`	`3316`
`3327`	`3317`	`if (_stringSource is null)`
`3328`	`3318`	`{`
`3329`		`- _stringSource = new StringSource(GetStringsForAutoComplete(Items));`
	`3319`	`+ _stringSource = new StringSource(GetStringsForAutoComplete());`
`3330`	`3320`	`if (!_stringSource.Bind(_childEdit, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))`
`3331`	`3321`	`{`
`3332`	`3322`	`throw new ArgumentException(SR.AutoCompleteFailureListItems);`
`3333`	`3323`	`}`
`3334`	`3324`	`}`
`3335`	`3325`	`else`
`3336`	`3326`	`{`
`3337`		`- _stringSource.RefreshList(GetStringsForAutoComplete(Items));`
	`3327`	`+ _stringSource.RefreshList(GetStringsForAutoComplete());`
`3338`	`3328`	`}`
`3339`	`3329`
`3340`	`3330`	`return;`
Original file line number	Diff line number	Diff line change
`@@ -721,17 +721,6 @@ private protected override void SelectInternal(int start, int length, int textLe`
`721`	`721`	`base.SelectInternal(start, length, textLen);`
`722`	`722`	`}`
`723`	`723`
`724`		`- private string[] GetStringsForAutoComplete()`
`725`		`- {`
`726`		`- string[] strings = new string[AutoCompleteCustomSource.Count];`
`727`		`- for (int i = 0; i < AutoCompleteCustomSource.Count; i++)`
`728`		`- {`
`729`		`- strings[i] = AutoCompleteCustomSource[i];`
`730`		`- }`
`731`		`-`
`732`		`- return strings;`
`733`		`- }`
`734`		`-`
`735`	`724`	`/// <summary>`
`736`	`725`	`/// Sets the AutoComplete mode in TextBox.`
`737`	`726`	`/// </summary>`
`@@ -767,15 +756,15 @@ private unsafe void SetAutoComplete(bool reset)`
`767`	`756`	`{`
`768`	`757`	`if (_stringSource is null)`
`769`	`758`	`{`
`770`		`- _stringSource = new StringSource(GetStringsForAutoComplete());`
	`759`	`+ _stringSource = new StringSource(AutoCompleteCustomSource.ToArray());`
`771`	`760`	`if (!_stringSource.Bind(this, (AUTOCOMPLETEOPTIONS)AutoCompleteMode))`
`772`	`761`	`{`
`773`	`762`	`throw new ArgumentException(SR.AutoCompleteFailure);`
`774`	`763`	`}`
`775`	`764`	`}`
`776`	`765`	`else`
`777`	`766`	`{`
`778`		`- _stringSource.RefreshList(GetStringsForAutoComplete());`
	`767`	`+ _stringSource.RefreshList(AutoCompleteCustomSource.ToArray());`
`779`	`768`	`}`
`780`	`769`	`}`
`781`	`770`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,21 +13,18 @@ namespace System.Windows.Forms;`
`13`	`13`	`/// </summary>`
`14`	`14`	`internal unsafe class StringSource : IEnumString.Interface, IManagedWrapper<IEnumString>`
`15`	`15`	`{`
`16`		`- private string[] strings;`
`17`		`- private int current;`
`18`		`- private int size;`
	`16`	`+ private string[] _strings;`
	`17`	`+ private int _current;`
	`18`	`+ private int _size;`
`19`	`19`	`private IAutoComplete2* _autoComplete2;`
`20`	`20`
`21`	`21`	`/// <summary>`
`22`	`22`	`/// Constructor.`
`23`	`23`	`/// </summary>`
`24`	`24`	`public StringSource(string[] strings)`
`25`	`25`	`{`
`26`		`- Array.Clear(strings, 0, size);`
`27`		`- this.strings = strings;`
`28`		`-`
`29`		`- current = 0;`
`30`		`- size = strings.Length;`
	`26`	`+ _strings = strings;`
	`27`	`+ _size = strings.Length;`
`31`	`28`
`32`	`29`	`PInvokeCore.CoCreateInstance(`
`33`	`30`	`in CLSID.AutoComplete,`
`@@ -68,10 +65,10 @@ public void ReleaseAutoComplete()`
`68`	`65`
`69`	`66`	`public void RefreshList(string[] newSource)`
`70`	`67`	`{`
`71`		`- Array.Clear(strings, 0, size);`
`72`		`- strings = newSource;`
`73`		`- current = 0;`
`74`		`- size = strings.Length;`
	`68`	`+ Array.Clear(_strings, 0, _size);`
	`69`	`+ _strings = newSource;`
	`70`	`+ _current = 0;`
	`71`	`+ _size = _strings.Length;`
`75`	`72`	`}`
`76`	`73`
`77`	`74`	`public unsafe HRESULT Clone(IEnumString** ppenum)`
`@@ -81,7 +78,7 @@ public unsafe HRESULT Clone(IEnumString** ppenum)`
`81`	`78`	`return HRESULT.E_POINTER;`
`82`	`79`	`}`
`83`	`80`
`84`		`- *ppenum = ComHelpers.GetComPointer<IEnumString>(new StringSource(strings) { current = current });`
	`81`	`+ *ppenum = ComHelpers.GetComPointer<IEnumString>(new StringSource(_strings) { _current = _current });`
`85`	`82`	`return HRESULT.S_OK;`
`86`	`83`	`}`
`87`	`84`
`@@ -94,10 +91,10 @@ public unsafe HRESULT Next(uint celt, PWSTR* rgelt, [Optional] uint* pceltFetche`
`94`	`91`
`95`	`92`	`uint fetched = 0;`
`96`	`93`
`97`		`- while (current < size && celt > 0)`
	`94`	`+ while (_current < _size && celt > 0)`
`98`	`95`	`{`
`99`		`- rgelt[fetched] = (char*)Marshal.StringToCoTaskMemUni(strings[current]);`
`100`		`- current++;`
	`96`	`+ rgelt[fetched] = (char*)Marshal.StringToCoTaskMemUni(_strings[_current]);`
	`97`	`+ _current++;`
`101`	`98`	`fetched++;`
`102`	`99`	`celt--;`
`103`	`100`	`}`
`@@ -112,19 +109,19 @@ public unsafe HRESULT Next(uint celt, PWSTR* rgelt, [Optional] uint* pceltFetche`
`112`	`109`
`113`	`110`	`public HRESULT Skip(uint celt)`
`114`	`111`	`{`
`115`		`- int newCurrent = current + (int)celt;`
`116`		`- if (newCurrent >= size)`
	`112`	`+ int newCurrent = _current + (int)celt;`
	`113`	`+ if (newCurrent >= _size)`
`117`	`114`	`{`
`118`	`115`	`return HRESULT.S_FALSE;`
`119`	`116`	`}`
`120`	`117`
`121`		`- current = newCurrent;`
	`118`	`+ _current = newCurrent;`
`122`	`119`	`return HRESULT.S_OK;`
`123`	`120`	`}`
`124`	`121`
`125`	`122`	`public HRESULT Reset()`
`126`	`123`	`{`
`127`		`- current = 0;`
	`124`	`+ _current = 0;`
`128`	`125`	`return HRESULT.S_OK;`
`129`	`126`	`}`
`130`	`127`	`}`