Winforms: ListBox.IntegerCollection should contain unique values, but if we use the item setter, we can have duplicated

[hughbe]

• .NET Core Version:
  Master

• Have you experienced this same bug with .NET Framework?:
  Yes

Problem description:
ListBox.IntegerCollection should be a collection of unique integers. However, if we call the collection[index] = valueAlreadyInCollection we can end up with duplicates - see repro

Expected behavior:

We should not add the duplicate. We should also sort the array

Minimal repro:

Notice that the collection contains duplicates and is not sorted

[WinFormsFact]
public void ListBoxIntegerCollection_IListItem_Set_ReturnsExpected()
{
    using var owner = new ListBox();
    IList collection = new ListBox.IntegerCollection(owner);
    collection.Add(2);
    collection.Add(1);
    collection.Add(1);
    collection.Add(3);

    // Set first.
    collection[0] = 4;
    Assert.Equal(new int[] { 4, 2, 3 }, collection.Cast<int>());
    Assert.Empty(owner.CustomTabOffsets);
    Assert.False(owner.IsHandleCreated);

    // Set middle.
    collection[1] = 1;
    Assert.Equal(new int[] { 4, 1, 3 }, collection.Cast<int>());
    Assert.Empty(owner.CustomTabOffsets);
    Assert.False(owner.IsHandleCreated);

    // Set last.
    collection[2] = 4;
    Assert.Equal(new int[] { 4, 1, 4 }, collection.Cast<int>());
    Assert.Empty(owner.CustomTabOffsets);
    Assert.False(owner.IsHandleCreated);
}

[weltkante]

Expected behavior:
We should not add the duplicate. We should also sort the array

I was confused of whether this is asking for new behavior. To clarify for other readers, the described behavior are the invariants which Add and other methods already maintain, so the expected behavior is that the indexers setter maintains the same invariants as if it replacement were done via a Remove+Add pair.

[RussKie]

Wow, looks like a bug:

https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.listbox.integercollection?view=netcore-3.1#remarks

[merriemcgaw]

We could throw an exception in this situation, but it really could be a breaking change for customers. I'd be concerned about the impact of the breaking change.

[hughbe]

I’m wore incline to remove the duplicates and resort the array

[weltkante]

Having indexer-setter work but Insert throw is indeed inconsistent, but I agree that for compatibility its better to have indexer behave like Remove+Add than starting to throw now.

[hughbe]

Having indexer-setter work but Insert throw is indeed inconsistent

We could change Insert to just add the object to the array?

[weltkante]

Yes thats what some other sorted collections do. I don't have a strong opinion over doing one or the other, just wanted to raise awareness.

indexer-setter is kind of like a "ReplaceAt" operation, so you can also argue for just supporting indexer-setter and still throwing in Insert. Its mostly a thing of proper documentation I think.

The important thing is that no method should break the collection invariants of being sorted and duplicate free, so either throw or keep the invariants intact.