Implement async-iterator methods#28218
Conversation
f00b1a1 to
3dd307a
Compare
|
@OmarTawfik @dotnet/roslyn-compiler for review. |
There was a problem hiding this comment.
Here's the decompiled code for this scenario (generated via verifier.Dump()), with a few comments added:
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.ExceptionServices;
using System.Threading.Tasks;
using System.Threading.Tasks.Sources;
[assembly: CompilationRelaxations(8)]
[assembly: RuntimeCompatibility(WrapNonExceptionThrows = true)]
[assembly: Debuggable(DebuggableAttribute.DebuggingModes.Default | DebuggableAttribute.DebuggingModes.DisableOptimizations | DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints | DebuggableAttribute.DebuggingModes.EnableEditAndContinue)]
[assembly: AssemblyVersion("0.0.0.0")]
internal class C
{
// This contains the lowered code for method `M()`
[CompilerGenerated]
private sealed class <M>d__0 : IAsyncEnumerable<int>, IAsyncEnumerator<int>, IAsyncDisposable, IValueTaskSource<bool>, IStrongBox<ManualResetValueTaskSourceLogic<bool>>, IAsyncStateMachine
{
public int <>1__state;
public AsyncVoidMethodBuilder <>t__builder; // This type does some un-necessary allocation of a Task (not needed for async-iterators). We may modify the BCL to optimize this
private TaskAwaiter <>u__1;
private int <>2__current;
public ManualResetValueTaskSourceLogic<bool> <>v__promiseOfValueOrEnd;
public bool <>w__promiseIsActive;
ManualResetValueTaskSourceLogic<bool> IStrongBox<ManualResetValueTaskSourceLogic<bool>>.Value
{
[DebuggerHidden]
get { return ref <>v__promiseOfValueOrEnd; }
}
private void MoveNext()
{
int num = <>1__state;
try
{
TaskAwaiter awaiter;
int <>1__state2;
switch (num)
{
default:
Console.Write("1 ");
awaiter = Task.CompletedTask.GetAwaiter();
if (!awaiter.IsCompleted)
{
num = (<>1__state = 0);
<>u__1 = awaiter;
// this is the handshake change to activate and reset the promise when starting background execution
if (!<>w__promiseIsActive)
{
<>w__promiseIsActive = true;
<>v__promiseOfValueOrEnd.Reset();
}
<M>d__0 <M>d__ = this;
<>t__builder.AwaitUnsafeOnCompleted(ref awaiter, ref <M>d__);
break;
}
goto IL_0097;
case 0:
awaiter = <>u__1;
<>u__1 = default(TaskAwaiter);
num = (<>1__state = -1);
goto IL_0097;
case 1:
{
Console.Write(" 4 ");
// reached the end
if (!<>w__promiseIsActive)
{
<>w__promiseIsActive = true;
<>v__promiseOfValueOrEnd.Reset();
}
<>v__promiseOfValueOrEnd.SetResult(false);
break;
}
IL_0097:
awaiter.GetResult();
Console.Write("2 ");
// reached `yield return 3;`
<>2__current = 3;
<>1__state2 = <>1__state;
<>1__state = 1;
if (<>w__promiseIsActive)
{
<>v__promiseOfValueOrEnd.SetResult(true);
}
break;
}
}
catch (Exception exception)
{
<>1__state = -2;
if (<>w__promiseIsActive)
{
<>v__promiseOfValueOrEnd.SetException(exception);
goto end_IL_010b;
}
throw;
end_IL_010b:;
}
}
void IAsyncStateMachine.MoveNext()
{
//ILSpy generated this explicit interface implementation from .override directive in MoveNext
this.MoveNext();
}
[DebuggerHidden]
private void SetStateMachine(IAsyncStateMachine stateMachine) { }
void IAsyncStateMachine.SetStateMachine(IAsyncStateMachine stateMachine)
{
//ILSpy generated this explicit interface implementation from .override directive in SetStateMachine
this.SetStateMachine(stateMachine);
}
[DebuggerHidden]
IAsyncEnumerator<int> IAsyncEnumerable<int>.GetAsyncEnumerator()
{
return this;
}
[DebuggerHidden]
ValueTask<bool> IAsyncEnumerator<int>.WaitForNextAsync()
{
if (<>1__state == -2)
{
return default(ValueTask<bool>);
}
if (!<>w__promiseIsActive || <>1__state == -1)
{
<M>d__0 <M>d__ = this;
<>t__builder.Start(ref <M>d__);
}
return new ValueTask<bool>(this, <>v__promiseOfValueOrEnd.Version);
}
[DebuggerHidden]
int IAsyncEnumerator<int>.TryGetNext(out bool success)
{
if (<>w__promiseIsActive)
{
<>w__promiseIsActive = false;
}
else
{
<M>d__0 <M>d__ = this;
<>t__builder.Start(ref <M>d__);
}
if (!<>w__promiseIsActive && <>1__state != -2)
{
success = true;
return <>2__current;
}
success = false;
return 0;
}
#region Implementation of promise
[DebuggerHidden]
bool IValueTaskSource<bool>.GetResult(short token)
{
return <>v__promiseOfValueOrEnd.GetResult(token);
}
[DebuggerHidden]
ValueTaskSourceStatus IValueTaskSource<bool>.GetStatus(short token)
{
return <>v__promiseOfValueOrEnd.GetStatus(token);
}
[DebuggerHidden]
void IValueTaskSource<bool>.OnCompleted(Action<object> continuation, object state, short token, ValueTaskSourceOnCompletedFlags flags)
{
<>v__promiseOfValueOrEnd.OnCompleted(continuation, state, token, flags);
}
#endregion
[DebuggerHidden]
ValueTask IAsyncDisposable.DisposeAsync()
{
<>v__promiseOfValueOrEnd.Reset();
<>1__state = -1;
return default(ValueTask);
}
}
// This contains the lowered code for method `Main()`
[CompilerGenerated]
private sealed class <Main>d__1 : IAsyncStateMachine
{
public int <>1__state;
public AsyncTaskMethodBuilder <>t__builder;
private IAsyncEnumerator<int> <>s__1;
private object <>s__2;
private int <>s__3;
private int <i>5__4;
private bool <>s__5;
private ValueTaskAwaiter<bool> <>u__1;
private ValueTaskAwaiter <>u__2;
private void MoveNext()
{
int num = <>1__state;
try
{
ValueTaskAwaiter awaiter;
object obj2;
switch (num)
{
default:
Console.Write("0 ");
<>s__1 = M().GetAsyncEnumerator();
<>s__2 = null;
<>s__3 = 0;
goto case 0;
case 0:
{
<Main>d__1 <Main>d__;
try
{
if (num != 0)
{
goto IL_0075;
}
ValueTaskAwaiter<bool> awaiter2 = <>u__1;
<>u__1 = default(ValueTaskAwaiter<bool>);
num = (<>1__state = -1);
goto IL_00d6;
IL_0075:
awaiter2 = <>s__1.WaitForNextAsync().GetAwaiter();
if (!awaiter2.IsCompleted)
{
num = (<>1__state = 0);
<>u__1 = awaiter2;
<Main>d__ = this;
<>t__builder.AwaitUnsafeOnCompleted(ref awaiter2, ref <Main>d__);
return;
}
goto IL_00d6;
IL_00d6:
<>s__5 = awaiter2.GetResult();
if (<>s__5)
{
while (true)
{
<i>5__4 = <>s__1.TryGetNext(out bool flag);
if (flag)
{
Console.Write(<i>5__4);
continue;
}
break;
}
goto IL_0075;
}
}
catch (object obj)
{
obj2 = (<>s__2 = obj);
}
if (<>s__1 == null)
{
break;
}
awaiter = <>s__1.DisposeAsync().GetAwaiter();
if (!awaiter.IsCompleted)
{
num = (<>1__state = 1);
<>u__2 = awaiter;
<Main>d__ = this;
<>t__builder.AwaitUnsafeOnCompleted(ref awaiter, ref <Main>d__);
return;
}
goto IL_0169;
}
case 1:
{
awaiter = <>u__2;
<>u__2 = default(ValueTaskAwaiter);
num = (<>1__state = -1);
goto IL_0169;
}
IL_0169:
awaiter.GetResult();
break;
}
obj2 = <>s__2;
if (obj2 != null)
{
Exception ex = obj2 as Exception;
if (ex == null)
{
throw obj2;
}
ExceptionDispatchInfo.Capture(ex).Throw();
}
int <>s__6 = <>s__3;
<>s__2 = null;
<>s__1 = null;
Console.Write("5");
}
catch (Exception exception)
{
<>1__state = -2;
<>t__builder.SetException(exception);
return;
}
<>1__state = -2;
<>t__builder.SetResult();
}
void IAsyncStateMachine.MoveNext()
{
//ILSpy generated this explicit interface implementation from .override directive in MoveNext
this.MoveNext();
}
[DebuggerHidden]
private void SetStateMachine(IAsyncStateMachine stateMachine)
{
}
void IAsyncStateMachine.SetStateMachine(IAsyncStateMachine stateMachine)
{
//ILSpy generated this explicit interface implementation from .override directive in SetStateMachine
this.SetStateMachine(stateMachine);
}
}
[AsyncStateMachine(typeof(<M>d__0))]
[DebuggerStepThrough]
private static IAsyncEnumerable<int> M()
{
<M>d__0 <M>d__ = new <M>d__0();
<M>d__.<>t__builder = AsyncVoidMethodBuilder.Create();
<M>d__.<>1__state = -1;
AsyncVoidMethodBuilder <>t__builder = <M>d__.<>t__builder;
<M>d__.<>v__promiseOfValueOrEnd = new ManualResetValueTaskSourceLogic<bool>(<M>d__);
<M>d__.<>w__promiseIsActive = true;
return <M>d__;
}
[AsyncStateMachine(typeof(<Main>d__1))]
[DebuggerStepThrough]
private static Task Main()
{
<Main>d__1 <Main>d__ = new <Main>d__1();
<Main>d__.<>t__builder = AsyncTaskMethodBuilder.Create();
<Main>d__.<>1__state = -1;
AsyncTaskMethodBuilder <>t__builder = <Main>d__.<>t__builder;
<>t__builder.Start(ref <Main>d__); // Note that regular async methods start the state machine running in the background (but async-iterator methods don't)
return <Main>d__.<>t__builder.Task;
}
private static void <Main>()
{
Main().GetAwaiter().GetResult();
}
}
// I've removed common code (interfaces, ManualResetValueTaskSourceLogic, IStrongBox)
``` #ResolvedThere was a problem hiding this comment.
case [](start = 20, length = 4)
case [](start = 20, length = 4)
📝 This was causing a crash. Covered by TestControlFlowAnalysis. #Closed
There was a problem hiding this comment.
|
@OmarTawfik for review. Thanks #Resolved |
There was a problem hiding this comment.
SpecialType.System_Boolean [](start = 49, length = 26)
Perhaps not in this PR, but we need to have tests that make sure appropriate errors are reported on all missing types used in these rewriters. #Resolved
There was a problem hiding this comment.
Definitely. Added a work item in #24037 #Resolved
There was a problem hiding this comment.
Conditional expression? #Closed
There was a problem hiding this comment.
unrelated: why is this file unordered (or grouped by feature)? it is very confusing to read/reason about. #Closed
There was a problem hiding this comment.
I agree. Finding the next available letter was not very easy.
I'd re-order it, but I don't really understand the groups (especially what does "Currently not parsed" mean).
I'll leave as-is. #Closed
There was a problem hiding this comment.
"Currently not parsed" means not handled in GeneratedNames.TryParseGeneratedName.
In reply to: 204532707 [](ancestors = 204532707)
There was a problem hiding this comment.
Verification.Passes [](start = 101, length = 19)
this is the default value? (others as well) #Closed
There was a problem hiding this comment.
CompileAndVerify(comp, expectedOutput: "0 1 2 3 4 5 Done", verify: Verification.Passes); [](start = 12, length = 88)
nit: all of these tests could be simplified into CompileAndVerify(source + s_common, expectedOutput: "0 1 2 3 4 5 Done");
This will verify both IL and errors. #Closed
There was a problem hiding this comment.
VerifyDiagnostics also verifies warnings, which seems useful. #Closed
There was a problem hiding this comment.
AsyncIteratorWithAwaitCompletedAndYield [](start = 20, length = 39)
Maybe another example with release output to make sure any optimization changes are expected. #Closed
OmarTawfik
left a comment
There was a problem hiding this comment.
Done with the review.
|
Maybe also rerun the CI? I couldn't see why it was failing. #Resolved |
|
@dotnet-bot test this please #Resolved |
| // If the async method's result type is a type parameter of the method, then the AsyncTaskMethodBuilder<T> | ||
| // needs to use the method's type parameters inside the rewritten method body. All other methods generated | ||
| // during async rewriting are members of the synthesized state machine struct, and use the type parameters | ||
| // structs type parameters. |
There was a problem hiding this comment.
Perhaps type parameters from the struct. #Resolved
| F.Field(F.Local(stateMachineVariable), stateField.AsMember(frameType)), | ||
| F.Literal(StateMachineStates.NotStartedStateMachine))); | ||
|
|
||
| // builder = local.$stateField.builder; |
There was a problem hiding this comment.
local.$stateField.builder [](start = 29, length = 25)
local.$builder? #Resolved
| F.Field(F.Local(stateMachineVariable), _promiseOfValueOrEndField.AsMember(frameType)), | ||
| F.New(mrvtslCtor, F.Local(stateMachineVariable)))); | ||
|
|
||
| // PROTOTYPE(async-streams): Why do we need AsMember? |
There was a problem hiding this comment.
Why do we need AsMember [](start = 45, length = 23)
Is it needed to emit the correct references to _promiseIsActive within the MoveNext for a generic async method? #Resolved
| F.Field(F.Local(stateMachineVariable), stateField.AsMember(frameType)), | ||
| F.Literal(StateMachineStates.NotStartedStateMachine))); | ||
|
|
||
| // builder = local.$stateField.builder; |
There was a problem hiding this comment.
builder [](start = 19, length = 7)
Are we using this local? #Resolved
There was a problem hiding this comment.
Good catch. Thanks
In an async method, we make this local and then use it to start the builder (background process). That's not needed for async-iterator method.
In reply to: 214171546 [](ancestors = 214171546)
| OpenMethodImplementation( IAsyncEnumerableOfElementType_WaitForNextAsync, hasMethodBodyDependency: false); | ||
|
|
||
| BoundFieldAccess promiseIsActiveField = F.Field(F.This(), _promiseIsActiveField); | ||
| LocalSymbol instSymbol = F.SynthesizedLocal(this.stateMachineType); |
There was a problem hiding this comment.
instSymbol [](start = 28, length = 10)
Does inst need to be declared in the outermost block? If not consider hiding it completely in GenerateCallStart(). #Resolved
| // if (this._promiseIsActive) | ||
| // { | ||
| // if (_valueOrEndPromise.GetStatus(_valueOrEndPromise.Version) == ValueTaskSourceStatus.Pending) throw new Exception(); | ||
| // if (State == StateMachineStates.NotStartedStateMachine) throw new Exception("You should call WaitForNextAsync first"); |
There was a problem hiding this comment.
Not included in the code generated below. #Resolved
|
|
||
| private void GenerateIValueTaskSourceImplementation_GetResult() | ||
| { | ||
| // Produce the implementation for `bool IValueTaskSource<bool>GetResult(short token)`: |
There was a problem hiding this comment.
GetResult [](start = 78, length = 9)
.GetResult #Resolved
|
|
||
| private void GenerateIStrongBox_get_Value() | ||
| { | ||
| // Produce the implementation for `ManualResetValueTaskSourceLogic<bool> IStrongBox<ManualResetValueTaskSourceLogic<bool>>.Value { get; }`: |
There was a problem hiding this comment.
ManualResetValueTaskSourceLogic [](start = 51, length = 37)
ref ... #Resolved
| OpenPropertyImplementation(IStrongBoxOfMrvtslOfBool_get_Value); | ||
|
|
||
| // return ref _valueOrEndPromise; | ||
| F.CloseMethod(F.Return(F.Field(F.This(), _promiseOfValueOrEndField))); |
There was a problem hiding this comment.
Return [](start = 32, length = 6)
@cston The trick for returning ref is inside Return:
public BoundReturnStatement Return(BoundExpression expression = null)
{
...
return new BoundReturnStatement(Syntax, CurrentFunction.RefKind /* here */, expression) { WasCompilerGenerated = true };
}
``` #Resolved
docs/features/async-streams.md
Outdated
| { | ||
| throw; | ||
| } | ||
| this.promiseOfValueOrEnd.SetException(ex); |
There was a problem hiding this comment.
this.promiseOfValueOrEnd.SetException(ex); [](start = 4, length = 42)
Remove this line #Resolved
| For async-iterators, we also catch any such exception and pass it on to the caller of the state machine (`WaitForNextAsync` and `TryGetNext`) via the promise of value-or-end: | ||
|
|
||
| ```C# | ||
| catch (Exception ex) |
There was a problem hiding this comment.
@agocke mentioned that throw means we'll lose locals. I'll add a PROTOTYPE comment to change this to:
catch (Exception ex) where { this.state = finishedState; value = promiseIsActive } // pseudo-code for a BoundSequence
{
this.promiseOfValueOrEnd.SetException(ex);
}
``` #Resolved
| } | ||
|
|
||
| /// <summary> | ||
| /// Generates the GetEnumerator method. |
There was a problem hiding this comment.
GetEnumerator [](start = 30, length = 13)
GetAsyncEnumerator #Resolved
|
|
||
| protected override void GenerateMoveNext(SynthesizedImplementationMethod moveNextMethod) | ||
| { | ||
| MethodSymbol setResultMethod = F.WellKnownMethod(WellKnownMember.System_Threading_Tasks_ManualResetValueTaskSourceLogic_T__SetResult, isOptional: true); |
There was a problem hiding this comment.
isOptional: true [](start = 150, length = 16)
Where are we reporting missing methods? #Resolved
There was a problem hiding this comment.
That's tracked by a PROTOTYPE marker. I have that in the following PR.
In reply to: 214999123 [](ancestors = 214999123)
| var model = comp.GetSemanticModel(tree); | ||
| var loop = tree.GetRoot().DescendantNodes().OfType<ForEachStatementSyntax>().Single(); | ||
|
|
||
| var ctrlFlowAnalaysis = model.AnalyzeControlFlow(loop); |
There was a problem hiding this comment.
Analaysis [](start = 24, length = 9)
Typo. #Resolved
| } | ||
|
|
||
| [Fact] | ||
| public void AsyncIteratorWitYieldAndAwait() |
There was a problem hiding this comment.
Wit [](start = 33, length = 3)
Typo #Resolved
| // async System.Collections.Generic.IAsyncEnumerable<int> M() | ||
| Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "M").WithLocation(4, 60) | ||
| ); | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
Consider looping over async enumerable to verify state machine returns one value. #Resolved
There was a problem hiding this comment.
Please add a test for a single yield break; with on preceding await.
In reply to: 215014459 [](ancestors = 215014459)
There was a problem hiding this comment.
The second scenario (just yield break; is covered in AsyncIteratorWithCustomCode):
verify(new[] { AwaitFast, YieldBreak });
verify(new[] { AwaitSlow, YieldBreak });
In reply to: 215016493 [](ancestors = 215016493,215014459)
agocke
left a comment
There was a problem hiding this comment.
LGTM
Some follow-on ideas: I think we should have at least one test that runs using a non-trivial sync context. Also, 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 windows_coreclr_release_prtest please |
|
Note: I skipped all the execution tests on CoreCLR and filed #29735 I'll go ahead and merge with this mitigation. |
|
test windows_debug_unit32_prtest please |
This PR implements lowering of async-iterator methods for main scenarios.
See
async-streams.mdfor design details.Relates to feature dotnet/csharplang#43 and spec https://github.com/dotnet/csharplang/blob/master/proposals/async-streams.md