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

Open issue: relax requirement that type be enumerable to participate in collection expressions #7744

Open
jcouv opened this issue Dec 6, 2023 · 14 comments
Assignees
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Dec 6, 2023

It is presently not possible to declare a collection type that doesn't implement some IEnumerable interface.
We should consider allowing types marked with [CollectionBuilder(...)] to not be enumerable (and thus not have an iterator type).

I ran into this issue trying to make a roslyn type (CSharpTestSource) usable with collection expressions. This required a dummy implementation of an IEnumerable interface despite that interface playing little role for compiling this code.

To achieve this, we would remove the requirement from the [CollectionBuilder] section:

The collection type must have an iteration type.

And we would instead use the type from the parameter type of the Create method, ie. E in ReadOnlySpan<E>. All elements of the collection need to an implicit conversion to that type.

Relates to #5354, dotnet/roslyn#66418
FYI @cston @CyrusNajmabadi

Here's the formalized proposal.

Design meetings

https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-01-31.md#relax-enumerable-requirement-for-collection-expressions

@CyrusNajmabadi
Copy link
Member

Not sure how I feel about this.

Why not make it enumerable?

Seems very weird that you found use a collection to create it, but then not be able to iterate it.

@jcouv
Copy link
Member Author

jcouv commented Dec 6, 2023

Here's the roslyn example that prompted this issue: dotnet/roslyn#71134
Having a dummy/unused IEnumerable implementation seems superfluous.

@CyrusNajmabadi
Copy link
Member

I guess the same feelings existed for collection expressions :-). I'll look at the example though!

@CyrusNajmabadi
Copy link
Member

I see. Imo the better fix at the language level is for implicit operators to participate in collection expression target typing.

If you have an implicit operators from array, then this should ideally just work.

@333fred
Copy link
Member

333fred commented Dec 6, 2023

We explicitly (no pun intended) excluded that when we did not make a collection expression conversion a standard implicit conversion.

@CyrusNajmabadi
Copy link
Member

We explicitly (no pun intended) excluded that when we did not make a collection expression conversion a standard implicit conversion.

Yup yup. But I'm curious if we might reconsider given some real world use case.

That said, CSharpSources seems, to me, to indicate a collection type. So not being enumerable is frankly kinda strange. I'll have to look and see how you guys actually end up using this.

@333fred
Copy link
Member

333fred commented Dec 6, 2023

I'll have to look and see how you guys actually end up using this.

We basically always realize it to a flattened array when we actually go to access it.

@CyrusNajmabadi
Copy link
Member

so you have a collection type which is like an array, but it's all wonky on conversions to/from an array. go tit. :)

not sure how i feel about this case. seems very different than basically like everything else out there. note that i view collection-exprs as a common way to create collections. Not a nice syntax to create any arbitrary type. But i am open to this sort of relaxation because... well... it doesn't seem like it would harm anything.

@RikkiGibson
Copy link
Contributor

I thought one intended use of CollectionBuilder was to allow creation of ref structs from collection-exprs. Is that not the case? SharpLab

using System;
using System.Runtime.CompilerServices;

[CollectionBuilder(typeof(RS), nameof(RS.Create))]
public ref struct RS {
    public RS Create(ReadOnlySpan<int> items) => throw null!;
    public static void M() {
        RS rs1 = [1, 2, 3]; // error CS9188: 'RS' has a CollectionBuilderAttribute but no element type.
    }
}

@cston
Copy link
Member

cston commented Dec 7, 2023

I thought one intended use of CollectionBuilder was to allow creation of ref structs from collection-exprs. Is that not the case?

It should be possible to create a ref struct collection with a pattern-based GetEnumerator() method (see sharplab.io).

[CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))]
public ref struct MyCollection<T>
{
    private readonly List<T> _list;
    public MyCollection(List<T> list) { _list = list; }
    public IEnumerator<T> GetEnumerator() => _list.GetEnumerator();
}

@jnm2
Copy link
Contributor

jnm2 commented Dec 7, 2023

There was a community request for this exact issue description at #7398 (reply in thread).

Also, it would enable Memory<T> (which has no GetEnumerator) to add [CollectionBuilder] and be targeted by collection expressions without being special-cased by the compiler.

@timcassell
Copy link

It would also be nice to relax the restriction for old style collection initializer syntax. An example where I wrote a class for a write-only collection initializer (before collection expressions existed): https://github.com/timcassell/ProtoBenchmarkHelpers#BenchmarkThreadHelper

@RikkiGibson
Copy link
Contributor

Old collection initializers had a requirement to implement IEnumerable because without it, it was felt to be unclear if just an Add(T) taking a parameter of some type is intended for adding an element to a collection, or for e.g. performing arithmetic.

I think a type with [CollectionBuilder] not needing to be enumerable is a bit easier of a sell, as adding the attribute to the type very clearly indicates the associated Create method is intended for creating a collection. And folks who are interested in creating "write-only" collections should be able to onboard to it and get the experience they want.

@jcouv jcouv self-assigned this Jan 23, 2024
@gulbanana
Copy link

gulbanana commented Jan 28, 2024

There was a community request for this exact issue description at #7398 (reply in thread).

Yep, I'd like to use CollectionBuilder (or some equivalent api) on a collection representing an ORM query. It's writeonly, as above - the query can be default-constructed and have elements added, but you can't iterate it until you've performed an async load or save.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants