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

Implement async-iterator methods #28218

Merged
merged 12 commits into from Sep 9, 2018

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jun 29, 2018

This PR implements lowering of async-iterator methods for main scenarios.
See async-streams.md for design details.

Relates to feature dotnet/csharplang#43 and spec https://github.com/dotnet/csharplang/blob/master/proposals/async-streams.md

@jcouv jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. New Feature - Async Streams Async Streams labels Jun 29, 2018
@jcouv jcouv added this to the 16.0 milestone Jun 29, 2018
@jcouv jcouv self-assigned this Jun 29, 2018
@jcouv jcouv requested a review from a team as a code owner June 29, 2018 17:57
@jcouv jcouv force-pushed the async-streams4 branch 4 times, most recently from f00b1a1 to 3dd307a Compare July 3, 2018 16:18
@jcouv jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 3, 2018
@jcouv jcouv requested a review from a team as a code owner July 3, 2018 16:25
@jcouv
Copy link
Member Author

jcouv commented Jul 3, 2018

@OmarTawfik @dotnet/roslyn-compiler for review.
I will complement this PR with some examples of decompiled C# code for async-iterator methods, as illustration.
Let me know if you have any questions. I'm happy to provide from design information (in person or in document).
Thanks #Resolved

@jcouv jcouv changed the title Implement async iterator methods Implement async-iterator methods Jul 3, 2018
comp.VerifyDiagnostics();
var verifier = CompileAndVerify(comp, expectedOutput: "0 1 2 3 4 5", verify: Verification.Passes);

