Async-streams: allow pattern-based disposal in await using and foreach (

#32731)

docs/features/async-streams.md

@@ -64,15 +64,19 @@ Just like in iterator methods, there are several restrictions on where a yield s
 
 An asynchronous `using` is lowered just like a regular `using`, except that `Dispose()` is replaced with `await DisposeAsync()`.
 
+Note that pattern-based lookup for `DisposeAsync` binds to instance methods that can be invoked without arguments.
+Extension methods do not contribute. The result of `DisposeAsync` must be awaitable.
+
 ### Detailed design for `await foreach` statement
 
 An `await foreach` is lowered just like a regular `foreach`, except that:
 - `GetEnumerator()` is replaced with `await GetAsyncEnumerator()`
 - `MoveNext()` is replaced with `await MoveNextAsync()`
 - `Dispose()` is replaced with `await DisposeAsync()`
 
-Note that pattern-based lookup for `GetAsyncEnumerator` and `MoveNextAsync` do not place particular requirements on those methods,
-as long as they could be invoked without arguments.
+Note that pattern-based lookup for `GetAsyncEnumerator`, `MoveNextAsync` and `DisposeAsync` binds to instance methods that can be invoked without arguments.
+Extension methods do not contribute. The result of `MoveNextAsync` and `DisposeAsync` must be awaitable.
+Disposal for `await foreach` does not include a fallback to a runtime check for an interface implementation.
 
 Asynchronous foreach loops are disallowed on collections of type dynamic,
 as there is no asynchronous equivalent of the non-generic `IEnumerable` interface.

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

@@ -9,6 +9,8 @@ namespace Microsoft.CodeAnalysis.CSharp
     /// </summary>
     internal sealed class AwaitableInfo
     {
+        public static readonly AwaitableInfo Empty = new AwaitableInfo(getAwaiterMethod: null, isCompletedProperty: null, getResultMethod: null);
+
         public readonly MethodSymbol GetAwaiter;
         public readonly PropertySymbol IsCompleted;
         public readonly MethodSymbol GetResult;

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

@@ -34,18 +34,24 @@ private BoundAwaitExpression BindAwait(BoundExpression expression, SyntaxNode no
             return new BoundAwaitExpression(node, expression, info, awaitExpressionType, hasErrors);
         }
 
-        internal AwaitableInfo BindAwaitInfo(BoundExpression expression, SyntaxNode node, Location location, DiagnosticBag diagnostics, ref bool hasErrors)
+        internal AwaitableInfo BindAwaitInfo(BoundExpression expressionOpt, SyntaxNode node, Location location, DiagnosticBag diagnostics, ref bool hasErrors)
         {
-            MethodSymbol getAwaiter;
-            PropertySymbol isCompleted;
-            MethodSymbol getResult;
+            hasErrors |= ReportBadAwaitWithoutAsync(location, diagnostics);
+            hasErrors |= ReportBadAwaitContext(node, location, diagnostics);
 
-            hasErrors |=
-                ReportBadAwaitWithoutAsync(location, diagnostics) |
-                ReportBadAwaitContext(node, location, diagnostics) |
-                !GetAwaitableExpressionInfo(expression, out getAwaiter, out isCompleted, out getResult, out _, node, diagnostics);
+            if (expressionOpt is null)
+            {
+                return AwaitableInfo.Empty;
+            }
+            else
+            {
+                MethodSymbol getAwaiter;
+                PropertySymbol isCompleted;
+                MethodSymbol getResult;
+                hasErrors |= !GetAwaitableExpressionInfo(expressionOpt, out getAwaiter, out isCompleted, out getResult, out _, node, diagnostics);
 
-            return new AwaitableInfo(getAwaiter, isCompleted, getResult);
+                return new AwaitableInfo(getAwaiter, isCompleted, getResult);
+            }
         }
 
         /// <summary>

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

@@ -677,7 +677,7 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
         {
             Debug.Assert(!(expr is null));
             Debug.Assert(!(expr.Type is null));
-            Debug.Assert(expr.Type.IsValueType && expr.Type.IsRefLikeType); // pattern dispose lookup is only valid on ref structs
+            Debug.Assert(expr.Type.IsRefLikeType || hasAwait); // pattern dispose lookup is only valid on ref structs or asynchronous usings
 
             // Don't try and lookup if we're not enabled
             if (MessageID.IDS_FeatureUsingDeclarations.RequiredVersion() > Compilation.LanguageVersion)
@@ -691,8 +691,15 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
                                                     diagnostics,
                                                     out var disposeMethod);
 
-            if ((!hasAwait && disposeMethod?.ReturnsVoid == false)
-                || (hasAwait && disposeMethod?.ReturnType.TypeSymbol.IsNonGenericTaskType(Compilation) == false)
+            if (disposeMethod?.IsExtensionMethod == true)
+            {
+                // Extension methods should just be ignored, rather than rejected after-the-fact
+                // Tracked by https://github.com/dotnet/roslyn/issues/32767
+
+                // extension methods do not contribute to pattern-based disposal
+                disposeMethod = null;
+            }
+            else if ((!hasAwait && disposeMethod?.ReturnsVoid == false)
                 || result == PatternLookupResult.NotAMethod)
             {
                 HashSet<DiagnosticInfo> useSiteDiagnostics = null;

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

@@ -49,7 +49,7 @@ private ForEachEnumeratorInfo(
             MethodSymbol getEnumeratorMethod,
             MethodSymbol currentPropertyGetter,
             MethodSymbol moveNextMethod,
-            bool needsDisposeMethod,
+            bool needsDisposal,
             AwaitableInfo disposeAwaitableInfo,
             MethodSymbol disposeMethod,
             Conversion collectionConversion,
@@ -68,7 +68,7 @@ private ForEachEnumeratorInfo(
             this.GetEnumeratorMethod = getEnumeratorMethod;
             this.CurrentPropertyGetter = currentPropertyGetter;
             this.MoveNextMethod = moveNextMethod;
-            this.NeedsDisposal = needsDisposeMethod;
+            this.NeedsDisposal = needsDisposal;
             this.DisposeAwaitableInfo = disposeAwaitableInfo;
             this.DisposeMethod = disposeMethod;
             this.CollectionConversion = collectionConversion;

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

@@ -209,7 +209,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
             MethodSymbol getEnumeratorMethod = builder.GetEnumeratorMethod;
             if (IsAsync)
             {
-                BoundExpression placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression, builder.MoveNextMethod?.ReturnType.TypeSymbol ?? CreateErrorType());
+                var placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression, builder.MoveNextMethod?.ReturnType.TypeSymbol ?? CreateErrorType());
                 awaitInfo = BindAwaitInfo(placeholder, _syntax.Expression, _syntax.AwaitKeyword.GetLocation(), diagnostics, ref hasErrors);
 
                 if (!hasErrors && awaitInfo.GetResult?.ReturnType.SpecialType != SpecialType.System_Boolean)
@@ -503,8 +503,11 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
 
         private bool GetAwaitDisposeAsyncInfo(ref ForEachEnumeratorInfo.Builder builder, DiagnosticBag diagnostics)
         {
-            BoundExpression placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression,
-                this.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask, diagnostics, this._syntax));
+            var awaitableType = builder.DisposeMethod is null
+                ? this.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask, diagnostics, this._syntax)
+                : builder.DisposeMethod.ReturnType.TypeSymbol;
+
+            var placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression, awaitableType);
 
             bool hasErrors = false;
             builder.DisposeAwaitableInfo = BindAwaitInfo(placeholder, _syntax.Expression, _syntax.AwaitKeyword.GetLocation(), diagnostics, ref hasErrors);
@@ -836,16 +839,17 @@ private void GetDisposalInfoForEnumerator(ref ForEachEnumeratorInfo.Builder buil
             TypeSymbol enumeratorType = builder.GetEnumeratorMethod.ReturnType.TypeSymbol;
             HashSet<DiagnosticInfo> useSiteDiagnostics = null;
 
-            if (!enumeratorType.IsSealed ||
+            // For async foreach, we don't do the runtime check
+            if ((!enumeratorType.IsSealed && !isAsync) ||
                 this.Conversions.ClassifyImplicitConversionFromType(enumeratorType,
                     isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),
                     ref useSiteDiagnostics).IsImplicit)
             {
                 builder.NeedsDisposal = true;
             }
-            else if (enumeratorType.IsRefLikeType)
+            else if (enumeratorType.IsRefLikeType || isAsync)
             {
-                // if it wasn't directly convertable to IDisposable, see if it is pattern disposable
+                // if it wasn't directly convertable to IDisposable, see if it is pattern-disposable
                 // again, we throw away any binding diagnostics, and assume it's not disposable if we encounter errors
                 var patternDisposeDiags = new DiagnosticBag();
                 var receiver = new BoundDisposableValuePlaceholder(_syntax, enumeratorType);

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

@@ -90,6 +90,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
             AwaitableInfo awaitOpt = null;
             TypeSymbol declarationTypeOpt = null;
             MethodSymbol disposeMethodOpt = null;
+            TypeSymbol awaitableTypeOpt = null;
 
             if (isExpression)
             {
@@ -117,11 +118,19 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
 
             if (hasAwait)
             {
-                TypeSymbol taskType = originalBinder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask);
-                hasErrors |= ReportUseSiteDiagnostics(taskType, diagnostics, awaitKeyword);
+                BoundAwaitableValuePlaceholder placeholderOpt;
+                if (awaitableTypeOpt is null)
+                {
+                    placeholderOpt = null;
+                }
+                else
+                {
+                    hasErrors |= ReportUseSiteDiagnostics(awaitableTypeOpt, diagnostics, awaitKeyword);
+                    placeholderOpt = new BoundAwaitableValuePlaceholder(syntax, awaitableTypeOpt).MakeCompilerGenerated();
+                }
 
-                BoundExpression placeholder = new BoundAwaitableValuePlaceholder(syntax, taskType).MakeCompilerGenerated();
-                awaitOpt = originalBinder.BindAwaitInfo(placeholder, syntax, awaitKeyword.GetLocation(), diagnostics, ref hasErrors);
+                // even if we don't have a proper value to await, we'll still report bad usages of `await`
+                awaitOpt = originalBinder.BindAwaitInfo(placeholderOpt, syntax, awaitKeyword.GetLocation(), diagnostics, ref hasErrors);
             }
 
             // This is not awesome, but its factored. 
@@ -146,6 +155,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
                     hasErrors);
             }
 
+            // initializes iDisposableConversion, awaitableTypeOpt and disposeMethodOpt
             bool populateDisposableConversionOrDisposeMethod(bool fromExpression)
             {
                 HashSet<DiagnosticInfo> useSiteDiagnostics = null;
@@ -155,14 +165,18 @@ bool populateDisposableConversionOrDisposeMethod(bool fromExpression)
 
                 if (iDisposableConversion.IsImplicit)
                 {
+                    if (hasAwait)
+                    {
+                        awaitableTypeOpt = originalBinder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask);
+                    }
                     return true;
                 }
 
                 TypeSymbol type = fromExpression ? expressionOpt.Type : declarationTypeOpt;
 
-                // If this is a ref struct, try binding via pattern.
+                // If this is a ref struct, or we're in a valid asynchronous using, try binding via pattern.
                 // We won't need to try and bind a second time if it fails, as async dispose can't be pattern based (ref structs are not allowed in async methods)
-                if (!(type is null) && type.IsValueType && type.IsRefLikeType)
+                if (!(type is null) && (type.IsRefLikeType || hasAwait))
                 {
                     BoundExpression receiver = fromExpression
                                                ? expressionOpt
@@ -171,6 +185,10 @@ bool populateDisposableConversionOrDisposeMethod(bool fromExpression)
                     disposeMethodOpt = originalBinder.TryFindDisposePatternMethod(receiver, syntax, hasAwait, diagnostics);
                     if (!(disposeMethodOpt is null))
                     {
+                        if (hasAwait)
+                        {
+                            awaitableTypeOpt = disposeMethodOpt.ReturnType.TypeSymbol;
+                        }
                         return true;
                     }
                 }

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -2981,16 +2981,16 @@ A catch() block after a catch (System.Exception e) block can catch non-CLS excep
     <value>Anonymous methods, lambda expressions, and query expressions inside structs cannot access instance members of 'this'. Consider copying 'this' to a local variable outside the anonymous method, lambda expression or query expression and using the local instead.</value>
   </data>
   <data name="ERR_NoConvToIDisp" xml:space="preserve">
-    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'.</value>
+    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable' or implement a suitable 'Dispose' method.</value>
   </data>
   <data name="ERR_NoConvToIDispWrongAsync" xml:space="preserve">
-    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'. Did you mean 'await using' rather than 'using'?</value>
+    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable' or implement a suitable 'Dispose' method. Did you mean 'await using' rather than 'using'?</value>
   </data>
   <data name="ERR_NoConvToIAsyncDisp" xml:space="preserve">
-    <value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable'</value>
+    <value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable' or implement a suitable 'DisposeAsync' method.</value>
   </data>
   <data name="ERR_NoConvToIAsyncDispWrongAsync" xml:space="preserve">
-    <value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable'. Did you mean 'using' rather than 'await using'?</value>
+    <value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable' or implement a suitable 'DisposeAsync' method. Did you mean 'using' rather than 'await using'?</value>
   </data>
   <data name="ERR_BadParamRef" xml:space="preserve">
     <value>Parameter {0} must be declared with the '{1}' keyword</value>