PR feedback

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

@@ -48,6 +48,10 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
         {
             var oldPending = SavePending(); // we do not allow branches into a try statement
             var initialState = this.State.Clone();
+
+            // use this state to resolve all the branches introduced and internal to try/catch
+            var pendingBeforeTry = SavePending(); 
+
             VisitTryBlock(node.TryBlock, node, ref initialState);
             var finallyState = initialState.Clone();
             var endState = this.State;
@@ -58,19 +62,30 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
                 IntersectWith(ref endState, ref this.State);
             }
 
+            // Give a chance to branches internal to try/catch to resolve.
+            // Carry forward unresolved branches.
+            RestorePending(pendingBeforeTry);
+
+            // NOTE: At this point all branches that are internal to try or catch blocks have been resolved.
+            //       However we have not yet restored the oldPending branches. Therefore all the branches 
+            //       that are currently pending must have been introduced in try/catch and do not terminate inside those blocks.
+            //
+            //       With exception of YieldReturn, these branches logically go through finally, if such present,
+            //       so we must Union/Intersect finally state as appropriate
+
             if (node.FinallyBlockOpt != null)
             {
                 // branches from the finally block, while illegal, should still not be considered
                 // to execute the finally block before occurring.  Also, we do not handle branches
                 // *into* the finally block.
                 SetState(finallyState);
+
+                // capture tryAndCatchPending before going into finally
+                // we will need pending branches as they were before finally later
+                var tryAndCatchPending = SavePending();
                 var unsetInFinally = AllBitsSet();
-                VisitFinallyBlock(node.FinallyBlockOpt, ref unsetInFinally);
-                
-                // 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)
+                VisitFinallyBlock(node.FinallyBlockOpt, ref unsetInFinally);                
+                foreach (var pend in tryAndCatchPending.PendingBranches)
                 {
                     if (pend.Branch == null) continue; // a tracked exception
                     if (pend.Branch.Kind != BoundKind.YieldReturnStatement)
@@ -80,6 +95,7 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
                     }
                 }
 
+                RestorePending(tryAndCatchPending);
                 UnionWith(ref endState, ref this.State);
                 if (trackUnassignments) IntersectWith(ref endState, ref unsetInFinally);
             }
@@ -89,16 +105,13 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
             return null;
         }
 
-        protected virtual void VisitTryBlock(BoundBlock tryBlock, BoundTryStatement node, ref TLocalState tryState)
+        protected virtual void VisitTryBlock(BoundStatement 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);
@@ -111,14 +124,11 @@ protected virtual void VisitCatchBlock(BoundCatchBlock catchBlock, ref TLocalSta
             }
 
             VisitStatement(catchBlock.Body);
-            RestorePending(oldPending);
         }
 
-        protected virtual void VisitFinallyBlock(BoundBlock finallyBlock, ref TLocalState unsetInFinally)
+        protected virtual void VisitFinallyBlock(BoundStatement 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,10 +245,32 @@ private void CheckReachable(BoundStatement statement)
             }
         }
 
-        protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalState endState)
+        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)
         {
             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

@@ -452,7 +452,7 @@ protected IEnumerable<Symbol> GetUnsafeAddressTaken()
             return _unsafeAddressTakenVariables.Keys.ToArray();
         }
 
-#region Tracking reads/writes of variables for warnings
+        #region Tracking reads/writes of variables for warnings
 
         protected virtual void NoteRead(
             Symbol variable,
@@ -708,7 +708,7 @@ private void NoteWrite(BoundExpression n, BoundExpression value, bool read)
             }
         }
 
-#endregion Tracking reads/writes of variables for warnings
+        #endregion Tracking reads/writes of variables for warnings
 
         /// <summary>
         /// Locals are given slots when their declarations are encountered.  We only need give slots
@@ -1458,7 +1458,7 @@ protected override LocalState UnreachableState()
             return result;
         }
 
-#region Visitors
+        #region Visitors
 
         public override void VisitPattern(BoundExpression expression, BoundPattern pattern)
         {
@@ -2031,10 +2031,10 @@ public override BoundNode VisitBaseReference(BoundBaseReference node)
             return null;
         }
 
-#region TryStatements
+        #region TryStatements
         private OptionalState _tryState;
 
-        protected override void VisitTryBlock(BoundBlock tryBlock, BoundTryStatement node, ref LocalState tryState)
+        protected override void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref LocalState tryState)
         {
             if (trackUnassignments)
             {
@@ -2098,7 +2098,7 @@ private void VisitCatchBlockInternal(BoundCatchBlock catchBlock, ref LocalState
             }
         }
 
-        protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalState unsetInFinally)
+        protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref LocalState unsetInFinally)
         {
             if (trackUnassignments)
             {
@@ -2122,7 +2122,7 @@ protected override void VisitFinallyBlock(BoundBlock finallyBlock, ref LocalStat
             }
         }
 
-#endregion TryStatements
+        #endregion TryStatements
 
         public override BoundNode VisitFieldAccess(BoundFieldAccess node)
         {
@@ -2221,7 +2221,7 @@ public override BoundNode VisitDynamicObjectInitializerMember(BoundDynamicObject
             return null;
         }
 
-#endregion Visitors
+        #endregion Visitors
 
         protected override string Dump(LocalState state)
         {

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

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

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(null, GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
+            Assert.Equal("by", 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,13 +4889,10 @@ 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").WithLocation(9, 26),
-                // (10,5): warning CS0162: Unreachable code detected
-                //     lab1:
-                Diagnostic(ErrorCode.WRN_UnreachableCode, "lab1").WithLocation(10, 5),
+                Diagnostic(ErrorCode.ERR_BadFinallyLeave, "goto"),
                 // (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").WithLocation(6, 13)
+                Diagnostic(ErrorCode.WRN_UnreferencedVarAssg, "i").WithArguments("i")
                 );
         }