Fix lowering for async-iterator method with type parameter #30105

jcouv · 2018-09-22T21:40:33Z

Fixes mismatch between T from the original method C.M<T>() and its lowered form unpronounceable<T>.MoveNext().
Addresses various minor PROTOTYPE comments.

jcouv · 2018-09-22T21:41:23Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs

@@ -31,7 +33,8 @@ private sealed class AsyncIteratorRewriter : AsyncRewriter
            {
                Debug.Assert(method.IteratorElementType != null);

-                // PROTOTYPE(async-streams): Why does AsyncRewriter have logic to ignore accessibility?


📝 removed the unnecessary _ignoreAccessibility field from AsyncRewriter.

jcouv · 2018-09-22T21:42:10Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs

            }

            /// <summary>
            /// Generates the body of the replacement method, which initializes the state machine. Unlike regular async methods, we won't start it.
            /// </summary>
            protected override BoundStatement GenerateStateMachineCreation(LocalSymbol stateMachineVariable, NamedTypeSymbol frameType)
            {
-                // PROTOTYPE(async-streams): TODO review this (what is this error case at the start?)


📝 Removed this comment, as I don't think this case can be reached with async-iterators. But left the code as I don't think it's harmful.

jcouv · 2018-09-22T21:43:33Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncStateMachine.cs

-                var elementType = asyncMethod.IteratorElementType;
-                //this.ElementType = TypeMap.SubstituteType(elementType).Type; // PROTOTYPE(async-streams): TODO
+                var elementType = TypeMap.SubstituteType(asyncMethod.IteratorElementType).Type;
+                this.ElementType = elementType;


📝 This is the central part of this PR (fixes the async-iterator scenario with a generic type parameter).

jcouv · 2018-09-22T21:44:28Z

src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorRewriter.cs

@@ -174,10 +174,7 @@ protected override void GenerateControlFields()

            // if it is an enumerable, and either Environment.CurrentManagedThreadId or Thread.ManagedThreadId are available
            // add a field: int initialThreadId
-            bool addInitialThreadId =


📝 both changes in this file just refactoring (extract method) in preparation for later PR related to thread ID.

jcouv · 2018-09-23T15:15:15Z

src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs

@@ -2157,7 +2157,6 @@ public override BoundNode VisitForEachStatement(BoundForEachStatement node)
            {
                _pendingBranches.Add(new PendingBranch(node, this.State));
            }
-            //if (_trackExceptions) NotePossibleException(node); // PROTOTYPE(async-streams)


📝 I've created a PR against master to remove _trackExceptions entirely.

jcouv · 2018-09-24T14:38:18Z

@agocke @cston @dotnet/roslyn-compiler for review. This is addressing a number of small follow-up issues (PROTOTYPE markers) in async-streams feature. Thanks

cston · 2018-09-26T17:58:50Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs

@@ -15,6 +15,8 @@ internal partial class AsyncRewriter : StateMachineRewriter
        /// </summary>
        private sealed class AsyncIteratorRewriter : AsyncRewriter
        {
+            private readonly TypeSymbol _elementType;


_elementType [](start = 40, length = 12)

If this value is only used in one location, consider using stateMachineType.ElementType directly there rather than creating a field. #Resolved

cston · 2018-09-26T18:40:20Z

            // (6,33): error CS9001: Async foreach statement cannot operate on variables of type 'C' because 'C' does not contain a public definition for 'GetAsyncEnumerator'

Should the error message say public instance? In this case, there is a public method. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncForeachTests.cs:85 in 28b53a5. [](commit_id = 28b53a5feef3d2a43c541f918ad48bc14200dcbf, deletion_comment = False)

cston · 2018-09-26T18:41:07Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

@@ -7,6 +7,7 @@

 namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
 {
+    using Microsoft.CodeAnalysis.CSharp.Symbols;
    using Roslyn.Test.Utilities;
    using static Instruction;


Please move outside namespace.

cston · 2018-09-26T18:43:53Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+    }
+}";
+            var options = new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)
+                .WithOptimizationLevel(OptimizationLevel.Debug);


TestOptions.DebugDll

cston · 2018-09-26T18:47:16Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

@@ -1075,6 +1138,101 @@ .maxstack  3
            }
        }

+        [ConditionalFact(typeof(WindowsDesktopOnly))]
+        public void AsyncIteratorWithGenericReturn()


AsyncIteratorWithGenericReturn [](start = 20, length = 30)

Consider testing class C<T> { IAsyncEnumerable<T> M() { ... } } as well.

jcouv · 2018-09-28T19:32:08Z

docs/features/async-streams.md

@@ -59,6 +59,9 @@ async IAsyncEnumerable<int> GetValuesFromServer()
 ### Detailed design for async `foreach` statement
 PROTOTYPE(async-streams): TODO

+Async foreach is disallowed on collections of type dynamic, as there is no async equivalent of the non-generic `IEnumerable` interface.
+
+


📝 I'll remove this extra line

jcouv · 2018-10-01T20:17:24Z

@dotnet/roslyn-compiler for a second review, since Andy is out. Thanks

jaredpar · 2018-10-01T23:27:49Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineRewriter.cs

+        /// <summary>
+        /// Returns true if either Thread.ManagedThreadId or Environment.CurrentManagedThreadId are available
+        /// </summary>
+        protected bool CanGetThreadId()


Doesn't this method need to verify that the GetMethod exists for System_Environment__CurrentManagedThreadId? The current invariant guaranteed here doesn't line up with what is needed in MakeCurrenThreadId #Resolved

