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

Remove IAsyncEnumerable from DbSet #24041

Closed
roji opened this issue Feb 1, 2021 · 3 comments · Fixed by #24145
Closed

Remove IAsyncEnumerable from DbSet #24041

roji opened this issue Feb 1, 2021 · 3 comments · Fixed by #24145
Assignees
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Feb 1, 2021

As #18124 describes, when a project references System.Linq.Async, LINQ operators directly on DbSet don't compile since they are ambiguous between IQueryable and IAsyncEnumerable. Our fix for this up to now has been to introduce DbSet.AsQueryable, which casts DbSet to IQueryable, thereby resolving the ambiguity.

This issue is particularly problematic for the new-user experience to EF Core; the compiler simply complains about the ambiguity, and the user has to search and discover about AsQueryable themselves; it's a very non-polished experience. The comments and reactions in #18124 also seem to indicate that this issue is worth addressing.

I've done a bit of research, here are some options we have.

1. Remove IAsyncEnumerable from DbSet

DbSet was originally made to implement IAsyncEnumerable so that it would be possible to async-enumerate it with the await foreach construct. C# 9 has changed foreach to pattern match, so any type exposing GetAsyncEnumerator (or where an extension method is defined) can now be enumerated. This allows us to remove the implementation of IAsyncEnumerable while keeping enumerability.

Unless I'm mistaken, user code should generally continue working as-is. Users will have issues where they specifically cast DbSet to IAsyncEnumerable, or if they've defined their own extension methods over IAsyncEnumerable and applied them to DbSet directly. So while this is a breaking change, the actual impact may be quite small - and there's always to option of using AsAsyncEnumerable to get an IAsyncEnumerable.

(Since EF Core 6.0 is planned to target .NET 5.0 (at least), and C# 9 is the .NET 5.0 default, that seems to work from a compatibility standpoint)

2. Add an analyzer with a code fix

We should be able to easily identify this scenario in a Roslyn analyzer, and at least provide a code fix to add AsQueryable. This bare minimum addresses the discoverability issue, which is the most problematic part IMHO. The new user experience is still pretty bad IMHO, and the final code is uglier than it should be.

Tracked by #22315.

3. Make IQueryable extend IAsyncEnumerable

IQueryable extends IEnumerable, which is why we don't have this ambiguity problem between these two types; in the past, we've discussed making IQueryable extend IAsyncEnumerable as well, and make the default interface implementation of GetAsyncEnumerator throw.

Aside from the general difficulty of pushing such a change in the BCL, there already exists a package for doing async IQueryable in dotnet/reactive: System.Linq.Async.Queryable. The design there went in a different direction than EF Core's: instead of an agnostic IQueryable which can be executed either synchronously or asynchronously, a separate IAsyncQueryable was introduced, extending IAsyncEnumerable (so two completely detached, parallel type systems). As far as I can tell, this was done to also support async lambda operators (e.g. WhereAwait), which EF Core doesn't support (though it may be reasonable to add such operators directly on IQueryable rather than having a separate IAsyncQueryable). One consequence of this decision, is that it's impractical for a single query root type to implement both IQueryable and IAsyncQueryable, since all operators would be ambiguous.

Regardless of the pros and cons of either design, making IQueryable extend IAsyncEnumerable follows the EF Core model, and goes against the model in System.Linq.Async.Queryable; we're unlikely to be able to push this through successfully.

@roji roji changed the title Resolve ambiguity with System.Linq.Async Remove IAsyncEnumerable from DbSet Feb 12, 2021
@roji roji self-assigned this Feb 12, 2021
@roji roji added this to the 6.0.0 milestone Feb 12, 2021
@roji
Copy link
Member Author

roji commented Feb 12, 2021

Design decision: we are going to remove IAsyncEnumerable from DbSet. Make sure all known usage scenarios continue working without changes (ToListAsync, foreach etc.)

@ajcvickers
Copy link
Member

@roji Make sure to track this as a breaking change--e.g. open a docs issue.

@drdamour
Copy link

+1 for overcoming an ugly problem with competing priorities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants