From 6bd932c0067c48808fd6d7f733812e46a4ea18f2 Mon Sep 17 00:00:00 2001 From: Lachlan Ennis <2433737+elachlan@users.noreply.github.com> Date: Sat, 24 Feb 2024 03:31:34 +1000 Subject: [PATCH] Fix `null` values in `AutoCompleteCustomSource` (#10905) * 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 --- .../Forms/AutoCompleteStringCollection.cs | 39 +++++++++++++++++-- .../Forms/Controls/ComboBox/ComboBox.cs | 30 +++++--------- .../Windows/Forms/Controls/TextBox/TextBox.cs | 15 +------ .../src/System/Windows/Forms/StringSource.cs | 37 ++++++++---------- .../AutoCompleteStringCollectionTests.cs | 27 +++++++++++++ 5 files changed, 91 insertions(+), 57 deletions(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/AutoCompleteStringCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/AutoCompleteStringCollection.cs index 3c8fea30ddd..0562e5f9138 100644 --- 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 _data = new(); + private readonly List _data = []; public AutoCompleteStringCollection() { @@ -57,8 +57,17 @@ public event CollectionChangeEventHandler? CollectionChanged /// Adds a string with the specified value to the /// . /// + /// + /// The position into which the new element was inserted, or -1 to indicate that + /// the item was not inserted into the collection. + /// 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 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() /// 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)); + } } /// @@ -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 index 47c4b708544..26e2000c1db 100644 --- 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(); + return []; } /// @@ -3283,7 +3273,7 @@ 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); @@ -3291,7 +3281,7 @@ private void SetAutoComplete(bool reset, bool recreate) } else { - _stringSource.RefreshList(GetStringsForAutoComplete(AutoCompleteCustomSource)); + _stringSource.RefreshList(AutoCompleteCustomSource.ToArray()); } return; @@ -3326,7 +3316,7 @@ 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); @@ -3334,7 +3324,7 @@ private void SetAutoComplete(bool reset, bool recreate) } 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 index 82149a87784..2973820e324 100644 --- 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; - } - /// /// Sets the AutoComplete mode in TextBox. /// @@ -767,7 +756,7 @@ 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); @@ -775,7 +764,7 @@ private unsafe void SetAutoComplete(bool reset) } 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 index 936ab654470..1e32739c3cf 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/StringSource.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/StringSource.cs @@ -13,9 +13,9 @@ namespace System.Windows.Forms; /// internal unsafe class StringSource : IEnumString.Interface, IManagedWrapper { - private string[] strings; - private int current; - private int size; + private string[] _strings; + private int _current; + private int _size; private IAutoComplete2* _autoComplete2; /// @@ -23,11 +23,8 @@ internal unsafe class StringSource : IEnumString.Interface, IManagedWrapper 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(new StringSource(strings) { current = current }); + *ppenum = ComHelpers.GetComPointer(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 index dcab87e1a92..e030dfa9118 100644 --- 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("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("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("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() {