Skip to content

Add System.Linq.AsyncEnumerable #111685

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

Merged
merged 5 commits into from
Jan 23, 2025
Merged

Add System.Linq.AsyncEnumerable #111685

merged 5 commits into from
Jan 23, 2025

Conversation

stephentoub
Copy link
Member

Closes #79782

For the implementation, I've favored clarity/simplicity over lots of optimizations we don't know to be valuable in this context. We can add more specialization in as we see a strong need. The equation for LINQ over IAsyncEnumerable is different for over IEnumerable, where a) we're typically dealing with asynchronous streams of data rather than in-memory, contiguous collections and b) efficient implementations of the interfaces are complicated.

For the tests, I've mainly used the equivalent LINQ functionality as an oracle.

cc: @eiriktsarpalis, @idg10, @HowardvanRooijen

@ghost
Copy link

ghost commented Jan 21, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 21, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 125 out of 140 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • src/installer/pkg/sfx/Microsoft.NETCore.App/PackageOverrides.txt: Language not supported
  • src/libraries/NetCoreAppLibrary.props: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/Directory.Build.props: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/System.Linq.AsyncEnumerable.sln: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/ref/System.Linq.AsyncEnumerable.csproj: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/src/Resources/Strings.resx: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/src/System.Linq.AsyncEnumerable.csproj: Language not supported
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/ContainsAsync.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/Chunk.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/Cast.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/ref/System.Linq.AsyncEnumerable.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/AsyncEnumerable.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/AverageAsync.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/Append.cs: Evaluated as low risk
  • src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/AggregateBy.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/AnyAsync.cs:56

  • The predicate parameter should be checked for null before being used.
Func<TSource, bool> predicate

src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/AnyAsync.cs:93

  • The predicate parameter should be checked for null before being used.
Func<TSource, CancellationToken, ValueTask<bool>> predicate

Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@stephentoub
Copy link
Member Author

@ViktorHofer, is this acceptable?

See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
System.ValueTuple.4.5.0
  Prebuilt usages are different from the baseline found at '/__w/1/s/eng/SourceBuildPrebuiltBaseline.xml'. If it's acceptable to update the baseline, copy the contents of the automatically generated baseline '/__w/1/s/artifacts/sb/prebuilt-report/generated-new-baseline.xml'.

@stephentoub
Copy link
Member Author

@idg10, @HowardvanRooijen, as maintainers of the existing System.Linq.Async, you stated you want to deprecate that and push developers to use an implementation built into .NET. Before we merge this, I just want to triple check that's still your desired outcome 😄 Any feedback before this merges?

@HowardvanRooijen
Copy link

Very much so! It's been great to watch this PR & discussion.

@jkotas
Copy link
Member

jkotas commented Jan 22, 2025

System.ValueTuple.4.5.0
prebuilt usages are different from the baseline

All other places in the repo reference the ValueTuple package for .NET Framework only:

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
<PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" />
</ItemGroup>
. Do the same here?

@stephentoub
Copy link
Member Author

What's being disposed here ? What the dispose code look like ? Does it try to .Wait ?

That's a compiler-generated dispose method for an async iterator. Here's what it looks like in sharplab.

            [DebuggerHidden]
            ValueTask IAsyncDisposable.DisposeAsync()
            {
                if (<>1__state >= -1)
                {
                    throw new NotSupportedException();
                }
                if (<>1__state == -2)
                {
                    return default(ValueTask);
                }
                <>w__disposeMode = true;
                <>v__promiseOfValueOrEnd.Reset();
                <<Prepend>g__Impl|0_0>d<TSource> stateMachine = this;
                <>t__builder.MoveNext(ref stateMachine);
                return new ValueTask(this, <>v__promiseOfValueOrEnd.Version);
            }

It does not do any synchronous blocking.

Or maybe it's some shape of SIMD that we don't have on WASM yet?

There's no use of SIMD in this library.

@stephentoub
Copy link
Member Author

It does not do any synchronous blocking.

DisposeAsync doesn't, but I just pushed a commit that removes three occurrences of calls to an xunit Assert.Equal overload that was doing synchronous blocking while enumerating an async enumerable. As these three occurrences look to correspond to the same methods that were hitting these strange failures, I suspect this commit is going to fix the issue in this PR. If it does, we then need to figure out why the failure mode was resulting in erroneous accesses to the generated iterator.
cc: @jcouv just for visibility

@nietras
Copy link
Contributor

nietras commented Jan 24, 2025

@stephentoub question, why is for example TSource not annotated with allows ref struct when appropriate?, asking cause I have a library with ref struct as TSource.

@stephentoub
Copy link
Member Author

@stephentoub question, why is for example TSource not annotated with allows ref struct when appropriate?, asking cause I have a library with ref struct as TSource.

Can you share the cases you see where it would be appropriate?

@nietras
Copy link
Contributor

nietras commented Jan 24, 2025

My open source csv library Sep, where row or column points to internal state that should not be stored on heap, to avoid programmer errors or similar.

It seems backwards to me to ask for use case, though, why should TSource not be allowed ref struct when no issue with it? Use cases will come, can't predict all.

@stephentoub
Copy link
Member Author

stephentoub commented Jan 24, 2025

I'm asking about which methods in this PR it would be relevant to. It wouldn't work for anything using an async iterator where the value would be yielded or otherwise captured, for example. Are you specifically asking about TSource rather than TResult and in cases where it wouldn't need to be stored, passed through a value task, etc?

@nietras
Copy link
Contributor

nietras commented Jan 24, 2025

Yes with regards to TSource, Count() is an example, Sum(), Select() presumably too. I don't have a complete list. I'd suggest this being added also to sync LINQ, where also issue, afaik.

@stephentoub
Copy link
Member Author

Yes with regards to TSource, Count() is an example, Sum(), Select() presumably too. I don't have a complete list. I'd suggest this being added also to sync LINQ, where also issue, afaik.

Please feel free to open an API proposal for it.

Note that as the code is written, most of those won't actually compile, either. It'd require either rewriting to not use WithCancellation and friends, or updating those as well to also support ref structs.

@timcassell
Copy link

@stephentoub question, why is for example TSource not annotated with allows ref struct when appropriate?, asking cause I have a library with ref struct as TSource.

How? IAsyncEnumerable<T> interface does not allow ref struct. I imagine it's even more difficult to support that for IAsyncEnumerable<T> than it is to support it for IEnumerable<T>, which also does not.

@stephentoub
Copy link
Member Author

@stephentoub question, why is for example TSource not annotated with allows ref struct when appropriate?, asking cause I have a library with ref struct as TSource.

How? IAsyncEnumerable<T> interface does not allow ref struct. I imagine it's even more difficult to support that for IAsyncEnumerable<T> than it is to support it for IEnumerable<T>, which also does not.

They both do in .NET 9.

@timcassell
Copy link

@stephentoub question, why is for example TSource not annotated with allows ref struct when appropriate?, asking cause I have a library with ref struct as TSource.

How? IAsyncEnumerable<T> interface does not allow ref struct. I imagine it's even more difficult to support that for IAsyncEnumerable<T> than it is to support it for IEnumerable<T>, which also does not.

They both do in .NET 9.

Ah, I didn't realize. It's not shown on the documentation. https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.iasyncenumerable-1?view=net-9.0

@nietras
Copy link
Contributor

nietras commented Jan 25, 2025

API proposal #111830

@stephentoub stephentoub added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet and removed needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Feb 13, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Linq breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Integrate Reactive's System.Linq.Async APIs into the Base Class Libraries