-
Notifications
You must be signed in to change notification settings - Fork 983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: implement generic IList<T> on collection types #2644
Comments
This is a great suggestion! @terrajobst @danmosemsft @ericstj are there any pitfalls to be mindful of? |
In PR #2646 it was noticed that WinForms exposes non-generic dictionaries via If we don't want to decide on how to treat dictionary-like collections yet (and keep the door open to do the switch later - implementing |
It'll probably cause noticeable change in places that probe for type-specific interfaces before falling back non-generic. At the least this will result in different behavior, it could cause problems if it executes brand new code-paths that aren't vetted. It seems reasonable to do in a new release like net 5, but I'd be cautious about doing it in an inplace update. I don't think we have a precedent to point you at here. We've done similar stuff in desktop and it broke a people but that's an in place update with higher compat bar. |
@ericstj do I take it we have a blessing from you? 😀 |
Any comment on how to treat dictionaries, they are also exposing a collection API (see my previous comment above for more detail)? The risk here is greater because moving to generic dictionaries changes the content type of the list vom On the other hand the 5.0 release is probably the biggest opportunity to do an API breaking change of this kind, letting it pass might mean you'll be unable to use generic dictionaries (which will also have negative effects on your ability to do nullability annotations, non-generic dictionaries cannot be annotated properly) I'll start working on a PR for the non-dictionary collections tomorrow, we'll see if any other issues come up. |
No, you have an opinion from me. I'm not aware of any hard rule about adding interfaces to existing classes. There may be concerns specific to your usage that could complicate things and you'll only discover that through careful design and testing. |
I went ahead and tried implementing
PS: if you think at any point that this not going to meet your 5.0 goals of backwards compatibility, I don't mind if this can't go in, its mainly a thing to explore to see if we can do it. If it doesn't fit the compatibility goal of the project I understand that, having started trying to implement the generic interfaces I see there are lots of trade offs to decide on. |
You cannot change the return type. That's a binary breaking change. Consider implementing interface explicitly to avoid breaking change. |
Thats exactly the thing I want discussed and decided upon by the responsible owners, writing the code for the PR there are too many options with non-obvious choises and trade-offs. At this point I only see it as a way to explore the options and consequences, so the strategy for the PR so far is to do a minimum-diff implementation. Third party frameworks often take breaking changes between major versions in order to modernize the framework (possibly changing assembly names e.g. by numbering them to avoid versioning issues). There were plenty breaking changes in WinForms going from Desktop to 3.0/3.1 so I think the question is which breaking changes are possible and worth taking for 5.0. If the answer is that the move from Desktop to Core was special in that it is equivalent to an assembly rename and at this point there must be no more breaking changes at all until the next time assemblies are renamed (if ever) then I'll rework accordingly. As far as return type changes are concerned, correct me if I'm mistaken but I think foreach loops take the public |
Having talked to @coolcsh, I believe we can do breaking changes in major versions, if we deem those beneficial (i.e. gains outweigh pains). This doesn't necessarily mean we should break things left, right and center though. |
@weltkante could you provide some examples of how user code would benefit from these changes? |
I've updated the OP with a breakdown of breaking changes vs. benefits. I've restricted myself to listing breaking changes with substantial benefits which cannot be achieved without workarounds, otherwise the list would probably be at least twice as long. Thanks for making me think more about the trade-offs, having written this I currently think the conservative sweet spot is just changing the signature of The other benefits are things you can get if you want to take more breaking changes, but they mostly affect the WinForms codebase and could be worked around internally. If you favor binary compatibility highly you can omit them at the cost of more workarounds within the WinForms codebase. Unless other suggestions come up tomorrow I'll rework the PR to explore said implementation where just |
Turns out one central collection, the |
#2921 takes effort to preserve binary compatibility to 3.x in order to allow roll-forward. Note that this conflicts with adding foreach/nullability support to collections in WinForms. (See comment over there for details.) |
I just want to post my opinion here as well. While we can take breaking changes in major versions, we need to do it judiciously. With regard to this particular change, it would cause two issues:
Of course, if the breaking change provides tons of value, it can definitely outweigh those concerns. 😄 That said, it's worth exploring ideas that provides similar productivity via non-breaking APIs with less upheaval of the platform, tooling and ecosystem. |
To be honest, I'm not entirely convinced that this change improves productivity by much. If it were non-breaking, I'd probably take it. But as @DustinCampbell said, breaking changes need to be worthwhile. I'm not sure this demonstrably improves the developer experience. But I'm not an expert, so I could be wrong. |
@terrajobst its mostly impacting the ability for nullability annotations, as long as you don't take a breaking change for GetEnumerator return types WinForms won't be able to annotate its collections. I'd say this will negatively impact productivity once nullability becomes the standard, everyone will have to work around it. If it weren't for nullability and it only affected At the end of the day its a strategic decision and the sooner its done the better. |
As much as I would LOVE to take this proposal, along with nullability annotations, I agree with the above comments about it being too breaking.
@weltkante can you elaborate? |
@merriemcgaw non-generic collections cannot be nullability annotated because they are hardwired to return "nullable object", even though many collections in WinForms don't allow null. This means every foreach loop over a WinForms collection requires workarounds because they report content may be null when it really can't be. Fixing this requires a nullability annotated These foreach loops already cause a lot of friction within the WinForms codebase when trying to turn on nullability, and I don't think it will be any different for outside consumers. Even though its not noticeable now, when the C# language turns on nullability by default WinForms will cause a lot of trouble. I understand if you decide for compatibility rather than foreach/nullability support. That doesn't mean you can't take the proposal though, you can still implement the interfaces and get LINQ support into the collections. Just foreach/nullability will not be supported without changing GetEnumerator return types. Once its decided which path to take I'll update the PR accordingly. |
The team will discuss this early next week, so we won't keep you hanging terribly long 😄 |
@weltkante Would it be possible to get a proof of the concept above? The concern about back-compat is one that I'm not likely to be able to convince the powers that be to make such a breaking change. But being able to show the use case you mention above makes it more palatable. |
Sure, I'll prepare a second PR so it can be compared more easily than just updating the existing PR. |
Thanks so much! We'll take a look. |
I've written some simple benchmarks to check any perceived benefit of going from [MemoryDiagnoser]
public class IEnumerableBenchmarks
{
private UntypedCollection<int> _untypedCollection;
private TypedCollection<int> _typedCollection;
[Params(10, 100, 1000, 100_000)]
public int N;
[GlobalSetup]
public void Setup()
{
var list = Enumerable.Range(1, N).ToArray();
_untypedCollection = new UntypedCollection<int>(list);
_typedCollection = new TypedCollection<int>(list);
}
[Benchmark(Baseline = true)]
public int untyped_GetEnumerator_var()
{
var sum = 0;
foreach (var item in _untypedCollection)
{
sum += (int)item;
}
return sum;
}
[Benchmark]
public int untyped_GetEnumerator_explicit()
{
var sum = 0;
foreach (int item in _untypedCollection)
{
sum += item;
}
return sum;
}
[Benchmark]
public int typed_GetEnumerator_var()
{
var sum = 0;
foreach (var item in _typedCollection)
{
sum += item;
}
return sum;
}
[Benchmark]
public int typed_GetEnumeratorOfT_explicit()
{
var sum = 0;
foreach (int item in _typedCollection)
{
sum += item;
}
return sum;
}
private class UntypedCollection<T> : IEnumerable
{
public UntypedCollection(IList list)
{
List = list;
}
private IList List { get; }
public int Property { get; set; }
IEnumerator IEnumerable.GetEnumerator() => List?.GetEnumerator();
}
private class TypedCollection<T> : IEnumerable<T>
{
public TypedCollection(IList<T> list)
{
List = list;
}
private IList<T> List { get; }
IEnumerator IEnumerable.GetEnumerator() => throw new NotSupportedException(); // ((IEnumerable)List)?.GetEnumerator();
IEnumerator<T> IEnumerable<T>.GetEnumerator() => ((IEnumerable<T>)List)?.GetEnumerator();
}
} The gains appear substantial, time is a in a order of magnitude, memory off the scales:
I trimmed the results to 10 and 100 items, that likely be our major customer use-cases, e.g. tens or hundreds of controls on a Form, rather than thousands. The full results are here: Benchmarks.IEnumerableBenchmarks-report-github.md.txt |
Thanks for checking the numbers, unfortunately I don't think they are achievable. Collection types are designed for .NET 1.0 era API and cannot be upgraded without replacing the inheritance hierarchy entirely (base classes are non-generic and not specific on their content type, also often exposing the underlying non-generic collection to user code). The best we can do is implementing the generic interfaces, not make the collections themselves generic (which is what you measured). The main goal is getting LINQ support by implementing the generic interfaces (which is mostly harmless regarding compatibility, as far as I understand). The yet to be decided point is how to handle nullability support in WinForms once VS/Roslyn switch in on by default. If WinForms wants to support nullability on its collection types properly (i.e. without workarounds necessary in call sites) it needs to do a binary breaking change at some point. |
I am not sure, but I think it would be totally non-breaking to implement additional interfaces. I mean nobody is asking to take away non-generic IEnumerable, ICollection or IList. All we have to do is implement additional Of course certain things have to remain non-generic. For e.g. ListBox.Items have to be non-generic (or should implement |
@nawfalhasan I've done a prototype and it doesn't work out. While adding interfaces is not binary breaking by itself you quickly run into complications because of the design and usage of base classes, which make it impossible to implement the interfaces correctly and efficiently. Lessons learned from the prototype why it doesn't work adding the generic interface:
The current idea is to attempt build a secondary set of generic collection properties which live in parallel to the nongeneric ones, so those can be started to obsolete at some point. However that has its own set of complexities keeping the collections in sync and I don't think it will make it for 5.0 either. |
@weltkante makes sense. Let me start by saying this feature request should be least of priorities of WinForms team as it isnt a significant benefit. But let me address each of the points:
Composition approach works too. But I kind of dislike the redundant nature of APIs. Bit confusing. Maybe the ship has sailed, lets leave it this here |
I have inspected and updated each subclass, its a mess due to the other points but doable, this point was about 3rd party subclasses outside the WinForms codebase.
The base class is used to provide most of the implementation of the collection API. Since its non-generic its implementation and virtual/abstract API is independent of what you actually store in the collection. Making the subclass collections generic means the superclass collection implementation no longer can be used as base of the collection implementation. You probably need to see it in source or try to implement the interface yourself to understand the full impact on why it makes things complicated. For example if non-generic
Thats what I proposed in my first prototype PR (#2749) but it has been rejected for 5.0, the decision is to be able to run your average 3.1 binaries on 5.0 runtime without recompiling. There is a second PR #2999 which doesn't make the generic GetEnumerator public but it at this point the usefulness of the PR is diminished so much that they decided its not worth take the increase of complexity. The team first wants to explore alternate solutions of implementing a second set of independent collections, so the API design doesn't need to be compromised, but this means for 5.0 there will probably be no resolution to this issue.
Not sure what you mean with composition. The ship (probably) sailed for the .NET 5.0 release since the decision on binary compatibility was made and any alternative solution will need quite some time to prototype and explore. My own prototyping (not made public yet) doesn't make me confident that I can pull off something mergeable since maintaining two collection instances is beyond the complexity I'm comfortable with implementing. It seems doable but I don't have the time to pull it off. |
Fwiw there is a precendent for doing something like that: dotnet/runtime#13933 Also even with an explicit interface implementation you can easily do a foreach loop using the generic enumerator by using the foreach (var c in collection.AsEnumerable()) {
// c has the correct type here
} |
@merriemcgaw I don't think anyone is working on this, so tagging as 5.0 is probably wrong. The two prototypes I made turned out to be dead ends as far as compatibility is concerned, and I'm not comfortable with managing the complexity of two distinct collection implementations. (I've yet to share the experiments I made for the latter, I don't have it high on my priorities, but if someone else needs this info of why this is so complex feel free to ping me and I'll try to prioritize writing a summary/explanation of my findings.) |
I'll remove the milestone and move it to future. Bummer that you ran into dead ends. If we get specific queries about it I'll be sure to reach out so that you can write up what you found, but there's no pressing reason at the moment. Thanks so much for evaluating this! |
@RussKie Would the team be against PRs changing non-public |
I believe, such change is quite welcome :) |
Historically WinForms existed before generic collections, and when generics were introduced the collection types in WinForms never were updated to implement the generic
IList<T>
interface. I think its time to revisit all collection types and implementIList<T>
for an appropriate typeT
.For maximum compatibility the
T
inIList<T>
should match the already existing indexer on the collection type, because binding logic will currently orient itself on the indexer to determine the type of the list, but if you start implementingIList<T>
it will prefer that over the indexer. To avoid changing how collection types behave during binding you should use the indexer type asT
.Some advantages of implementing
IList<T>
in addition toIList
:IList
collections by callingCast<T>()
orOfType<T>()
first but this hides the type of the collection so LINQ cannot make use of e.g.Count
orCopyTo
interface methods.foreach
on an untyped list does implicit typecasts on every element - implementing the genericIList<T>
avoids theseIList<T>
Since it turned out there are many trade-offs when implementing generic collections here is a breakdown of breaking changes vs. their benefits:
Breaking changes grouped by benefit/scenario
Convenience
(1)
var
in foreach loops is not supported:foreach (var child in parent.Controls)
defaults to child beingobject
(same for every other collection type, not just the control collection). You have to write out the collection content type manually in every loop.Fixing this requires implementing generic
IEnumerable<T>
and changing the return type of the publicGetEnumerator
from non-genericIEnumerator
to genericIEnumerator<T>
because foreach picks up the public method in favor of interfaces. I assume this breaks binary compatibility.(2) You can use LINQ without prefixing the collection with a
Cast<T>()
orOfType<T>()
LINQ call. Currently you have to writepanel.Controls.Cast<Control>()
to access the control collection, repeating the content type same as in the foreach loop.Fixing this does not require a breaking change, you can implement
IEnumerable<T>
explicitely without breaking binary compatibility (if I understood correctly adding an interface is not breaking compatibility).Performance
(3) LINQ expressions such as
Last()
orToList()
will iterate over the whole collection since they don't see the non-generic IList. Implementing generic IList will allow more efficient access.Fixing this requires implementing
IList<T>
but it can be implemented explicitely, so theoretically it can be done without breaking binary compatibility.Practically there is a problem where some collection types use virtual methods, theoretically allowing 3rd party subclasses. Explicitely implementing
IList<T>
without requiring an update of subclasses leads to very weird and inefficient implementations ofbool ICollection<T>.Remove(T item)
because the current virtual methods won't tell whether an item was removed.Analyzers
(4) Analyzers are usually only written with generic collections in mind. This actually came up during writing the PR, xunit only has support for generic collections, its analyzers won't pick up on antipatterns if you are using non-generic collections.
For adding analyzer support it probably should be enough implementing
IList<T>
explicitely so it probably has no extra cost and is just a benefit if you decide to implement generic collections at all.Nullability
(5) Nullability checks on
foreach
loops will look at the publicGetEnumerator
, nullability checks for LINQ and other extension methods will look at the interface.Most collections can't contain null, but you have no way to annotate the nullability on non-generic IList. Adding support for nullability to foreach loops requires a breaking change because you need to change the return type of public
GetEnumerator
methods to the generic version.(6) some collection classes expose non-generic backing collections to subclasses. These are holding back the WinForms code base from modernizing itself (including proper use of nullability annotations), since you can't change backing collections to a generic type if they are exposed the way they are currently.
Benefits grouped by amount of breaking change
Benefits are incremental in the order I list them.
explicit
IEnumerable<T>
Implement only
IEnumerable<T>
as explicit interface implementation on collection classes.Cast<T>()
orOfType<T>()
explicit
IList<T>
Implement
IList<T>
but make the implementation not public unless the method already exists with the exact signature. All other methods are implemented as explicit interface implementations.Last()
andToList()
extension methodschanging GetEnumerator signature to a generic type
This is the minimum breaking change you have to take for additional benefits.
var
changing
void Remove()
signature to return booleanFor non-virtual Remove() methods this is optional, you can always do a private or internal Remove() implementation which returns bool, but for virtual Remove() methods this is a major breaking change.
IList<T>.Remove
massively, including better performance - without this change you have to double-check the contents for removalchange backing collection types
Some collections expose their (non-generic) backing collection to subclasses. Changing this is obviously a major breaking change.
❗️ This may impact VS Designer, and this impact will have to be assessed.
The text was updated successfully, but these errors were encountered: