Use reachability to influence how the async exception handler rewrites try blocks. This is a work in progress

Author: 333fred

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

@@ -617,7 +617,7 @@ BoundBlock runAnalysis(BoundBlock block, BindingDiagnosticBag blockDiagnostics)
                     // It's a bit of code duplication, but refactoring would make things worse.
                     // However, we don't need to report diagnostics here. They will be reported when analyzing the parent method.
                     var ignored = DiagnosticBag.GetInstance();
-                    var endIsReachable = ControlFlowPass.Analyze(localSymbol.DeclaringCompilation, localSymbol, block, ignored);
+                    var endIsReachable = ControlFlowPass.Analyze(localSymbol.DeclaringCompilation, localSymbol, block, ignored, out PooledDictionary<BoundNode, bool> asyncTryFinallyEndsReachable);
                     ignored.Free();
                     if (endIsReachable)
                     {
@@ -630,6 +630,12 @@ BoundBlock runAnalysis(BoundBlock block, BindingDiagnosticBag blockDiagnostics)
                             blockDiagnostics.Add(ErrorCode.ERR_ReturnExpected, localSymbol.GetFirstLocation(), localSymbol);
                         }
                     }
+
+                    if (asyncTryFinallyEndsReachable != null)
+                    {
+                        block = (BoundBlock)FlowAnalysisPass.AsyncTryFinallyEndsReachableRewriter.Rewrite(block, asyncTryFinallyEndsReachable);
+                        asyncTryFinallyEndsReachable.Free();
+                    }
                 }
 
                 return block;
@@ -3220,7 +3226,7 @@ private BoundTryStatement BindTryStatement(TryStatementSyntax node, BindingDiagn
             var tryBlock = BindEmbeddedBlock(node.Block, diagnostics);
             var catchBlocks = BindCatchBlocks(node.Catches, diagnostics);
             var finallyBlockOpt = (node.Finally != null) ? BindEmbeddedBlock(node.Finally.Block, diagnostics) : null;
-            return new BoundTryStatement(node, tryBlock, catchBlocks, finallyBlockOpt);
+            return new BoundTryStatement(node, tryBlock, catchBlocks, finallyBlockOpt, AsyncTryFinallyEndReachable.Unknown);
         }
 
         private ImmutableArray<BoundCatchBlock> BindCatchBlocks(SyntaxList<CatchClauseSyntax> catchClauses, BindingDiagnosticBag diagnostics)

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

@@ -455,6 +455,7 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
                     deconstructStep,
                     awaitInfo,
                     body,
+                    AsyncTryFinallyEndReachable.Unknown,
                     this.BreakLabel,
                     this.ContinueLabel,
                     hasErrors);
@@ -596,6 +597,7 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
                 deconstructStep,
                 awaitInfo,
                 body,
+                AsyncTryFinallyEndReachable.Unknown,
                 this.BreakLabel,
                 this.ContinueLabel,
                 hasErrors);

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

@@ -164,7 +164,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
             // In the future it might be better to have a separate shared type that we add the info to, and have the callers create the appropriate bound nodes from it
             if (isUsingDeclaration)
             {
-                return new BoundUsingLocalDeclarations(syntax, patternDisposeInfo, awaitOpt, declarationsOpt, hasErrors);
+                return new BoundUsingLocalDeclarations(syntax, patternDisposeInfo, awaitOpt, AsyncTryFinallyEndReachable.Unknown, declarationsOpt, hasErrors);
             }
             else
             {
@@ -178,6 +178,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
                     boundBody,
                     awaitOpt,
                     patternDisposeInfo,
+                    AsyncTryFinallyEndReachable.Unknown,
                     hasErrors);
             }
 

src/Compilers/CSharp/Portable/BoundTree/AsyncTryFinallyEndReachable.cs

@@ -0,0 +1,33 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+namespace Microsoft.CodeAnalysis.CSharp;
+
+/// <summary>
+/// Tracks the reachability of the end of a bound node that either already is a try/finally, or has the potential to become one during lowering.
+/// Only nodes that can have an await in the finally block are tracked with this enum, for use in <see cref="AsyncExceptionHandlerRewriter.VisitTryStatement(BoundTryStatement)"/>.
+/// Try/finally nodes that are synthesized by the compiler during lowering and do not have an await in the finally block can use <see cref="AsyncTryFinallyEndReachable.Ignored"/>.
+/// </summary>
+internal enum AsyncTryFinallyEndReachable
+{
+    /// <summary>
+    /// The reachability of the end of the node has not been determined yet.
+    /// </summary>
+    Unknown,
+
+    /// <summary>
+    /// The end of the node is reachable.
+    /// </summary>
+    Reachable,
+
+    /// <summary>
+    /// The end of the node is unreachable.
+    /// </summary>
+    Unreachable,
+
+    /// <summary>
+    /// The reachability of the end of the node is intentionally not set. This should never be encountered in a try/finally that has an await in the finally block.
+    /// </summary>
+    Ignored
+}

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -40,6 +40,7 @@
   <ValueType Name="CollectionExpressionTypeKind"/>
   <ValueType Name="AccessorKind" />
   <ValueType Name="SafeContext" />
+  <ValueType Name="AsyncTryFinallyEndReachable"/>
 
   <AbstractNode Name="BoundInitializer" Base="BoundNode"/>
 
@@ -1124,6 +1125,7 @@
   <Node Name="BoundUsingLocalDeclarations" Base="BoundMultipleLocalDeclarationsBase">
     <Field Name="PatternDisposeInfoOpt" Type="MethodArgumentInfo?"/>
     <Field Name="AwaitOpt" Type="BoundAwaitableInfo?"/>
+    <Field Name="EndIsReachable" Type="AsyncTryFinallyEndReachable" />
   </Node>
 
   <!-- 
@@ -1285,6 +1287,7 @@
     <Field Name="DeconstructionOpt" Type="BoundForEachDeconstructStep?"/>
     <Field Name="AwaitOpt" Type="BoundAwaitableInfo?"/>
     <Field Name="Body" Type="BoundStatement"/>
+    <Field Name="EndIsReachable" Type="AsyncTryFinallyEndReachable" />
   </Node>
 
   <!-- All the information need to apply a deconstruction at each iteration of a foreach loop involving a deconstruction-declaration. -->
@@ -1301,6 +1304,7 @@
     <Field Name="Body" Type="BoundStatement"/>
     <Field Name="AwaitOpt" Type="BoundAwaitableInfo?"/>
     <Field Name="PatternDisposeInfoOpt" Type="MethodArgumentInfo?"/>
+    <Field Name="EndIsReachable" Type="AsyncTryFinallyEndReachable" />
   </Node>
 
   <Node Name="BoundFixedStatement" Base="BoundStatement">
@@ -1314,7 +1318,7 @@
     <Field Name="Body" Type="BoundStatement"/>
   </Node>
 
-  <Node Name="BoundTryStatement" Base="BoundStatement">
+  <Node Name="BoundTryStatement" Base="BoundStatement" HasValidate="true">
     <Field Name="TryBlock" Type="BoundBlock"/>
     <Field Name="CatchBlocks" Type="ImmutableArray&lt;BoundCatchBlock&gt;"/>
     <Field Name="FinallyBlockOpt" Type="BoundBlock?"/>
@@ -1354,6 +1358,8 @@
     -->
 
     <Field Name="PreferFaultHandler" Type="bool"/>
+
+    <Field Name="EndIsReachable" Type="AsyncTryFinallyEndReachable" />
   </Node>
 
   <Node Name="BoundCatchBlock" Base="BoundNode">

src/Compilers/CSharp/Portable/BoundTree/BoundTryStatement.cs

@@ -0,0 +1,26 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Diagnostics;
+
+namespace Microsoft.CodeAnalysis.CSharp;
+
+internal partial class BoundTryStatement : BoundStatement
+{
+    private partial void Validate()
+    {
+#if DEBUG
+        if (EndIsReachable == AsyncTryFinallyEndReachable.Ignored && FinallyBlockOpt != null)
+        {
+            Debug.Assert(!AsyncExceptionHandlerRewriter.AwaitsInFinally(FinallyBlockOpt),
+                """
+                There are awaits in this finally block, reachability cannot be ignored! Ensure that valid reachability
+                is set for this try statement, and ensure that we have async tests for the case where the end of the
+                try is not reachable and the construct is in a Task<T> or ValueTask<T>-returning method, and runtime
+                async is enabled for the test case to ensure that we not branching to invalid locations.
+                """);
+        }
+#endif
+    }
+}

