Move reporting of ERR_IteratorMustBeAsync to avoid race condition

src/Compilers/CSharp/Portable/Binder/Binder.cs

@@ -317,13 +317,10 @@ internal virtual GeneratedLabelSymbol ContinueLabel
         /// <summary>
         /// Get the element type of this iterator.
         /// </summary>
-        /// <param name="node">Node to report diagnostics, if any, such as "yield statement cannot be used
-        /// inside a lambda expression"</param>
-        /// <param name="diagnostics">Where to place any diagnostics</param>
         /// <returns>Element type of the current iterator, or an error type.</returns>
-        internal virtual TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        internal virtual TypeWithAnnotations GetIteratorElementType()
         {
-            return Next.GetIteratorElementType(node, diagnostics);
+            return Next.GetIteratorElementType();
         }
 
         /// <summary>

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

@@ -207,19 +207,23 @@ private void CheckRequiredLangVersionForAsyncIteratorMethods(DiagnosticBag diagn
             }
         }
 
-        private BoundStatement BindYieldReturnStatement(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        protected virtual void ValidateYield(YieldStatementSyntax node, DiagnosticBag diagnostics)
         {
-            var binder = this;
+            Next?.ValidateYield(node, diagnostics);
+        }
 
-            TypeSymbol elementType = binder.GetIteratorElementType(node, diagnostics).Type;
+        private BoundStatement BindYieldReturnStatement(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        {
+            ValidateYield(node, diagnostics);
+            TypeSymbol elementType = GetIteratorElementType().Type;
             BoundExpression argument = (node.Expression == null)
                 ? BadExpression(node).MakeCompilerGenerated()
-                : binder.BindValue(node.Expression, diagnostics, BindValueKind.RValue);
+                : BindValue(node.Expression, diagnostics, BindValueKind.RValue);
             argument = ValidateEscape(argument, ExternalScope, isByRef: false, diagnostics: diagnostics);
 
             if (!argument.HasAnyErrors)
             {
-                argument = binder.GenerateConversionForAssignment(elementType, argument, diagnostics);
+                argument = GenerateConversionForAssignment(elementType, argument, diagnostics);
             }
             else
             {
@@ -261,7 +265,7 @@ private BoundStatement BindYieldBreakStatement(YieldStatementSyntax node, Diagno
                 Error(diagnostics, ErrorCode.ERR_YieldNotAllowedInScript, node.YieldKeyword);
             }
 
-            GetIteratorElementType(node, diagnostics);
+            ValidateYield(node, diagnostics);
             CheckRequiredLangVersionForAsyncIteratorMethods(diagnostics);
             return new BoundYieldBreakStatement(node);
         }
@@ -1706,7 +1710,7 @@ private BoundBlock BindBlockParts(BlockSyntax node, DiagnosticBag diagnostics)
                 var method = ContainingMemberOrLambda as MethodSymbol;
                 if ((object)method != null)
                 {
-                    method.IteratorElementTypeWithAnnotations = GetIteratorElementType(null, diagnostics);
+                    method.IteratorElementTypeWithAnnotations = GetIteratorElementType();
                 }
                 else
                 {

src/Compilers/CSharp/Portable/Binder/BuckStopsHereBinder.cs

@@ -143,7 +143,7 @@ internal override BoundExpression ConditionalReceiverExpression
 
         // This should only be called in the context of syntactically incorrect programs.  In other
         // contexts statements are surrounded by some enclosing method or lambda.
-        internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        internal override TypeWithAnnotations GetIteratorElementType()
         {
             // There's supposed to be an enclosing method or lambda.
             throw ExceptionUtilities.Unreachable;

src/Compilers/CSharp/Portable/Binder/ExecutableCodeBinder.cs

@@ -155,16 +155,39 @@ public void ValidateIteratorMethods(DiagnosticBag diagnostics)
                     }
                 }
 
+                Location errorLocation = iterator.Locations[0];
                 if (iterator.IsVararg)
                 {
                     // error CS1636: __arglist is not allowed in the parameter list of iterators
-                    diagnostics.Add(ErrorCode.ERR_VarargsIterator, iterator.Locations[0]);
+                    diagnostics.Add(ErrorCode.ERR_VarargsIterator, errorLocation);
                 }
 
                 if (((iterator as SourceMemberMethodSymbol)?.IsUnsafe == true || (iterator as LocalFunctionSymbol)?.IsUnsafe == true)
                     && Compilation.Options.AllowUnsafe) // Don't cascade
                 {
-                    diagnostics.Add(ErrorCode.ERR_IllegalInnerUnsafe, iterator.Locations[0]);
+                    diagnostics.Add(ErrorCode.ERR_IllegalInnerUnsafe, errorLocation);
+                }
+
+                var returnType = iterator.ReturnType;
+                RefKind refKind = iterator.RefKind;
+                TypeWithAnnotations elementType = InMethodBinder.GetIteratorElementTypeFromReturnType(Compilation, refKind, returnType, errorLocation, diagnostics);
+
+                if (elementType.IsDefault)
+                {
+                    if (refKind != RefKind.None)
+                    {
+                        Error(diagnostics, ErrorCode.ERR_BadIteratorReturnRef, errorLocation, iterator);
+                    }
+                    else if (!returnType.IsErrorType())
+                    {
+                        Error(diagnostics, ErrorCode.ERR_BadIteratorReturn, errorLocation, iterator, returnType);
+                    }
+                }
+
+                bool asyncInterface = InMethodBinder.IsAsyncStreamInterface(Compilation, refKind, returnType);
+                if (asyncInterface && !iterator.IsAsync)
+                {
+                    diagnostics.Add(ErrorCode.ERR_IteratorMustBeAsync, errorLocation, iterator, returnType);
                 }
             }
         }

src/Compilers/CSharp/Portable/Binder/InContainerBinder.cs

@@ -234,7 +234,7 @@ internal override void GetCandidateExtensionMethods(
             }
         }
 