verifier.VerifyIL("C.M", @"
Copy link
Member Author

@jcouv jcouv Jul 3, 2018

Choose a reason for hiding this comment

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

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)
``` #Resolved

@@ -142,6 +142,7 @@ override protected void LeaveRegion()
case BoundKind.YieldReturnStatement:
case BoundKind.AwaitExpression:
case BoundKind.UsingStatement:
case BoundKind.ForEachStatement when ((BoundForEachStatement)pending.Branch).AwaitOpt != null:
Copy link
Member Author

@jcouv jcouv Jul 3, 2018

Choose a reason for hiding this comment

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

case [](start = 20, length = 4)

case [](start = 20, length = 4)

📝 This was causing a crash. Covered by TestControlFlowAnalysis. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe update the comment explaining why?


In reply to: 199896338 [](ancestors = 199896338)

@jcouv jcouv closed this Jul 5, 2018
@jcouv jcouv reopened this Jul 5, 2018
@jcouv
Copy link
Member Author

jcouv commented Jul 16, 2018

@OmarTawfik for review. Thanks #Resolved

// Add a field: ManualResetValueTaskSourceLogic<bool> promiseOfValueOrEnd
_promiseOfValueOrEndField = F.StateMachineField(
F.WellKnownType(WellKnownType.System_Threading_Tasks_ManualResetValueTaskSourceLogic_T)
.Construct(F.SpecialType(SpecialType.System_Boolean)),
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jcouv jcouv Jul 23, 2018

Choose a reason for hiding this comment

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

Definitely. Added a work item in #24037 #Resolved

var rewriter = new AsyncRewriter(bodyWithAwaitLifted, method, methodOrdinal, stateMachineType, slotAllocatorOpt, compilationState, diagnostics);

AsyncRewriter rewriter;
if (method.IsIterator)
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

Conditional expression? #Closed

@@ -38,6 +38,8 @@ internal enum GeneratedNameKind
AsyncBuilderField = 't',
DynamicCallSiteContainerType = 'o',
DynamicCallSiteField = 'p',
AsyncIteratorPromiseOfValueOrEndBackingField = 'v',
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

unrelated: why is this file unordered (or grouped by feature)? it is very confusing to read/reason about. #Closed

Copy link
Member Author

@jcouv jcouv Jul 23, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

"Currently not parsed" means not handled in GeneratedNames.TryParseGeneratedName.


In reply to: 204532707 [](ancestors = 204532707)

@@ -193,7 +196,7 @@ class C : System.IAsyncDisposable
";
var comp = CreateCompilationWithTasksExtensions(source + s_interfaces, options: TestOptions.DebugExe);
comp.VerifyDiagnostics();
CompileAndVerify(comp, expectedOutput: "C body DisposeAsync1 DisposeAsync2 end", verify: Verification.Skipped);
CompileAndVerify(comp, expectedOutput: "C body DisposeAsync1 DisposeAsync2 end", verify: Verification.Passes);
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

Verification.Passes [](start = 101, length = 19)

this is the default value? (others as well) #Closed

}";
var comp = CreateCompilationWithTasksExtensions(source + s_common, options: TestOptions.DebugExe);
comp.VerifyDiagnostics();
CompileAndVerify(comp, expectedOutput: "0 1 2 3 4 5 Done", verify: Verification.Passes);
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jcouv jcouv Jul 23, 2018

Choose a reason for hiding this comment

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

VerifyDiagnostics also verifies warnings, which seems useful. #Closed

// Test a plain return statement

[Fact]
public void AsyncIteratorWithAwaitCompletedAndYield()
Copy link
Contributor

@OmarTawfik OmarTawfik Jul 20, 2018

Choose a reason for hiding this comment

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

AsyncIteratorWithAwaitCompletedAndYield [](start = 20, length = 39)

Maybe another example with release output to make sure any optimization changes are expected. #Closed

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

Done with the review.

@OmarTawfik
Copy link
Contributor

OmarTawfik commented Jul 20, 2018

Maybe also rerun the CI? I couldn't see why it was failing. #Resolved

@jcouv
Copy link
Member Author

jcouv commented Jul 20, 2018

@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.
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

Perhaps type parameters from the struct. #Resolved

F.Field(F.Local(stateMachineVariable), stateField.AsMember(frameType)),
F.Literal(StateMachineStates.NotStartedStateMachine)));

// builder = local.$stateField.builder;
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

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?
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

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;
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

builder [](start = 19, length = 7)

Are we using this local? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

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");
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

Not included in the code generated below. #Resolved


private void GenerateIValueTaskSourceImplementation_GetResult()
{
// Produce the implementation for `bool IValueTaskSource<bool>GetResult(short token)`:
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

GetResult [](start = 78, length = 9)

.GetResult #Resolved


private void GenerateIStrongBox_get_Value()
{
// Produce the implementation for `ManualResetValueTaskSourceLogic<bool> IStrongBox<ManualResetValueTaskSourceLogic<bool>>.Value { get; }`:
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

ManualResetValueTaskSourceLogic [](start = 51, length = 37)

ref ... #Resolved

OpenPropertyImplementation(IStrongBoxOfMrvtslOfBool_get_Value);

// return ref _valueOrEndPromise;
F.CloseMethod(F.Return(F.Field(F.This(), _promiseOfValueOrEndField)));
Copy link
Member Author

@jcouv jcouv Aug 30, 2018

Choose a reason for hiding this comment

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

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

{
throw;
}
this.promiseOfValueOrEnd.SetException(ex);
Copy link
Member Author

@jcouv jcouv Aug 30, 2018

Choose a reason for hiding this comment

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

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)
Copy link
Member Author

@jcouv jcouv Aug 30, 2018

Choose a reason for hiding this comment

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

@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.
Copy link
Member

@cston cston Aug 30, 2018

Choose a reason for hiding this comment

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

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);
Copy link
Member

@cston cston Sep 4, 2018

Choose a reason for hiding this comment

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

isOptional: true [](start = 150, length = 16)

Where are we reporting missing methods? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

@cston cston Sep 4, 2018

Choose a reason for hiding this comment

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

Analaysis [](start = 24, length = 9)

Typo. #Resolved

}

[Fact]
public void AsyncIteratorWitYieldAndAwait()
Copy link
Member

@cston cston Sep 4, 2018

Choose a reason for hiding this comment

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

Wit [](start = 33, length = 3)

Typo #Resolved

// async System.Collections.Generic.IAsyncEnumerable<int> M()
Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "M").WithLocation(4, 60)
);
}
Copy link
Member

@cston cston Sep 4, 2018

Choose a reason for hiding this comment

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

} [](start = 8, length = 1)

Consider looping over async enumerable to verify state machine returns one value. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for a single yield break; with on preceding await.


In reply to: 215014459 [](ancestors = 215014459)

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

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.

@jcouv jcouv closed this Sep 6, 2018
@jcouv jcouv reopened this Sep 6, 2018
@jcouv
Copy link
Member Author

jcouv commented Sep 6, 2018

test windows_coreclr_release_prtest please

@jcouv
Copy link
Member Author

jcouv commented Sep 9, 2018

Note: I skipped all the execution tests on CoreCLR and filed #29735
The async iterator tests (even those not involving exceptions) get xunit into a hung state. It seems really wacky though, because in the test that Jared and I dug into, I found out that adding a Console.Write changes the outcome (ie. the test passes instead of hangs), despite appearing as an innocuous change in the IL (branch structure is unchanged).

I'll go ahead and merge with this mitigation.

@jcouv
Copy link
Member Author

jcouv commented Sep 9, 2018

test windows_debug_unit32_prtest please

@jcouv jcouv merged commit ee1e200 into dotnet:features/async-streams Sep 9, 2018
@jcouv jcouv deleted the async-streams4 branch September 9, 2018 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants