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

Consider supporting collection literals for IAsyncEnumerable<T> #111715

Open
eiriktsarpalis opened this issue Jan 22, 2025 · 29 comments
Open

Consider supporting collection literals for IAsyncEnumerable<T> #111715

eiriktsarpalis opened this issue Jan 22, 2025 · 29 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Collections
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jan 22, 2025

For testing purposes, it'd be reasonable. I don't know how much value it would add beyond that. Maybe as a shorthand for Empty, as you allude to here with your comment placement.

Originally posted by @stephentoub in #111685 (comment)

API Proposal

namespace System.Collections.Generic;

+[CollectionBuilder(typeof(AsyncEnumerableExtensions), nameof(AsyncEnumerableExtensions.Create))]
public interface IAsyncEnumerable<T>;

+public static class AsyncEnumerableExtensions
+{
+    public static IAsyncEnumerable<T> Create(ReadOnlySpan<T> values);
+}
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 22, 2025
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Jan 22, 2025
@stephentoub
Copy link
Member

My preference would be that IAsyncEnumerable<T> is on the same plan as IEnumerable<T>, where the compiler handles the implementation. If we decide we want to leave the compiler out of it, we'd end up with the builder pattern and would need to add an appropriate Create(ReadOnlySpan<T>) method somewhere.

@eiriktsarpalis
Copy link
Member Author

What decides whether a type should be handled by the compiler or the library itself? If because of perf-related reasons, I would guess it's of least concern when it comes to the use cases of this type.

cc @CyrusNajmabadi

@stephentoub
Copy link
Member

stephentoub commented Jan 22, 2025

I've thought of it as either how core the interface is or performance; this would be about the former rather than the latter. But I defer to @CyrusNajmabadi on that.

It would definitely become a compiler concern if it didn't require eager evaluation, e.g. if:

IAsyncEnumerable<string> pages = [await DownloadAsync(page1), await DownloadAsync(page2)];

became the functional equivalent of:

static async IAsyncEnumerable<string> $Impl()
{
    yield return await DownloadAsync(page1);
    yield return await DownloadAsync(page2);
}

that would have to be a compiler-provided implementation. If instead it was always eager and the logical equivalent of:

string[] tmp = [await DownloadAsync(page1), await DownloadAsync(page2)];
IAsyncEnumerable<string> pages = tmp.ToAsyncEnumerable();

then it could be done in library with a builder, at a small additional allocation expense, but not a big deal for these scenarios.

@eiriktsarpalis
Copy link
Member Author

Good point. It also opens up the question of whether the spread operator should be supported for IAEs:

IAsyncEnumerable<int> a, b;
IAsyncEnumerable<int> concatenated = [.. a, .. b];

@eiriktsarpalis eiriktsarpalis removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 22, 2025
@eiriktsarpalis eiriktsarpalis removed this from the 10.0.0 milestone Jan 22, 2025
@eiriktsarpalis eiriktsarpalis added the untriaged New issue has not been triaged by the area owner label Jan 22, 2025
@CyrusNajmabadi
Copy link
Member

The reasons IEnumerable were special cased were:

  1. it's just so core to .Net, having been around since the 2.0 days.
  2. It's prevalent in many APIs.
  3. We didn't want anyone to have to take a dep on a new BCL to find ways to construct it.
  4. We felt we could optimize this space in several cases.
  5. IEnumerable (and a few other interfaces) are already special cased in the language due to how we see arrays.

