Skip to content
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

[API Proposal]: allow collection expressions for 'System.Collections.ObjectModel' collection types #110161

Open
Sergio0694 opened this issue Nov 25, 2024 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 25, 2024

Background and motivation

This proposal is about allowing collection expressions to be used to initialize ReadOnlyCollection<T> instances.
There are two main motivations for this:

  1. In lots of scenarios (eg. apps using MVVM), the type of the inner collection simply does not matter. You just want to expose a set of values from your viewmodel, so that the UI can bind to it. It's very common to want to expose them as a readonly type or interface, to enforce that nobody can modify contents directly. In such cases, you will have eg. a property initializer directly creating a ReadOnlyCollection<T> value from some array expression. Being able to use a collection expression would make such scenarios nicer and less verbose.
  2. In UWP/WinUI 3 scenarios using trimming and AOT with CsWinRT, there is a known limitation with collection expressions that make it so that targeting a readonly interface type (ie. IEnumerable<int> e = [1, 2, 3]) results in code that doesn't work at all with AOT. To work around this, ReadOnlyCollection<T> is a very easy and convenient choice: you can simply replace readonly interfaces for public properties in viewmodels with this, and that will make the code perfectly AOT compatible (because CsWinRT will be able to "see" the concrete type being used). Currently though, you cannot use collection expressions with it, meaning when you change properties to use this type, you'll be forced to switch to more verbose syntax with an object creation expression and a nested collection expression. It would be much nicer to just be able to use a collection expression directly.
    While we're at it, we can also do the same for ObservableCollection<T> and Collection<T>.

I'd be happy to contribute this if it gets approved 🙂

Note

Extracted from #108457 (comment)

API Proposal

System.Collections.Generic
{
    public partial class CollectionExtensions
    {
        [EditorBrowsable(EditorBrowsableState.Never)]
        public static Collection<T> CreateCollection<T>(params ReadOnlySpan<T> values);

        [EditorBrowsable(EditorBrowsableState.Never)]
        public static ReadOnlyCollection<T> CreateReadOnlyCollection<T>(params ReadOnlySpan<T> values);

        [EditorBrowsable(EditorBrowsableState.Never)]
        public static ObservableCollection<T> CreateObservableCollection<T>(params ReadOnlySpan<T> values);
    }
}

System.Collections.ObjectModel
{
    [CollectionBuilder(typeof(CollectionExtensions), "CreateCollection")]
    public partial class Collection<T>;

    [CollectionBuilder(typeof(CollectionExtensions), "CreateReadOnlyCollection")]
    public partial class ReadOnlyCollection<T>;

    [CollectionBuilder(typeof(CollectionExtensions), "CreateObservableCollection")]
    public partial class ObservableCollection<T>;
}

Note

The contract of this builder method should make it clear that the returning type is guaranteed to be exactly of each of these types, and not of a derived type. That is necessary to ensure that WinRT marshalling will work correctly, as it needs to "see" the types.

API Usage

public class MyViewModel : ObservableObject
{
    public ReadOnlyCollection<string> Names { get; } =
    [
        "Bob",
        "Alice"
    ];

    [ObservableProperty]
    public partial string? SelectedName { get; set; }
}
<ListView
    ItemsSource="{x:Bind ViewModel.Names}"
    SelectedItem="{x:Bind ViewModel.SelectedName, Mode=TwoWay}" />

Risks

No risk. This enables new syntax that couldn't be used before, and developers using that would not care about the concrete type of the inner list being used. Furthermore, ReadOnlyCollection<T> doesn't expose the inner list anyway, so it should not even matter at all in any case.

@Sergio0694 Sergio0694 added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections labels Nov 25, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 25, 2024
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Nov 25, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 25, 2024
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Nov 25, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 25, 2024
@eiriktsarpalis
Copy link
Member

SGTM 👍

@Sergio0694
Copy link
Contributor Author

@eiriktsarpalis I just realized it would make sense to also do the same for:

  • ObservableCollection<T>
  • Collection<T>

Can I just update this proposal or should I create a new one?

@eiriktsarpalis
Copy link
Member

Feel free to update the current proposal.

@Sergio0694 Sergio0694 changed the title [API Proposal]: allow collection expressions for 'ReadOnlyCollection<T>' [API Proposal]: allow collection expressions for 'System.Collections.ObjectModel' collection types Nov 27, 2024
@kronic
Copy link
Contributor

kronic commented Nov 29, 2024

@Sergio0694 Add ReadOnlyObservableCollection<T>

@Sergio0694
Copy link
Contributor Author

I intentionally didn't add that because it's a wrapper around ObservableCollection<T>, meaning you'd create an ObservableCollection<T> to keep private, and then expose the wrapping ReadOnlyObservableCollection<T> instance, and do changes to the private ObservableCollection<T> instance. If you just created a ReadOnlyObservableCollection<T> instance from a collection expression and that just created an ObservableCollection<T> on the fly that you wouldn't be able to see or interact with, I'm not sure it'd be much use 😅

@terrajobst
Copy link
Member

terrajobst commented Dec 3, 2024

Video

  • We can't put the methods on CollectionExtensions because the object model ones live in a higher assembly
  • We could add another type (e.g. System.Collections.ObjectModel.Collection or one non-generic type per collection (like we do in immutable).
  • Also, we only really need it for read-only collections because anything that supports collection initializers also supports collection expressions.
    • Collection<T> already works
    • ObservableCollection<T> already works
  • We considered asking for constructor support, but object model collections typically wrap the passed-in collection which create weirdness if we add a span-based one that copies.
namespace System.Collections.ObjectModel
{
    [CollectionBuilder(typeof(ReadOnlyCollection), "CreateCollection")]
    public partial class ReadOnlyCollection<T>;

    [CollectionBuilder(typeof(ReadOnlyCollection), "CreateSet")]
    public partial class ReadOnlySet<T>;

    public static class ReadOnlyCollection
    {
        public static ReadOnlyCollection<T> CreateCollection<T>(params ReadOnlySpan<T> values);
        public static ReadOnlySet<T> CreateSet<T>(params ReadOnlySpan<T> values);
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Dec 3, 2024
@eiriktsarpalis
Copy link
Member

@Sergio0694 care to submit a PR?

@Sergio0694
Copy link
Contributor Author

Sure thing, will do! Thank you for bringing this to API review! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Projects
None yet
Development

No branches or pull requests

4 participants