This method is just a refactoring from IteratorRewriter (extracted for later re-use)

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

jaredpar · 2018-10-01T23:28:57Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -1613,35 +1613,44 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
        {
            base.AddSynthesizedAttributes(moduleBuilder, ref attributes);

-            if (this.IsAsync || this.IsIterator)
+            bool isAsync = this.IsAsync;
+            bool isIterator = this.IsIterator;


Is there a reason that these should be locals? Curious if it's just style or you're protecting against mutations that happen below. #Resolved

I made those locals because they are used multiple times, but there is no fundamental or other reason.

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

jaredpar · 2018-10-01T23:32:41Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs

@@ -101,17 +99,18 @@ protected override void GenerateControlFields()
                    boolType,
                    GeneratedNames.MakeAsyncIteratorPromiseIsActiveFieldName(), isPublic: true);

+                // the element type may contain method type parameters, which are now alpha-renamed into type parameters of the generated class
+                TypeSymbol elementType = ((AsyncStateMachine)stateMachineType).ElementType;


📝 Did we consider parameterizing the base type on the state machine type? #WontFix

You mean to avoid the cast? I could look into that

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

jaredpar · 2018-10-01T23:33:37Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncStateMachine.cs

@@ -16,6 +16,7 @@ internal sealed class AsyncStateMachine : StateMachineTypeSymbol
        private readonly TypeKind _typeKind;
        private readonly MethodSymbol _constructor;
        private readonly ImmutableArray<NamedTypeSymbol> _interfaces;
+        internal readonly TypeSymbol ElementType; // only for async-iterators


IteratorElementType? #Resolved

jaredpar

Extract helper methods for current thread ID

jcouv · 2018-10-02T00:00:38Z

Rebased my changes to resolve conflict.

jcouv added Area-Compilers New Feature - Async Streams Async Streams labels Sep 22, 2018

jcouv added this to the 16.0 milestone Sep 22, 2018

jcouv self-assigned this Sep 22, 2018

jcouv requested a review from a team as a code owner September 22, 2018 21:40

jcouv commented Sep 22, 2018

View reviewed changes

jcouv commented Sep 23, 2018

View reviewed changes

cston reviewed Sep 26, 2018

View reviewed changes

cston approved these changes Sep 26, 2018

View reviewed changes

jcouv force-pushed the async-streams6 branch from d831009 to d64f2d3 Compare September 27, 2018 00:42

jcouv commented Sep 28, 2018

View reviewed changes

jaredpar reviewed Oct 1, 2018

View reviewed changes

jaredpar approved these changes Oct 1, 2018

View reviewed changes

jcouv added 5 commits October 1, 2018 16:53

Fix scenario with type parameter

9e37df9

Extract helper methods for current thread ID

Add tests for parameters and this

b2cbaff

Remove ignore accessibility flag

7c44621

Synthesize proper attributes

2d6fcbd

Update speclet

77dfe41

jcouv and others added 8 commits October 1, 2018 16:54

Convert PROTOTYPE to issue

8d3055a

Test ForEachStatementInfo.Equals and .GetHashCode

9e13cae

Produce error for in async-foreach

0c74cd1

More foreach tests

f62437e

More tests

e402fa2

Block dynamic

c96110d

Address feedback from Chuck

b5447bd

Rename to IteratorElementType

140cf08

jcouv force-pushed the async-streams6 branch from 2ae04b0 to 140cf08 Compare October 2, 2018 00:00

jcouv merged commit 4bf785c into dotnet:features/async-streams Oct 2, 2018

jcouv deleted the async-streams6 branch October 2, 2018 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lowering for async-iterator method with type parameter #30105

Fix lowering for async-iterator method with type parameter #30105

jcouv commented Sep 22, 2018

jcouv Sep 22, 2018

jcouv Sep 22, 2018

jcouv Sep 22, 2018

jcouv Sep 22, 2018

jcouv Sep 23, 2018

jcouv commented Sep 24, 2018

cston Sep 26, 2018 •

edited by jcouv

cston commented Sep 26, 2018 •

edited by jcouv

cston Sep 26, 2018

cston Sep 26, 2018

cston Sep 26, 2018

jcouv Sep 28, 2018

jcouv commented Oct 1, 2018

jaredpar Oct 1, 2018 •

edited by jcouv

jcouv Oct 1, 2018

jaredpar Oct 1, 2018 •

edited by jcouv

jcouv Oct 1, 2018

jaredpar Oct 1, 2018 •

edited by jcouv

jcouv Oct 1, 2018

jaredpar Oct 1, 2018 •

edited by jcouv

jaredpar left a comment

jcouv commented Oct 2, 2018

Fix lowering for async-iterator method with type parameter #30105

Fix lowering for async-iterator method with type parameter #30105

Conversation

jcouv commented Sep 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Sep 24, 2018

cston Sep 26, 2018 • edited by jcouv

Choose a reason for hiding this comment

cston commented Sep 26, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Oct 1, 2018

jaredpar Oct 1, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Oct 1, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Oct 1, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Oct 1, 2018 • edited by jcouv

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

jcouv commented Oct 2, 2018

cston Sep 26, 2018 •

edited by jcouv

cston commented Sep 26, 2018 •

edited by jcouv

jaredpar Oct 1, 2018 •

edited by jcouv

jaredpar Oct 1, 2018 •

edited by jcouv

jaredpar Oct 1, 2018 •

edited by jcouv

jaredpar Oct 1, 2018 •

edited by jcouv