Based on all of this, we decided to just bake in support for this core type. To me, IAsyncEnumerable is quite different (but i'm open to the discussion), and doesn't meet my internal bar check for warranting all the lang and compiler work to support this. I'd prefer it just come about through a standard [CB] attribute approach as that's low cost and sufficient imo.

@CyrusNajmabadi
Copy link
Member

If instead it was always eager and the logical equivalent of:

Note: our view on collection expressions is that they're always eager. For example, [a, b] is not the equivalent of yield a; yield b; for for IEnumerable. It's much closer to new[] { a, b } (readonly of course).

@stephentoub
Copy link
Member

It also opens up the question of whether the spread operator should be supported for IAEs:

We've tried really hard to ensure that asynchronous waits are visible in the code. I'm not against supporting spreads with IAE, but I would want a syntax that included await as part of it. Having [..a, ..b] silently imply await foreach is, IMHO, dangerous.

To me, IAsyncEnumerable is quite different (but i'm open to the discussion)

Ok. I'm fine then if we want to add a builder for it; we'll need a proposal for what that is. It would need to live in corelib.

@huoyaoyuan
Copy link
Member

Supporting await should be non-starter.
Simpler cases that wrap an eager collection can be useful for mock tests, and ToAsyncEnumerable may not always work fine with type inferrence.

@eiriktsarpalis
Copy link
Member Author

Note: our view on collection expressions is that they're always eager.

I'm not against supporting spreads with IAE, but I would want a syntax that included await as part of it. Having [..a, ..b] silently imply await foreach is, IMHO, dangerous.

Given that collection expressions are understood to always have eager semantics, I agree that it would be inappropriate for the spread operator to accept IAE operands. I'll update the issue with a proposal for a CB attribute.

FWIW there is value in having a language-integrated mechanism for appending IAEs in a deferred manner, but it seems like this should be the purview of iterator methods per the recursive iterator proposal.

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jan 23, 2025
@eiriktsarpalis eiriktsarpalis modified the milestones: 10.0.0, Future Jan 23, 2025
@julealgon
Copy link

@eiriktsarpalis

Given that collection expressions are understood to always have eager semantics, I agree that it would be inappropriate for the spread operator to accept IAE operands.

What is the problem with something like [await .. a, .. b] ? To me that's analogous to the added await in front of a foreach loop that works on IAsyncEnumerable.

I've actually needed this exact case recently and even tried to add the await keyword to see whether it would work, so to me it would be fairly intuitive if it did.

@huoyaoyuan
Copy link
Member

What is the problem with something like [await .. a, .. b] ?

It's not valid in current C#. It needs to be proposed and designed with the language.

@julealgon
Copy link

What is the problem with something like [await .. a, .. b] ?

It's not valid in current C#. It needs to be proposed and designed with the language.

@huoyaoyuan I know that. I was just questioning it as part of this issue here. From the discussions above, it gave me the impression that it was not even an option to have that syntax (as in, a new feature with that syntax)?

It feels super natural to me to have that in the language.

@stephentoub
Copy link
Member

+public static class AsyncEnumerableExtensions
+{

  • public static IAsyncEnumerable Create(ReadOnlySpan values);
    +}

I don't think we want to introduce a new type called AsyncEnumerableExtensions, since these aren't extension methods. Maybe we hold our noses and add a CreateAsyncEnumerable to the existing CollectionExtensions, maybe even marked EditorBrowsable since it's far from an ideal location. Or maybe really bury it, putting it on the somewhat-related-but-still-a-stretch AsyncIteratorMethodBuilder type... at least then it's somewhere we don't expect anyone to ever manually look. Or something else.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jan 24, 2025

Maybe we hold our noses and add a CreateAsyncEnumerable to the existing CollectionExtensions

If we're including the builder with the Microsoft.Bcl.AsyncInterfaces NuGet package wouldn't it make reusing CollectionExtensions impractical?

maybe even marked EditorBrowsable since it's far from an ideal location.

I left it out since in our recent review of collection literals in ReadOnlyCollection<T> we concluded that builder methods should be made visible. I can see why we'd want to hide this one though given it's not creating "canonical" instances.

@bartonjs
Copy link
Member

bartonjs commented Jan 28, 2025

Video

This is easy to do for net10+ only, e.g.

namespace System.Collections.Generic
{
    [CollectionBuilder(typeof(AsyncIteratorMethodBuilder), nameof(AsyncIteratorMethodBuilder.CreateAsyncEnumerable))]
    public interface IAsyncEnumerable<T>
    {
    }
}

namespace System.Runtime.CompilerServices
{
    public partial struct AsyncIteratorMethodBuilder
    {
        public static IAsyncEnumerable<T> CreateAsyncEnumerable<T>(ReadOnlySpan<T> values);
    }
}

but Microsoft.Bcl.AsyncInterfaces typeforwards IAsyncEnumerable and AsyncIteratorMethodBuilder, which means that it's not easily possible to make it work for .NET Standard 2.0 TFM or older .NET Core TFMs. If the downlevel TFMs are important then this needs to be special cased in the compiler (possibly in conjunction with doing this change for net10, or possibly standalone)

@bartonjs bartonjs added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 28, 2025
@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jan 29, 2025

It was brought up during API review that due to the difficulties of getting a CBA-based approach to work with older TFMs, it might be preferable if this got implemented by the compiler. Alternatively this would be .NET 10+ only feature.

cc @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Member

I have no problem with this being a 10.0 feature. This doesn't seem like a major ask. And this doesn't seem like a difficult requirement for people asking for this.

@julealgon
Copy link

@CyrusNajmabadi

I have no problem with this being a 10.0 feature. This doesn't seem like a major ask. And this doesn't seem like a difficult requirement for people asking for this.

It would be pretty jarring seeing this only work for .NET 10+ projects but other collection expressions working everywhere though.

From a consumer/language user perspective, I think it would feel much nicer if this worked consistently across all targets like other collection initializers do already.

@CyrusNajmabadi
Copy link
Member

That's how normal collection expressions work as well. Many won't work if you don't have the release that contains the CBA for it.

@julealgon
Copy link

That's how normal collection expressions work as well. Many won't work if you don't have the release that contains the CBA for it.

I'm not sure I'm following you. I'm using collection expressions on NET472 and netstandard2.0 just fine.

@huoyaoyuan
Copy link
Member

You can't use any collection expressions for types requiring CollectionBuilderAttribute on down level frameworks, including ones mentioned in #87569 #92190 #110161 . IEnumerable<T> and IList<T> are specially treated by the compiler because they are required to be implemented by arrays and List<T>.

@julealgon
Copy link

You can't use any collection expressions for types requiring CollectionBuilderAttribute on down level frameworks, including ones mentioned in #87569 #92190 #110161 . IEnumerable<T> and IList<T> are specially treated by the compiler because they are required to be implemented by arrays and List<T>.

Ahhh I see, thanks for clarifying @huoyaoyuan .

In that case, I am inclined to prefer IAsyncEnumerable also being handled by the compiler directly like IEnumerable does, again for consistency.

@CyrusNajmabadi
Copy link
Member

We special cased IEnumerable because of how enormously widespread it is, going back to literally .net 2.0. even that took a lot of discussion and convincing that that was worth it.

For the vast amount of collections out there that are much more recent and used far less, we designed a general extensibility mechanism to give them a way to work if the API author wants that. I think we'd need a really compelling reason to bring this into the language itself. Consistency doesn't cut it for me.

@julealgon
Copy link

We special cased IEnumerable because of how enormously widespread it is, going back to literally .net 2.0. even that took a lot of discussion and convincing that that was worth it.

For the vast amount of collections out there that are much more recent and used far less, we designed a general extensibility mechanism to give them a way to work if the API author wants that. I think we'd need a really compelling reason to bring this into the language itself. Consistency doesn't cut it for me.

@CyrusNajmabadi all I can say about that is that we use IAsyncEnumerable very extensively throughout our 160+ project solution, and usage will ramp up more and more as we convert more of the codebase to async and adopt System.Linq.Async more. As of right now, this entire solution targets NET472.

Of course I can only speak for myself on this. Consistent behavior with IEnumerable would make a lot of sense in our case.

@CyrusNajmabadi
Copy link
Member

Why can't that library do a revision that includes CBA? Reving a library is so much cheaper than reving the lang/compiler

@huoyaoyuan
Copy link
Member

IAsyncEnumerable<T> is shipped by the nuget package Microsoft.Bcl.AsyncInterfaces on netfx. The package can do a revision to include CBA.

@CyrusNajmabadi
Copy link
Member

IAsyncEnumerable<T> is shipped by the nuget package Microsoft.Bcl.AsyncInterfaces on netfx. The package can do a revision to include CBA.

That seems orders of magnitude simpler and more reasonable than rev'ing the language and compiler to support this. The lang route is at least a full year out, and would require updating your compiler stack. The library revision route presumably can be done just by adding a trivial API and publishing a new package version.

@stephentoub
Copy link
Member

stephentoub commented Feb 1, 2025

IAsyncEnumerable<T> is shipped by the nuget package Microsoft.Bcl.AsyncInterfaces on netfx. The package can do a revision to include CBA.

That seems orders of magnitude simpler and more reasonable than rev'ing the language and compiler to support this. The lang route is at least a full year out, and would require updating your compiler stack. The library revision route presumably can be done just by adding a trivial API and publishing a new package version.

That's not viable. IAsyncEnumerable type forwards to corelib. .NET 8 and .NET 9 wouldn't receive the attribute nor a builder method it could point to. If the language had a CollectionBuilderForAttribute, then we could do it, but it doesn't.

@CyrusNajmabadi
Copy link
Member

If there are enough types doing that, or this is an important enough case, I would prefer doing that. That way we're not just continually fixing things by adding "just one more type" for each release. But we're fixing it with a general solution that we can and should use for all future cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

No branches or pull requests

6 participants