Fixes an issue with flow analysis where branches within `try` could be...

    Fixes an issue with flow analysis where branches within `try` could be considered as going through `finally`

    We could incorrectly assume that variable is assigned (or unassigned in async capture tracking case).

    As a resutl we could
    - allow use of unassigned locals (undefined behavior)
    - not capture a local whose value survives `await` (NRE crash)

    Also added a regression test fro VB, which does not seem affected.

    Fixes:#19831

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

@@ -64,10 +64,13 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
                 // to execute the finally block before occurring.  Also, we do not handle branches
                 // *into* the finally block.
                 SetState(finallyState);
-                var tryAndCatchPending = SavePending();
                 var unsetInFinally = AllBitsSet();
                 VisitFinallyBlock(node.FinallyBlockOpt, ref unsetInFinally);
-                foreach (var pend in tryAndCatchPending.PendingBranches)
+                
+                // all the branches that are still pending must have been introduced in try/finally 
+                // with exception of YieldReturn, the branches logically go through finally
+                // so we must Union/Intersect finally state as appropriate
+                foreach (var pend in PendingBranches)
                 {
                     if (pend.Branch == null) continue; // a tracked exception
                     if (pend.Branch.Kind != BoundKind.YieldReturnStatement)
@@ -77,7 +80,6 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
                     }
                 }
 
-                RestorePending(tryAndCatchPending);
                 UnionWith(ref endState, ref this.State);
                 if (trackUnassignments) IntersectWith(ref endState, ref unsetInFinally);
             }
@@ -87,13 +89,16 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
             return null;
         }
 
-        protected virtual void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState)
+        protected virtual void VisitTryBlock(BoundBlock tryBlock, BoundTryStatement node, ref TLocalState tryState)
         {
+            var oldPending = SavePending();
             VisitStatement(tryBlock);
+            RestorePending(oldPending);
         }
 
         protected virtual void VisitCatchBlock(BoundCatchBlock catchBlock, ref TLocalState finallyState)
         {
+            var oldPending = SavePending();
             if (catchBlock.ExceptionSourceOpt != null)
             {
                 VisitLvalue(catchBlock.ExceptionSourceOpt);
@@ -106,11 +111,14 @@ protected virtual void VisitCatchBlock(BoundCatchBlock catchBlock, ref TLocalSta
             }
 
             VisitStatement(catchBlock.Body);
+            RestorePending(oldPending);
         }
 
-        protected virtual void VisitFinallyBlock(BoundStatement finallyBlock, ref TLocalState unsetInFinally)
+        protected virtual void VisitFinallyBlock(BoundBlock finallyBlock, ref TLocalState unsetInFinally)
         {
+            var oldPending = SavePending();
             VisitStatement(finallyBlock); // this should generate no pending branches
+            RestorePending(oldPending);
         }
     }
 }

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

@@ -245,32 +245,10 @@ private void CheckReachable(BoundStatement statement)
             }
         }
 