-        internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        internal override TypeWithAnnotations GetIteratorElementType()
         {
             if (IsScriptClass)
             {
@@ -245,7 +245,7 @@ internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSynta
             else
             {
                 // This path would eventually throw, if we didn't have the case above.
-                return Next.GetIteratorElementType(node, diagnostics);
+                return Next.GetIteratorElementType();
             }
         }
 

src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs

@@ -13,27 +13,15 @@ namespace Microsoft.CodeAnalysis.CSharp
     /// <summary>
     /// A binder for a method body, which places the method's parameters in scope
     /// and notes if the method is an iterator method.
+    /// Note: instances of this type can be re-used across different attempts at compiling the same method (caching by binder factory).
     /// </summary>
     internal sealed class InMethodBinder : LocalScopeBinder
     {
         private MultiDictionary<string, ParameterSymbol> _lazyParameterMap;
         private readonly MethodSymbol _methodSymbol;
         private SmallDictionary<string, Symbol> _lazyDefinitionMap;
-        private IteratorInfo _iteratorInfo;
-
-        private class IteratorInfo
-        {
-            public static readonly IteratorInfo Empty = new IteratorInfo(default, default(ImmutableArray<Diagnostic>));
-
-            public readonly TypeWithAnnotations ElementType;
-            public readonly ImmutableArray<Diagnostic> ElementTypeDiagnostics;
-
-            public IteratorInfo(TypeWithAnnotations elementType, ImmutableArray<Diagnostic> elementTypeDiagnostics)
-            {
-                this.ElementType = elementType;
-                this.ElementTypeDiagnostics = elementTypeDiagnostics;
-            }
-        }
+        private TypeWithAnnotations.Boxed _iteratorElementType;
+        private readonly static TypeWithAnnotations.Boxed SentinelElementType = new TypeWithAnnotations.Boxed(default);
 
         public InMethodBinder(MethodSymbol owner, Binder enclosing)
             : base(enclosing, enclosing.Flags & ~BinderFlags.AllClearedAtExecutableCodeBoundary)
@@ -88,17 +76,17 @@ internal override bool IsInMethodBody
 
         internal void MakeIterator()
         {
-            if (_iteratorInfo == null)
+            if (_iteratorElementType == null)
             {
-                _iteratorInfo = IteratorInfo.Empty;
+                _iteratorElementType = SentinelElementType;
             }
         }
 
         internal override bool IsDirectlyInIterator
         {
             get
             {
-                return _iteratorInfo != null;
+                return _iteratorElementType != null;
             }
         }
 
@@ -126,7 +114,11 @@ internal override GeneratedLabelSymbol ContinueLabel
             }
         }
 
