[Umbrella] Work items and test plan for async streams

[jcouv]

Spec: https://github.com/dotnet/csharplang/blob/master/proposals/csharp-8.0/async-streams.md
Championed issue: dotnet/csharplang#43

Notes on cancellation token and [EnumeratorCancellation]: http://blog.monstuff.com/archives/2019/03/async-enumerables-with-cancellation.html

FAQ and known issues

• "missing ManualResetValueTaskSourceLogic":
  The preview1 compiler is looking for that type, but .NET Core 3 preview 1 contains ManualResetValueTaskSourceCore. The solution is to include this code snippet in your program. This will be fixed in preview2.
• enumerator disposed too early (yield return in construct with finally) (see issue Async-Streams: iteration stops early on Core #31268)
• NullableReferenceTypes project property doesn't take full effect in legacy projects yet (Add support for Nullable build setting project-system#4058)

---

Async-iterator methods

• EnC
  • block
  • enable (issue EnC: support async-iterator methods #32266, issue EnC: support using declarations, including asynchronous ones #32589 for await using declarations)
• making cancellation token available to producers (PRs Async-enumerator methods honor the EnumeratorCancellation attribute #35121 and [EnumeratorCancellation] combines tokens #35326, API proposal https://github.com/dotnet/corefx/issues/37012)
• Consider optimizing handling of yield return when directly following another yield return (to reduce cost in ManualResetValueTaskSourceCore.SetResult) (issue Async-streams: Consider optimizing handling of yield directly following yield #31248, needs LDM)
• Factor the implementation of GetStatus and OnCompleted for implementing IValueTaskSource and IValueTaskSource<bool> Async-streams: consider factoring IValueTaskSource implementation methods #31517
• Test goto, continue, break
• Consider optimizing stack consumption Tweak async code gen for reduced exceptional stack consumption #26567
• Improve message when missing async Async-streams: improve diagnostic when missing async keyword #31113
• test async method returning a custom type that derives from IAsyncEnumerable<T> and IAsyncEnumerator<T>.
• BCL design
  • factor some code into a base type? (no)
  • introduce new exception type? (not needed at the moment)
  • Any work needed for mono support? (issue Support for async-streams (C# 8.0) mono/mono#12019, PR Add support for async-streams (C# 8.0). mono/mono#12674, EnumeratorCancellation issue Support for EnumeratorCancellationAttribute (C# 8.0) mono/mono#14454)
• test spilling (maybe need to integrate with Neal's changes in patterns branch)
• Get more test ideas from async method and iterator method tests
• Debugging experience (issue Async-streams: stepping through async-iterator seems wrong #32246, fixed in preview3)
• Consider adding a guard in DisposeAsync(): if state is -1, then throw. PR Add guard to DisposeAsync #31764
• Optimize value returned from MoveNextAsync() if possible (issue Async-streams: Consider optimizing return logic of MoveNextAsync() #31246, PR Async-streams: Optimize return of MoveNextAsync #31645)
• Add optional token to GetAsyncEnumerator API (PR Async-streams: Add CancellationToken to GetAsyncEnumerator #31624)
• Should we add some guards for API mis-use? MoveNextAsync should guard against out-of-order invocation #30109
• Generate token checks (answer: no)
• Dispose on async-iterator with finally issue DisposeAsync of async-iterator method should execute required finally blocks #30260, PR Async-streams: Disposal in async-iterator methods #31527
• Confirm desired behavior for DisposeAsync(). it is ok to dispose before enumerating, and then you start enumerating?
• Consider optimizing disposeModeField away when it isn't needed (answer: the optimization is not worth the loss of regularity)
• Emit AsyncIteratorStateMachineAttribute PR Async-streams: Emit AsyncIteratorStateMachine attribute on async-iterator methods #31553
• Test scenario that uses AwaitOnCompleted instead of AwaitUnsafeOnCompleted
• Adopt new BCL APIs (AsyncIteratorMethodBuilder, ManualResetValueTaskSourceCore, remove IStrongBox<T>) PR Async-streams: use ManualResetValueTaskSourceCore and AsyncIteratorMethodBuilder types #31330
• Returning IAsyncEnumerator<T> Async-streams: Add support for enumerator-returning async-iterator method #31057, PR Async-iterator method can return IAsyncEnumerator<T> #31114
• What do exception stack traces look like? Does the stack trace simplifier need to be updated?
• Confirm threadID design (see https://github.com/dotnet/corefx/issues/3481)
• GetAsyncEnumerator() method should make a new instance in some cases Async-Streams: GetAsyncEnumerator should produce new instances when needed #30275, PR Allow multiple calls to GetAsyncEnumerator #31105
• IOperation and CFG should not crash Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363
• Update IAsyncEnumerable API Update IAsyncEnumerator API #30280
• hand-crafted prototype (https://github.com/jcouv/async-iterators/blob/master/src/Program.cs)
• lowering implementation (Implement async-iterator methods #28218)
• CoreCLR testing issue https://github.com/dotnet/coreclr/issues/20032
• support generic methods
• manual testing in IDE (setting up project references, typing, debugging)

Async using and foreach

• Design and fix IOperation and CFG support Finalize design and implementation of IOperation and CFG for async-streams feature #30362
• test with ref struct iterator (issue Async-streams: Add test for ref struct enumerator #32794)
• pattern-based disposal should not even consider extensions (issue Dispose extension methods should not even be considered in pattern-based disposal #32767)
• add test with pattern-based GetAsyncEnumerator with a Caller attribute on optional parameter (check LINQ behavior)
• Allow extension methods to contribute to pattern-based binding (issue Async-streams: allow pattern-based await foreach to use extension methods #32289)
• Tweak message for ERR_AwaitForEachMissingMemberWrongAsync
• Implement semantic model API similar to GetAwaitExpressionInfo
• Test scripting
• Test subtleties of SatisfiesForEachPattern (special method of looking up Current)
• Test type that is both IEnumerable<T> and IAsyncEnumerable<T>.
• Test type that is both IDisposable and IAsyncDisposable.
• Allow pattern-based disposal in await foreach and await using (issue Async-streams: await foreach doesn't dispose result from ConfigureAwait #32316)
• Async-enumerable returned by ConfigureAwait/WithCancellation not recognized by await foreach (issue ConfiguredAsyncEnumerable in netcoreapp3.0 preview is not acceptable with await foreach (CS8142)? #31609, PR Async-streams: Recognize MoveNextAsync that returns an awaitable #32233)
• Pattern-based lookup should pass no argument for CancellationToken (issue Adjust pattern-based lookup for await foreach #32111, PR Pattern-based 'await foreach' should find parameterless 'GetAsyncEnumerator' and 'MoveNextAsync' #32184)
• Integration with "enhanced using and foreach" feature Test plan for enhanced using and foreach #28588
• Test/block expression trees (N/A because they are statements)
• IOperation and CFG should not crash Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363
• Use await using and await foreach syntax Async-streams: Update syntax to 'await using' and 'await foreach' #30525
• Update IAsyncEnumerable API Update IAsyncEnumerator API #30280
• Do we prefer pattern-based over interface-based? (I assume perf is better)
• Only convert/deconstruct when TryGetNext succeeded Convert/deconstruct in async-foreach only if TryGetNext succeeded #30258
• async using (PR Implement async using statement #23961)
• async foreach (PR Implement async foreach statement #24750)
• Update IAsyncEnumerator.WaitForNextAsync to return a ValueTask<bool>.
• Pattern-based foreach should recognize task-like return from WaitForNextAsync
• Verify debugging and sequence points
• Handle foreach await (ref x in ...) ... (error, TestWithPattern_Ref)
• Test with missing well-known members, if not already covered
• Pattern-based async-foreach should recognized WaitForNextAsync that returns a task-like.

Productivity (code fixers/refactorings/etc):

• ExtractMethod doesn't work on parts of or entire async-iterator method body (that's expected)
• FindAllReferences (moved all FAR issues into FindAllReferences should handle Dispose referenced in using and foreach statements #28228)
  • it should find implicit references to GetAwaiter from an async-foreach or async-dispose.
  • it should find implicit references to DisposeAsync from an async-foreach or async-dispose.
• Verify FAR on AsyncDispose method
• decompilation feature in IDE (we'll let ILSpy library fix this)
• Verify proper warnings if any involved member is obsolete (implicit conversions and such)
• Code fixers:
  • fix foreach (by adding/removing await) depending on the collection type. (PR Add MakeStatementAsynchronous fixer #33463)
  • fix using (by adding/removing await) depending on resource type. (PR Add MakeStatementAsynchronous fixer #33463)
  • fix method declaration depending on presence/absence of yield or await (MakeMethodAsync PR MakeMethodAsync: fix iterator methods #31846)
  • using foreach await in non-async method should offer to convert the method to async Adding some IDE code fix tests for async using & foreach #26632
• IDE logic probably needs to be adjusted for completion inside await using and await foreach. For instance, see IsLocalVariableDeclarationContext.
• Review TypeInferrer Fixing bugs in C# TypeInferrer #30211 (comment) (PR Fixing inferred type for yield return in async iterator methods #31046)
• parsing (PR Parsing for using and foreach await #23866)
• IDE completion and colorizing (Fixing completion in async foreach/using #23960)
• Verify highlighting for async and await (see Fixing async keyword highlighting on local functions #25037, PR Adding await keyword highlighting on async using & foreach #25056) (verified manually on method and local function)
• Test with ConvertForeachToFor refactoring (verified manually, not triggered)

LDM open issues:

• async LINQ
  • Parsing issue with return from e in async-collection select await e + 1; // await isn't a keyword
  • how does above scenario and return from e in async-collection select e + 1; lower into LINQ APIs? (the one with await involves Task<int> and the other one involves int directly) How many overloads of Select do we need?
• Should there be some helper method to convert from IEnumerable to IAsyncEnumerable, or from Task to IAsyncEnumerable?
• Should the generate token checks be in GetAsyncEnumerator or in MoveNextAsync? (if we do, we need to store the token and maybe dispose it too) (answer: no)
• Revisit blocking off a word (either in parameter list, like params, or for variable name, like value) for token (answer: we're not going to use a keyword)
• pattern-based await using should recognize a DisposeAsync method that returns a task-like (or only ValueTask)? (not applicable because no ref structs in async methods)
• Should pattern-based foreach recognize ... GetAsyncEnumerator() (without CancellationToken)? (yes, LDM 1/9)
• What attributes should be emitted on async-iterator state machines? (answer: AsyncIteratorStateMachineAttribute)
• cancellation of async-streams (both as consumer and producer) (see LDM 11/28)
• confirm syntax for async foreach
• Should we disallow struct async enumerator? (no, same as regular async, see TestWithPattern_WithStruct_MoveNextAsyncReturnsTask)
• Should DisposeAsync return a non-generic ValueTask, since there is now one? Or stick with Task?
• Extension methods for GetAsyncEnumerator, WaitForNextAsync, and TryGetNext do not contribute. This mirrors behavior for regular foreach. But I'd like to confirm. (answer: this is probably something we want to support. Let's queue that for later in the implementation)
• Should the pattern for async enumerators also recognize when a task-like is returned instead of Task<bool>? (answer: yes)
• I think we'll need to block dynamic since there is no async counterpart to the non-generic IEnumerable that would convert dynamic to. (answer: seems ok)
• Do we need async keyword on async iterator methods? I assume yes.
• Since not enumerable lambdas, I assume the same for async-enumerable. (answer: correct. No async iterator lambda)
• async-iterator without yield or await, should warn? (answer: without yield it's not recognized as an iterator, warn when no await)
• I suspect we'll need to declare the loop variable differently: calling TryGetNext first, then checking success and only then dealing with conversions and deconstructions.

Championed issue: dotnet/csharplang#43 (includes LDM notes)

Test ideas for async foreach:

• Verify that async-dispose doesn't have a similar bug with struct resource

Test ideas for async using:

• Look up Lippert's blog on using with struct or generic type T
• Does the pattern or the interface get picked up first? Is it observable? (maybe if the pattern allows task-like returning method)

Test ideas for async iterators:

• From Andy: we should have at least one test that runs using a non-trivial sync context.
• From Andy: would it be useful to emit asserts for invalid/unexpected states for at least a little while? We could do it only for debug codegen.
• Test with yield or await in try/catch/finally
• More tests with exception thrown in async-iterator
• There is a case in GetIteratorElementType with IsDirectlyInIterator that relates to speculation, needs testing
• yield break disallowed in finally and top-level script (see BindYieldBreakStatement); same for yield return (see BindYieldReturnStatement)
• binding for yield return (BindYieldReturnStatement) validates escape rules, needs testing
• test yield in async lambda (still error)
• test with IAsyncEnumerable<dynamic>
• other tests with dynamic?
• test should cover both case with AwaitOnCompleted and AwaitUnsafeOnCompleted
• test async IAsyncEnumerable<int> M() { return TaskLike(); }
• Can we avoid making IAsyncEnumerable<T> special from the start? Making mark it with an attribute like we did for task-like?
• Do some manual validation on debugging scenarios, including with exceptions (thrown after yield and after await).
• Test with one or both or the threadID APIs missing.

---

BCL (Core)

• Add DefaultCancellationAttribute type: https://github.com/dotnet/corefx/issues/37012
• Design IAsyncEnumerable and supporting types to BCL: https://github.com/dotnet/corefx/issues/32640
• Design IAsyncDisposable in BCL: https://github.com/dotnet/corefx/issues/32665
• Design for CancellationToken in IAsyncEnumerable: https://github.com/dotnet/corefx/issues/33338
  PRs Add initial async iterators support to System.Private.CoreLib coreclr#20442 and Expose async iterator-related types corefx#33104
• Stack pretty printing Improve async iterator exception stack traces coreclr#21103
• Add optional token to GetAsyncEnumerator API
• Add extension method WithCancellation(...) to call GetAsyncEnumerator and wrap the result into an IAsyncEnumerable
• AsyncIteratorStateMachineAttribute https://github.com/dotnet/corefx/issues/33770, PR Add AsyncIteratorStateMachineAttribute coreclr#21313 (and using it Factor new AsyncIteratorStateMachineAttribute into TryResolveStateMachineMethod coreclr#21396)
• Add CancellationToken to GetAsyncEnumerator() PR Add CancellationToken parameter to GetAsyncEnumerator coreclr#21397, documentation https://github.com/dotnet/corefx/issues/33338#issuecomment-444728011
• Add WithCancellation design https://github.com/dotnet/corefx/issues/33909
• Add IAsyncDisposable.ConfigureAwait (PRs Implement IAsyncDisposable.ConfigureAwait coreclr#22160 and Expose/test IAsyncDisposable.ConfigureAwait corefx#34783)

BCL (mono)

• Port types to mono (Support for async-streams (C# 8.0) mono/mono#12019)

BCL (package)

• Add EnumeratorCancellationAttribute (PR Add [EnumeratorCancellation] to Microsoft.Bcl.AsyncInterfaces corefx#37719)

---

References:

• ValueTask
• IValueTaskSource

[jcouv]

Notes from chat with Stephen:

The promise should not just be a TaskCompletionSource. It should be possible to make a re-usable ValueTask<bool> and completion machinery, thus avoiding allocating entirely.
See ManualResetValueTaskSource.cs in corefx. It should be based on IValueTaskSource which should ship in .NET Core 2.1 and down-level (.NET Standard 1.1 or maybe 1.0).

WaitForNextAsync (or MoveNextAsync) would return a ValueTask<bool>. ValueTask<bool> can be backed by a bool, a Task<bool>, or an IValueTaskSource<bool>. So to return the ValueTask<bool> wrapping the Unpronounceable, the Unpronounceable would need to implement IValueTaskSource<bool> so that it can be the backing implementation for that ValueTask<bool>.

It is possible that .NET Core 2.2 would introduce some reusable type(s) for abstracting this sort of stuff. Then we may not need to inline all the extra fields and low-level logic.
As long as the compiler-generated code has a way to tell whether there is no promise, then we’re good. That seems to be possible.
We need to discuss this type dependency.

We’ll have a base type, which common machinery (as both our prototypes did). We should discuss how this type will arrive in the compilation (always generate, or generate if not referenced, or always use reference?).
If the API can be referenced, then we need to lock API. This question also affects above API dependency.

For ConfigureAwait, we could have an extension ConfiguredValueTaskAwaitable<bool> ConfigureAwait(this IValueTaskSource), and ConfiguredValueTaskAwaitable<T> is a task-like.
So we’d need the pattern-based async-foreach to recognize WaitForNextAsync methods that return a task-like.
Need to confirm with LDM.

For CancellationToken, we could just treat the token as a regular local, captured and available to use in the user code.
There may have been some pushback on this (Bart, maybe Lucien). Not clear what the problem is (composability) or what the alternative would be (state machine intrinsics?).

For GetAsyncEnumerable(), we can use the same logic as currently used in async unpronounceable type.
But should read this thread (https://github.com/dotnet/corefx/issues/3481) and confirm with Vlad.
The thread check is meant to emulate an interlocked exchange. We should confirm this is indeed better than interlocked.

We discussed trade-off between WaitForNextAsync/TryGetNext vs. MoveNextAsync/Current patterns.
It just comes down to perf (number of interface calls) vs. pattern complexity (for implementing LINQ methods like Group, etc that aren’t straight foreach loops, for example).