src/Compilers/CSharp/Portable/BoundTree/Constructors.cs

@@ -613,8 +613,8 @@ public BoundDefaultExpression(SyntaxNode syntax, TypeSymbol type, bool hasErrors
 
     internal partial class BoundTryStatement
     {
-        public BoundTryStatement(SyntaxNode syntax, BoundBlock tryBlock, ImmutableArray<BoundCatchBlock> catchBlocks, BoundBlock? finallyBlockOpt, LabelSymbol? finallyLabelOpt = null)
-            : this(syntax, tryBlock, catchBlocks, finallyBlockOpt, finallyLabelOpt, preferFaultHandler: false, hasErrors: false)
+        public BoundTryStatement(SyntaxNode syntax, BoundBlock tryBlock, ImmutableArray<BoundCatchBlock> catchBlocks, BoundBlock? finallyBlockOpt, AsyncTryFinallyEndReachable endIsReachable, LabelSymbol? finallyLabelOpt = null)
+            : this(syntax, tryBlock, catchBlocks, finallyBlockOpt, finallyLabelOpt, preferFaultHandler: false, endIsReachable: endIsReachable, hasErrors: false)
         {
         }
     }

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -867,7 +867,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType, bool inExpressionTr
 
             ValidateUnsafeParameters(diagnostics, cacheKey.ParameterTypes);
 
-            bool reachableEndpoint = ControlFlowPass.Analyze(compilation, lambdaSymbol, block, diagnostics.DiagnosticBag);
+            bool reachableEndpoint = ControlFlowPass.Analyze(compilation, lambdaSymbol, block, diagnostics.DiagnosticBag, out PooledDictionary<BoundNode, bool>? asyncTryFinallyEndsReachable);
             if (reachableEndpoint)
             {
                 if (Binder.MethodOrLambdaRequiresValue(lambdaSymbol, this.Binder.Compilation))
@@ -881,6 +881,14 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType, bool inExpressionTr
                 }
             }
 
+            if (asyncTryFinallyEndsReachable != null)
+            {
+                // If the lambda is async, we need to ensure that the async try-finally endpoints are reachable.
+                // This is necessary for correct code generation.
+                block = (BoundBlock)FlowAnalysisPass.AsyncTryFinallyEndsReachableRewriter.Rewrite(block, asyncTryFinallyEndsReachable);
+                asyncTryFinallyEndsReachable.Free();
+            }
+
             if (IsAsync && !ErrorFacts.PreventsSuccessfulDelegateConversion(diagnostics.DiagnosticBag))
             {
                 if (returnType.HasType && // Can be null if "delegateType" is not actually a delegate type.

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs

@@ -321,8 +321,13 @@ private void HandleReturn()
         {
             _builder.MarkLabel(s_returnLabel);
 
-            Debug.Assert(_method.ReturnsVoid == (_returnTemp == null));
+            Debug.Assert((_method.ReturnsVoid == (_returnTemp == null))
+                || (_method.ReturnsVoid
+                    && _module.Compilation.IsRuntimeAsyncEnabledIn(_method)
+                    && ((InternalSpecialType)_method.ReturnType.OriginalDefinition.ExtendedSpecialType) is InternalSpecialType.System_Threading_Tasks_Task or InternalSpecialType.System_Threading_Tasks_ValueTask),
+                "return temp should be null for void methods, or for async Task/ValueTask methods");
 
+            // PROTOTYPE: Revisit
             if (_emitPdbSequencePoints && !_method.IsIterator && !_method.IsAsync)
             {
                 // In debug mode user could set a breakpoint on the last "}" of the method and 

src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs

@@ -1676,7 +1676,7 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
 
             EnsureOnlyEvalStack();
 
-            return node.Update(tryBlock, catchBlocks, finallyBlock, finallyLabelOpt: node.FinallyLabelOpt, node.PreferFaultHandler);
+            return node.Update(tryBlock, catchBlocks, finallyBlock, finallyLabelOpt: node.FinallyLabelOpt, node.PreferFaultHandler, node.EndIsReachable);
         }
 
         public override BoundNode VisitCatchBlock(BoundCatchBlock node)