-        internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        protected override void ValidateYield(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        {
+        }
+
+        internal override TypeWithAnnotations GetIteratorElementType()
         {
             RefKind refKind = _methodSymbol.RefKind;
             TypeSymbol returnType = _methodSymbol.ReturnType;
@@ -139,49 +131,26 @@ internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSynta
                 // and deduce an iterator element type from the return type.  If we didn't do this, the 
                 // TypeInfo.ConvertedType of the yield statement would always be an error type.  However, we will 
                 // not mutate any state (i.e. we won't store the result).
-                var elementType = GetIteratorElementTypeFromReturnType(Compilation, refKind, returnType, node, diagnostics).elementType;
+                var elementType = GetIteratorElementTypeFromReturnType(Compilation, refKind, returnType, errorLocation: null, diagnostics: null);
                 return !elementType.IsDefault ? elementType : TypeWithAnnotations.Create(CreateErrorType());
             }
 
-            if (_iteratorInfo == IteratorInfo.Empty)
+            if (_iteratorElementType == SentinelElementType)
             {
-                DiagnosticBag elementTypeDiagnostics = DiagnosticBag.GetInstance();
-
-                (TypeWithAnnotations elementType, bool asyncInterface) = GetIteratorElementTypeFromReturnType(Compilation, refKind, returnType, node, elementTypeDiagnostics);
-
-                Location errorLocation = _methodSymbol.Locations[0];
+                TypeWithAnnotations elementType = GetIteratorElementTypeFromReturnType(Compilation, refKind, returnType, errorLocation: null, diagnostics: null);
                 if (elementType.IsDefault)
                 {
-                    if (refKind != RefKind.None)
-                    {
-                        Error(elementTypeDiagnostics, ErrorCode.ERR_BadIteratorReturnRef, errorLocation, _methodSymbol);
-                    }
-                    else if (!returnType.IsErrorType())
-                    {
-                        Error(elementTypeDiagnostics, ErrorCode.ERR_BadIteratorReturn, errorLocation, _methodSymbol, returnType);
-                    }
                     elementType = TypeWithAnnotations.Create(CreateErrorType());
                 }
-                else if (asyncInterface && !_methodSymbol.IsAsync)
-                {
-                    Error(elementTypeDiagnostics, ErrorCode.ERR_IteratorMustBeAsync, errorLocation, _methodSymbol, returnType);
-                }
-
-                var info = new IteratorInfo(elementType, elementTypeDiagnostics.ToReadOnlyAndFree());
 
-                var oldInfo = Interlocked.CompareExchange(ref _iteratorInfo, info, IteratorInfo.Empty);
-                if (oldInfo == IteratorInfo.Empty)
-                {
-                    diagnostics.AddRange(_iteratorInfo.ElementTypeDiagnostics);
-                }
+                Interlocked.CompareExchange(ref _iteratorElementType, new TypeWithAnnotations.Boxed(elementType), SentinelElementType);
             }
 
-            return _iteratorInfo.ElementType;
+            return _iteratorElementType.Value;
         }
 
-        // If an element type is found, we also return whether the interface is meant to be used with async.
-        internal static (TypeWithAnnotations elementType, bool asyncInterface) GetIteratorElementTypeFromReturnType(CSharpCompilation compilation,
-            RefKind refKind, TypeSymbol returnType, CSharpSyntaxNode errorLocationNode, DiagnosticBag diagnostics)
+        internal static TypeWithAnnotations GetIteratorElementTypeFromReturnType(CSharpCompilation compilation,
+            RefKind refKind, TypeSymbol returnType, Location errorLocation, DiagnosticBag diagnostics)
         {
             if (refKind == RefKind.None && returnType.Kind == SymbolKind.NamedType)
             {
@@ -193,25 +162,41 @@ internal static (TypeWithAnnotations elementType, bool asyncInterface) GetIterat
                         var objectType = compilation.GetSpecialType(SpecialType.System_Object);
                         if (diagnostics != null)
                         {
-                            ReportUseSiteDiagnostics(objectType, diagnostics, errorLocationNode);
+                            ReportUseSiteDiagnostics(objectType, diagnostics, errorLocation);
                         }
-                        return (TypeWithAnnotations.Create(objectType), false);
+                        return TypeWithAnnotations.Create(objectType);
 
                     case SpecialType.System_Collections_Generic_IEnumerable_T:
                     case SpecialType.System_Collections_Generic_IEnumerator_T:
-                        return (((NamedTypeSymbol)returnType).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0], false);
+                        return ((NamedTypeSymbol)returnType).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
                 }
 
-                if (TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T), TypeCompareKind.ConsiderEverything2) ||
-                    TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerator_T), TypeCompareKind.ConsiderEverything2))
+                if (TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T), TypeCompareKind.ConsiderEverything) ||
+                    TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerator_T), TypeCompareKind.ConsiderEverything))
                 {
-                    return (((NamedTypeSymbol)returnType).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0], true);
+                    return ((NamedTypeSymbol)returnType).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
                 }
             }
 
             return default;
         }
 