-        protected override void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref LocalState tryState)
-        {
-            if (node.CatchBlocks.IsEmpty)
-            {
-                base.VisitTryBlock(tryBlock, node, ref tryState);
-                return;
-            }
-
-            var oldPending = SavePending(); // we do not support branches into a try block
-            base.VisitTryBlock(tryBlock, node, ref tryState);
-            RestorePending(oldPending);
-        }
-
-        protected override void VisitCatchBlock(BoundCatchBlock catchBlock, ref LocalState finallyState)
-        {
-            var oldPending = SavePending(); // we do not support branches into a catch block
-            base.VisitCatchBlock(catchBlock, ref finallyState);
-            RestorePending(oldPending);
-        }
-
-        protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref LocalState endState)
+        protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalState endState)
         {
             var oldPending1 = SavePending(); // we do not support branches into a finally block
-            var oldPending2 = SavePending(); // track only the branches out of the finally block
             base.VisitFinallyBlock(finallyBlock, ref endState);
-            RestorePending(oldPending2); // resolve branches that remain within the finally block
             foreach (var branch in PendingBranches)
             {
                 if (branch.Branch == null) continue; // a tracked exception

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

@@ -2034,7 +2034,7 @@ public override BoundNode VisitBaseReference(BoundBaseReference node)
 #region TryStatements
         private OptionalState _tryState;
 
-        protected override void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref LocalState tryState)
+        protected override void VisitTryBlock(BoundBlock tryBlock, BoundTryStatement node, ref LocalState tryState)
         {
             if (trackUnassignments)
             {
@@ -2098,7 +2098,7 @@ private void VisitCatchBlockInternal(BoundCatchBlock catchBlock, ref LocalState
             }
         }
 
-        protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref LocalState unsetInFinally)
+        protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalState unsetInFinally)
         {
             if (trackUnassignments)
             {

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

@@ -809,12 +809,10 @@ protected struct SavedPending
             public readonly ArrayBuilder<PendingBranch> PendingBranches;
             public readonly PooledHashSet<BoundStatement> LabelsSeen;
 
-            public SavedPending(ref ArrayBuilder<PendingBranch> pendingBranches, ref PooledHashSet<BoundStatement> labelsSeen)
+            public SavedPending(ArrayBuilder<PendingBranch> pendingBranches, PooledHashSet<BoundStatement> labelsSeen)
             {
                 this.PendingBranches = pendingBranches;
                 this.LabelsSeen = labelsSeen;
-                pendingBranches = ArrayBuilder<PendingBranch>.GetInstance();
-                labelsSeen = PooledHashSet<BoundStatement>.GetInstance();
             }
         }
 
@@ -826,7 +824,11 @@ public SavedPending(ref ArrayBuilder<PendingBranch> pendingBranches, ref PooledH
         protected SavedPending SavePending()
         {
             Debug.Assert(!this.IsConditionalState);
-            var result = new SavedPending(ref _pendingBranches, ref _labelsSeen);
+            var result = new SavedPending(_pendingBranches, _labelsSeen);
+
+            _pendingBranches = ArrayBuilder<PendingBranch>.GetInstance();
+            _labelsSeen = PooledHashSet<BoundStatement>.GetInstance();
+
             if (_trackExceptions)
             {
                 _pendingBranches.Add(new PendingBranch(null, this.State));
@@ -836,7 +838,10 @@ protected SavedPending SavePending()
         }
 
         /// <summary>
-        /// We use this to restore the old set of pending branches after visiting a construct that contains nested statements.
+        /// We use this when closing a block that may contain labels or branches
+        /// - branches to new labels are resolved
+        /// - new labels are removed (no longer can be reached)
+        /// - unresolved pending branches are carried forward
         /// </summary>
         /// <param name="oldPending">The old pending branches, which are to be merged with the current ones</param>
         protected void RestorePending(SavedPending oldPending)
@@ -881,20 +886,22 @@ protected void RestorePending(SavedPending oldPending)
                 }
             }
 
-            int i = 0;
-            int n = _pendingBranches.Count;
             if (_trackExceptions)
             {
                 Debug.Assert(oldPending.PendingBranches[0].Branch == null);
                 Debug.Assert(this.PendingBranches[0].Branch == null);
                 this.IntersectWith(ref oldPending.PendingBranches[0].State, ref this.PendingBranches[0].State);
-                i++;
-            }
 
-            for (; i < n; i++)
-            {
-                oldPending.PendingBranches.Add(this.PendingBranches[i]);
+                for (int i = 1, c = this.PendingBranches.Count; i < c; i++)
+                {
+                    oldPending.PendingBranches.Add(this.PendingBranches[i]);
+                }
             }
+            else
+            { 
+                oldPending.PendingBranches.AddRange(this.PendingBranches);
+            }
+
             _pendingBranches.Free();
             _pendingBranches = oldPending.PendingBranches;
 

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

@@ -298,7 +298,7 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
             return null;
         }
 
