WIP: implement generic IList interface #2749

weltkante · 2020-01-21T18:31:36Z

Contributes to #2644
Alternative implementation to PR #2999

This is a WIP PR for exploring the impact #2644 may have and whether its worth taking the breaking change.

This PR contains adding IList<T> interface implementation to types currently implementing IList. I thought it'd be a good way to start the work. If you want me to split down further in either multiple commits or separate PRs I don't mind, I fully expect having to go through the full changes several times anyways. On that note, if you have nits don't bother reviewing/annotating all of them, just pick one representative for discussion.

Notes:

As far as placement of new code goes I tried to orient myself on the corresponding IList methods (where available), this makes for weird placement since not all IList<T> methods are together, but considering I don't really want to reorder the existing methods (unless asked to) I figured it'd be the best solution to minimize diffs for now.
There are plenty of odd design decisions in the original collections subclassing hierarchy which makes updating them harder.
Some subclass hierarchies could profit from adding generics to their signatures. Of course this is a highly breaking change.
Some classes expose a backing collection (usually of type ArrayList) to their subclasses. Should we break subclasses by changing the type of the backing list to a generic collection? Nullability annotations would profit from such a change.
Existing Remove implementations don't tell the caller whether they removed something, but IList<T> requires this in its API. I've updated them where it looked sensible but some Remove implementations are virtual and changing return type to bool would require updating subclasses. I opted to not do this in this PR before discussing this point, instead for now I did implement IList<T>.Remove as explicit interface with a weird workaround. That is just placeholder until it is clear what is desired.
- Should we require updating subclasses and change the return type?
- If not, how much do we want to assume about how the subclass behaves? (This decides how weird/complicated the explicit interface implementation has to be.)
There are bugs in some existing collection classes where they don't check in Remove implementations whether the item to be removed is actually part of the collection. They run logic which severs the item and partially disconnects it from its true owner, but without notifying that owner. I did not fix these bugs yet (some are marked with comments though), I'm going to create a separate issue for this problem (so it doesn't have to wait for this issue/PR, which probably will take a while to dicscuss and resolve all points).
xunit apparently doesn't support non-generic lists, so when you start implementing the generic list interface all kinds of collection-tests stop compiling because some analyzer wants you to use Assert.Contains or Assert.DoesNotContain. Minor inconvenience which is easily fixed.

Microsoft Reviewers: Open in CodeFlow

codecov · 2020-01-21T18:46:33Z

Codecov Report

Merging #2749 into master will decrease coverage by 3.09285%.
The diff coverage is 27.4%.

@@                Coverage Diff                 @@
##              master      #2749         +/-   ##
==================================================
- Coverage   61.94595%   58.8531%   -3.09285%     
==================================================
  Files           1256       1240         -16     
  Lines         447545     428861      -18684     
  Branches       39209      38934        -275     
==================================================
- Hits          277236     252398      -24838     
- Misses        164844     171122       +6278     
+ Partials        5465       5341        -124

Flag	Coverage Δ
#Debug	`58.8531% <27.4%> (-3.09286%)`	⬇️
#production	`31.8017% <20.74236%> (-1.45615%)`	⬇️
#test	`98.97457% <100%> (+0.00131%)`	⬆️

...stem.Windows.Forms/tests/UnitTests/System/Windows/Forms/TabControl.TabPageCollectionTests.cs

weltkante · 2020-01-22T22:48:05Z

src/System.Windows.Forms/src/System/Windows/Forms/ArraySubsetEnumerator.cs

 using System.Diagnostics;

 namespace System.Windows.Forms
 {
-    public class ArraySubsetEnumerator : IEnumerator
+    public class ArraySubsetEnumerator<T> : IEnumerator<T>


While this may look like a major breaking change it isn't, the class previously was internal and mistakenly was made public in a recent refactoring.

I noticed it as well:

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

Lines 1 to 5 in ad8ff38

System.Windows.Forms.ArraySubsetEnumerator

System.Windows.Forms.ArraySubsetEnumerator.ArraySubsetEnumerator(object[] array, int count) -> void

System.Windows.Forms.ArraySubsetEnumerator.Current.get -> object

System.Windows.Forms.ArraySubsetEnumerator.MoveNext() -> bool

System.Windows.Forms.ArraySubsetEnumerator.Reset() -> void

We need to make it internal again.

weltkante

This iteration tries to limit signature changes to GetEnumerator methods to see whats required to enable foreach picking up the generic signature. Turns out a few classes have problems with that because their GetEnumerator is virtual.

weltkante · 2020-01-26T20:54:27Z

src/System.Windows.Forms/src/System/Windows/Forms/AutoCompleteStringCollection.cs

+        public void Remove(string value) => TryRemove(value);
+
+        bool ICollection<string>.Remove(string value) => TryRemove(value);
+
+        private bool TryRemove(string value)


In this PR iteration instead of changing return type of public Remove methods I instead add a private TryRemove method to avoid code duplication so that interfaces can use the same call.

weltkante · 2020-01-26T20:57:37Z

src/System.Windows.Forms/src/System/Windows/Forms/CheckedListBox.cs

            {
                int[] indices = new int[Count];
                CopyTo(indices, 0);
-                return indices.GetEnumerator();
+                return WindowsFormsUtils.GetArrayEnumerator(indices);


Getting the generic IEnumerator<> from an array requires casting the array, going through a helper function to avoid getting the cast wrong. If you prefer inlined casts I can of course remove this helper function.

weltkante · 2020-01-26T20:59:34Z

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

+            public new virtual IEnumerator<Control> GetEnumerator()
            {
                return new ControlCollectionEnumerator(this);
            }

+            protected override IEnumerator GetEnumeratorCore()
+            {
+                return GetEnumerator();
+            }


This is an attempt to enable foreach picking up the generic enumerator, if you have better suggestions I'm open to try them out.

This particular attempt is adding a second virtual mehtod GetEnumeratorCore on the base class so subclasses can both override and shadow the GetEnumerator method at the same time. Alternatives which do less contortions (e.g. adding a generic parameter to the base class or changing how GetEnumerator is exposed on the base class) would probably be even more breaking to the subclass hierarchy. Since for now we want to explore solutions with minimum breaking changes while still getting the advantages of foreach signatures I chose to explore above workaround.

weltkante · 2020-01-26T21:07:20Z

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

@@ -536,6 +536,7 @@ public void ColumnHeaderCollection_Clear_Empty_Success()
        }

        [Fact]
+        [Diagnostics.CodeAnalysis.SuppressMessage("Assertions", "xUnit2017:Do not use Contains() to check if a value exists in a collection", Justification = "We are testing the Contains method itself")]


Do you prefer #pragma warning disable over attributes? Can update if desired.

weltkante · 2020-01-26T21:08:36Z

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

@@ -15,7 +15,7 @@ public class BindingsCollectionTests : IClassFixture<ThreadExceptionFixture>
        public void Ctor_Default()
        {
            var collection = new SubBindingsCollection();
-            Assert.Equal(0, collection.Count);
+            Assert.Empty(collection);


These are not testing the Count property explicitely so I followed the xunit analyzer and updated the call

weltkante · 2020-06-02T18:34:11Z

Closing PR, changing GetEnumerator signatures is not acceptable due to binary compatibility requirements - it is expected to run 3.x binaries on 5.0 (see issue discussion for alternative approaches)

weltkante mentioned this pull request Jan 21, 2020

Collection Remove methods are sometimes missing ownership checks. #2750

Closed

hughbe reviewed Jan 22, 2020

View reviewed changes

...stem.Windows.Forms/tests/UnitTests/System/Windows/Forms/TabControl.TabPageCollectionTests.cs Outdated Show resolved Hide resolved

weltkante commented Jan 22, 2020

View reviewed changes

weltkante force-pushed the GenericCollections branch from c2343d0 to 3e827a3 Compare January 26, 2020 20:48

weltkante commented Jan 26, 2020

View reviewed changes

hughbe mentioned this pull request Feb 10, 2020

Enable nullability for ScrollBar/ScrollProperties #2823

Merged

WIP

df05a6d

weltkante force-pushed the GenericCollections branch from 3e827a3 to df05a6d Compare March 20, 2020 20:57

ghost assigned weltkante Mar 20, 2020

This was referenced Mar 20, 2020

WIP: implement generic collections without signature changes #2999

Closed

Proposal: implement generic IList<T> on collection types #2644

Open

weltkante closed this Jun 2, 2020

ghost added the draft draft PR label Sep 11, 2021

ghost locked as resolved and limited conversation to collaborators Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: implement generic IList interface #2749

WIP: implement generic IList interface #2749

weltkante commented Jan 21, 2020 •

edited

Loading

codecov bot commented Jan 21, 2020 •

edited

Loading

weltkante Jan 22, 2020

RussKie Jan 22, 2020

weltkante left a comment

weltkante Jan 26, 2020

weltkante Jan 26, 2020

weltkante Jan 26, 2020 •

edited

Loading

weltkante Jan 26, 2020

weltkante Jan 26, 2020

weltkante commented Jun 2, 2020

	System.Windows.Forms.ArraySubsetEnumerator
	System.Windows.Forms.ArraySubsetEnumerator.ArraySubsetEnumerator(object[] array, int count) -> void
	System.Windows.Forms.ArraySubsetEnumerator.Current.get -> object
	System.Windows.Forms.ArraySubsetEnumerator.MoveNext() -> bool
	System.Windows.Forms.ArraySubsetEnumerator.Reset() -> void

WIP: implement generic IList interface #2749

WIP: implement generic IList interface #2749

Conversation

weltkante commented Jan 21, 2020 • edited Loading

Microsoft Reviewers: Open in CodeFlow

codecov bot commented Jan 21, 2020 • edited Loading

Codecov Report

weltkante Jan 22, 2020

Choose a reason for hiding this comment

RussKie Jan 22, 2020

Choose a reason for hiding this comment

weltkante left a comment

Choose a reason for hiding this comment

weltkante Jan 26, 2020

Choose a reason for hiding this comment

weltkante Jan 26, 2020

Choose a reason for hiding this comment

weltkante Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

weltkante Jan 26, 2020

Choose a reason for hiding this comment

weltkante Jan 26, 2020

Choose a reason for hiding this comment

weltkante commented Jun 2, 2020

weltkante commented Jan 21, 2020 •

edited

Loading

codecov bot commented Jan 21, 2020 •

edited

Loading

weltkante Jan 26, 2020 •

edited

Loading