+        internal static bool IsAsyncStreamInterface(CSharpCompilation compilation, RefKind refKind, TypeSymbol returnType)
+        {
+            if (refKind == RefKind.None && returnType.Kind == SymbolKind.NamedType)
+            {
+                TypeSymbol originalDefinition = returnType.OriginalDefinition;
+
+                if (TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T), TypeCompareKind.ConsiderEverything) ||
+                    TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerator_T), TypeCompareKind.ConsiderEverything))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
         internal override void LookupSymbolsInSingleBinder(
             LookupResult result, string name, int arity, ConsList<TypeSymbol> basesBeingResolved, LookupOptions options, Binder originalBinder, bool diagnose, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
         {

src/Compilers/CSharp/Portable/Binder/WithLambdaParametersBinder.cs

@@ -71,13 +71,17 @@ internal override bool IsDirectlyInIterator
 
         // NOTE: Specifically not overriding IsIndirectlyInIterator.
 
-        internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
+        internal override TypeWithAnnotations GetIteratorElementType()
+        {
+            return TypeWithAnnotations.Create(CreateErrorType());
+        }
+
+        protected override void ValidateYield(YieldStatementSyntax node, DiagnosticBag diagnostics)
         {
             if (node != null)
             {
                 diagnostics.Add(ErrorCode.ERR_YieldInAnonMeth, node.YieldKeyword.GetLocation());
             }
-            return TypeWithAnnotations.Create(CreateErrorType());
         }
 
         internal override void LookupSymbolsInSingleBinder(

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

@@ -7758,7 +7758,7 @@ public override BoundNode VisitYieldReturnStatement(BoundYieldReturnStatement no
             }
             var method = _delegateInvokeMethod ?? (MethodSymbol)_symbol;
             TypeWithAnnotations elementType = InMethodBinder.GetIteratorElementTypeFromReturnType(compilation, RefKind.None,
-                method.ReturnType, errorLocationNode: null, diagnostics: null).elementType;
+                method.ReturnType, errorLocation: null, diagnostics: null);
 
             _ = VisitOptionalImplicitConversion(expr, elementType, useLegacyWarnings: false, trackMembers: false, AssignmentKind.Return);
             return null;

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

@@ -1881,6 +1881,7 @@ public static async System.Collections.Generic.IAsyncEnumerable<int> M()
 
         [Fact]
         [WorkItem(31608, "https://github.com/dotnet/roslyn/issues/31608")]
+        [WorkItem(39970, "https://github.com/dotnet/roslyn/issues/39970")]
         public void AsyncIterator_WithoutAwait_WithoutAsync()
         {
             string source = @"
@@ -1892,11 +1893,13 @@ static System.Collections.Generic.IAsyncEnumerable<int> M()
     }
 }";
             var comp = CreateCompilationWithAsyncIterator(source);
-            comp.VerifyDiagnostics(
+            var expected = new DiagnosticDescription[] {
                 // (4,61): error CS8403: Method 'C.M()' with an iterator block must be 'async' to return 'IAsyncEnumerable<int>'
                 //     static System.Collections.Generic.IAsyncEnumerable<int> M()
                 Diagnostic(ErrorCode.ERR_IteratorMustBeAsync, "M").WithArguments("C.M()", "System.Collections.Generic.IAsyncEnumerable<int>").WithLocation(4, 61)
-                );
+            };
+            comp.VerifyDiagnostics(expected);
+            comp.VerifyEmitDiagnostics(expected);
         }
 
         [Fact]

src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs

@@ -14586,14 +14586,16 @@ public static void Main()
 }
 ";
             var comp = CreateCompilation(text);
-            comp.VerifyDiagnostics(
+            var expected = new DiagnosticDescription[] {
                 // (12,13): error CS1621: The yield statement cannot be used inside an anonymous method or lambda expression
                 //             yield return this; // CS1621
                 Diagnostic(ErrorCode.ERR_YieldInAnonMeth, "yield"),
                 // (8,24): error CS0161: 'C.GetEnumerator()': not all code paths return a value
                 //     public IEnumerator GetEnumerator()
                 Diagnostic(ErrorCode.ERR_ReturnExpected, "GetEnumerator").WithArguments("C.GetEnumerator()")
-                );
+            };
+            comp.VerifyDiagnostics(expected);
+            comp.VerifyEmitDiagnostics(expected);
         }
 
         [Fact]