-        protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref LocalState unsetInFinally)
+        protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalState unsetInFinally)
         {
             if (_seenYieldInCurrentTry)
             {

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

@@ -5021,8 +5021,8 @@ public void CaptureAssignedInOuterFinally()
         {
             var source = @"
 
-using System;
-using System.Threading.Tasks;
+    using System;
+    using System.Threading.Tasks;
 
     public class Program
     {

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/FlowTests.cs

@@ -2351,5 +2351,52 @@ void TestFunc(int i)
 ";
             CreateCompilationWithMscorlib45(source).VerifyDiagnostics();
         }
+
+        [Fact, WorkItem(19831, "https://github.com/dotnet/roslyn/issues/19831")]
+        public void AssignedInFinallyUsedInTry()
+        {
+            var source =
+@"  
+    public class Program
+    {
+        static void Main(string[] args)
+        {
+            Test();
+        }
+
+        public static void Test()
+        {
+            object obj; 
+
+            try
+            {
+                goto l3;
+
+                l1:
+                goto l2;
+
+                l3:
+                goto l1;
+
+
+                l2:
+
+                // Should be compile error
+                // 'obj' is uninitialized
+                obj.ToString();
+            }
+            finally
+            {
+                obj = 1;
+            }
+        }
+    }
+";
+            CreateCompilationWithMscorlib45(source).VerifyDiagnostics(
+                // (28,17): error CS0165: Use of unassigned local variable 'obj'
+                //                 obj.ToString();
+                Diagnostic(ErrorCode.ERR_UseDefViolation, "obj").WithArguments("obj").WithLocation(28, 17)
+                );
+        }
     }
 }

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/TryLockUsingStatementTests.cs

@@ -256,7 +256,7 @@ public void TryMethod(out byte para)
             Assert.Equal(0, controlFlowAnalysisResults.ReturnStatements.Count());
             Assert.False(controlFlowAnalysisResults.EndPointIsReachable);
             Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared));
-            Assert.Equal("by", GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
+            Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
             Assert.Equal("by", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsIn));
             Assert.Equal("by", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut));
             Assert.Equal("by", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside));

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

@@ -4889,10 +4889,13 @@ static void Main(string[] args)
                 VerifyDiagnostics(
                 // (9,26): error CS0157: Control cannot leave the body of a finally clause
                 //         finally { i = 3; goto lab1; }// invalid
-                Diagnostic(ErrorCode.ERR_BadFinallyLeave, "goto"),
+                Diagnostic(ErrorCode.ERR_BadFinallyLeave, "goto").WithLocation(9, 26),
+                // (10,5): warning CS0162: Unreachable code detected
+                //     lab1:
+                Diagnostic(ErrorCode.WRN_UnreachableCode, "lab1").WithLocation(10, 5),
                 // (6,13): warning CS0219: The variable 'i' is assigned but its value is never used
                 //         int i = 0;
-                Diagnostic(ErrorCode.WRN_UnreferencedVarAssg, "i").WithArguments("i")
+                Diagnostic(ErrorCode.WRN_UnreferencedVarAssg, "i").WithArguments("i").WithLocation(6, 13)
                 );
         }
 

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenAsyncTests.vb

@@ -9145,6 +9145,46 @@ False
             CompileAndVerify(compilation, expectedOutput:=expectedOutput)
             CompileAndVerify(compilation.WithOptions(TestOptions.ReleaseExe), expectedOutput:=expectedOutput)
         End Sub
+
+        <Fact, WorkItem(19831, "https://github.com/dotnet/roslyn/issues/19831")>
+        Public Sub CaptureAssignedInOuterFinally()
+            Dim source = <compilation name="Async">
+                             <file name="a.vb">
+imports System.Threading.Tasks
+imports System
+
+Module Module1
+
+    Sub Main()
+        Test().Wait()
+        System.Console.WriteLine("success")
+    End Sub
+
+    Async Function Test() As Task
+        Dim obj = New Object()
+
+        Try
+            For i = 0 To 3
+                ' NRE on second iteration
+                obj.ToString()
+                Await Task.Yield()
+            Next
+
+        Finally
+            obj = Nothing
+        End Try
+    End Function
+End Module
+
+                             </file>
+                         </compilation>
+
+            Dim expectedOutput = "success"
+
+            Dim compilation = CompilationUtils.CreateCompilationWithReferences(source, references:=LatestVbReferences, options:=TestOptions.DebugExe)
+            CompileAndVerify(compilation, expectedOutput:=expectedOutput)
+            CompileAndVerify(compilation.WithOptions(TestOptions.ReleaseExe), expectedOutput:=expectedOutput)
+        End Sub
     End Class
 End Namespace