WIP: implement generic collections without signature changes

[weltkante]

Contributes to #2644
Alternative implementation to PR #2749

This is a WIP PR for exploring the impact #2644 may have and whether its worth taking the breaking change. This PR attempts to make no breaking change to public signatures except implementing additional interfaces.

This will allow LINQ support and other extension methods pick up the member type of the list, but the major downside of not allowing breaking changes is that plain foreach loops will not pick up nullability annotations because they need the return type of GetEnumerator adjusted, which is a breaking change (see PR #2749 for an implementation attempt)

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.
• Existing Remove implementations don't tell the caller whether they removed something, but IList<T> requires this in its API. In some places where collections can be subclassed it is not easily possible to obtain this information, which leads to a bit weird looking implementation. Depending on how much you want to assume about subclass behavior these could be simplified.
• 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

[hughbe]

this will be breaking in terms of parameter validation and exceptions

[weltkante]

Hmm, I didn't write that PR with keeping exception compatibility in mind. I changed the types of private collection members in anticipation of it making nullability easier to implement, and it being slightly more performant (less typecasts in some enumerators, not sure if that applies here). It would be possible to not change any private collection members at expense of additional (small) runtime overhead.

That said, if you compare the ICollection.CopyTo implementation of List<T> between Desktop Framework and .NET Core you can see they already are doing "far worse" breaking changes than what you get by switching from ArrayList to List<T> on Desktop Framework. The .NET Core team should have had plenty of time to address compatibility concerns by now, if they think it is ok to break exception compatibility of basic collection classes then WinForms shouldn't try to keep it (they could arbitrarily break you anytime.)

PS: thats not intended as a generalized statement, just my opinion on this very specific case.

[edit] I see a line in the PR description "this PR attempts to make no breaking changes except [...]" might have been misleading; it was meant to be no breaking changes regarding binary compatibility (i.e. public signature changes). Updated the PR description accordingly.

[RussKie]

this will be breaking in terms of parameter validation and exceptions

Could you please be a little more specific?

[RussKie]

I created a simple benchmark to assess any possible performance gains for this change.

    [MemoryDiagnoser]
    public class FormControlsBenchmarks
    {
        private Form _form;

        [Params(10, 100, 1000)]
        public int N;

        [GlobalSetup]
        public void Setup()
        {
            _form = new Form();

            foreach (int i in Enumerable.Range(1, N))
            {
                var label = new Label { Name = $"label{i:####0}" };
                var textBox = new TextBox { Name = $"textBox{i:####0}" };
                var button = new Button { Name = $"button{i:####0}" };

                _form.Controls.Add(label);
                _form.Controls.Add(textBox);
                _form.Controls.Add(button);
            }
        }

        [GlobalCleanup]
        public void Dispose()
        {
            _form.Dispose();
        }

        [Benchmark(Baseline = true)]
        public string EnumerateControls_GetEnumerator_var()
        {
            string name = null;
            foreach (var c in _form.Controls)
            {
                name = ((Control)c).Name;
            }

            return name;
        }

        [Benchmark]
        public string EnumerateControls_GetEnumerator_explicit()
        {
            string name = null;
            foreach (Control c in _form.Controls)
            {
                name = c.Name;
            }

            return name;
        }
    }

1. I build the latest master and ran the benchmark

Method	N	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
EnumerateControls_GetEnumerator_var	10	937.3 ns	18.81 ns	17.59 ns	1.00	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_explicit	10	928.3 ns	1.97 ns	1.64 ns	0.99	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_var	100	9,544.2 ns	106.24 ns	94.18 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	100	9,766.1 ns	160.55 ns	150.18 ns	1.02	-	-	-	32 B
EnumerateControls_GetEnumerator_var	1000	114,561.5 ns	1,993.27 ns	1,864.50 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	1000	113,059.1 ns	563.64 ns	499.65 ns	0.99	-	-	-	32 B

2. I cherry-picked the changes for Control.ControlCollection and re-ran the benchmark

Method	N	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
EnumerateControls_GetEnumerator_var	10	1,014.0 ns	19.92 ns	38.39 ns	1.00	0.0038	-	-	32 B
EnumerateControls_GetEnumerator_explicit	10	994.2 ns	19.37 ns	28.39 ns	0.98	0.0038	-	-	32 B
EnumerateControls_GetEnumerator_var	100	10,462.0 ns	208.52 ns	376.00 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	100	10,507.8 ns	227.91 ns	348.04 ns	1.01	-	-	-	32 B
EnumerateControls_GetEnumerator_var	1000	120,022.1 ns	2,350.59 ns	3,295.20 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	1000	120,901.6 ns	2,392.99 ns	2,238.40 ns	1.01	-	-	-	32 B

3. I slightly modified the benchmark to cast to IList<Control> explicitly (e.g. foreach (Control c in (IList<Control>)_form.Controls))

Method	N	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
EnumerateControls_GetEnumerator_var	10	821.6 ns	3.28 ns	2.91 ns	1.00	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_explicit	10	811.8 ns	2.85 ns	2.66 ns	0.99	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_var	100	9,112.4 ns	75.40 ns	70.53 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	100	9,007.9 ns	62.89 ns	58.83 ns	0.99	-	-	-	32 B
EnumerateControls_GetEnumerator_var	1000	107,375.1 ns	838.65 ns	784.48 ns	1.00	-	-	-	33 B
EnumerateControls_GetEnumerator_explicit	1000	108,443.4 ns	667.39 ns	624.28 ns	1.01	-	-	-	32 B

The results are nowhere near as impressive as collected in #2644 (comment). In fact going from 1 to 2 saw a slight degradation.
Adding the explicit cast provided slight gains, but they are marginal.

[EDIT] The results are a somewhat unstable and fluctuate, but overall they hover around these figures.

[RussKie]

I have also cherry-picked the Control.ControlCollection changes to from #2749 and re-run the benchmark.
The results haven't changed much:

Method	N	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
EnumerateControls_GetEnumerator_var	10	801.1 ns	1.29 ns	1.21 ns	1.00	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_explicit	10	808.0 ns	2.83 ns	2.51 ns	1.01	0.0048	-	-	32 B
EnumerateControls_GetEnumerator_var	100	9,211.2 ns	57.17 ns	53.47 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	100	9,202.1 ns	27.04 ns	25.30 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_var	1000	107,692.3 ns	596.83 ns	529.07 ns	1.00	-	-	-	32 B
EnumerateControls_GetEnumerator_explicit	1000	107,174.7 ns	769.52 ns	719.81 ns	1.00	-	-	-	32 B

The results are a somewhat unstable and fluctuate, but overall they hover around these figures.

[weltkante]

The major holdback for performance is probably that we cannot change the underlying collection type (the member where the values are stored) for most collection classes because it is exposed as part of the public API and/or inheritance hierarchy prevents specializing into a particular content type. This means we don't get any of the performance advantages because we still call the non-generic API internally.

At this point I don't think performance should be a decision factor, since its clearly not achievable to upgrade. The main point of doing the PR at all is LINQ support, and the yet to be decided upon point (which PR to take) depends on the roadmap for nullability support WinForms wants to provide when its finally turned on by default in VS/Roslyn.

[weltkante]

Closing as the current approach was rejected in favor of compatibility. More detailed commenting on the issue itself.