NET-868 Fix S1854 FP: Don't raise when inner finally assignment is us…

…ed to outer block
SonarSource · Dec 17, 2024 · af3f528 · af3f528
1 parent 852b0ee
commit af3f528
Show file tree

Hide file tree

Showing 3 changed files with 268 additions and 7 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
@@ -144,10 +144,14 @@ private void BuildBranches(BasicBlock block)
                 AddPredecessorsOutsideRegion(finallyBlock);
             }
         }
-        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) || block.IsEnclosedIn(ControlFlowRegionKind.FilterAndHandler))
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) || block.IsEnclosedIn(ControlFlowRegionKind.Filter))
         {
             BuildBranchesCatch(block);
         }
+        if (block.EnclosingNonLocalLifetimeRegion() is { Kind: ControlFlowRegionKind.Finally })
+        {
+            BuildBranchesToOuterCatch(block, block.EnclosingNonLocalLifetimeRegion().EnclosingRegion);
+        }
 
         void AddPredecessorsOutsideRegion(BasicBlock destination)
         {
@@ -165,9 +169,16 @@ private void BuildBranchesCatch(BasicBlock source)
         {
             BuildBranchesRethrow(source);
         }
-        else if (source.EnclosingRegion(ControlFlowRegionKind.TryAndCatch) is { } innerTryCatch
-            && innerTryCatch.EnclosingRegion(ControlFlowRegionKind.Try) is { } outerTry
-            && outerTry.EnclosingRegion(ControlFlowRegionKind.TryAndCatch) is { } outerTryCatch)
+        else if (source.EnclosingRegion(ControlFlowRegionKind.TryAndCatch) is { } innerTryCatch)
+        {
+            BuildBranchesToOuterCatch(source, innerTryCatch);
+        }
+    }
+
+    private void BuildBranchesToOuterCatch(BasicBlock source, ControlFlowRegion region)
+    {
+        if (region.EnclosingRegion(ControlFlowRegionKind.Try) is { } outerTry
+           && outerTry.EnclosingRegion(ControlFlowRegionKind.TryAndCatch) is { } outerTryCatch)
         {
             foreach (var outerCatch in CatchOrFilterRegions(outerTryCatch))
             {

diff --git a/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs b/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs
@@ -285,7 +285,7 @@ public void NestedCatch_LiveOutOuterCatch_ForEach()
     }
 
     [TestMethod]
-    public void NestedCatch_LiveOuterFilterHandler_FromInnerCatch()
+    public void NestedCatch_LiveOutOuterFilterHandler_FromInnerCatch()
     {
         const string code = """
             int usedInCatch = 0;
@@ -395,18 +395,24 @@ public void NestedCatch_LiveOutOuterCatch_CanThrowFromFilterHandler()
                 {
                     Method(1);
                 }
-                catch when(Method(2) == usedInCatch) {  }
+                catch when(Method(2) == usedInCatch)
+                {
+                    usedInCatch = 1;
+                    Method(3);
+                }
             }
             catch
             {
                 Method(usedInCatch);
-                Method(3);
+                Method(4);
             }
             """;
         var context = CreateContextCS(code);
         context.Validate("Method(0);", LiveOut("usedInCatch"));
         context.Validate("Method(1);", LiveIn("usedInCatch"), LiveOut("usedInCatch"));
         context.Validate("Method(2) == usedInCatch", LiveIn("usedInCatch"), LiveOut("usedInCatch"));
+        context.Validate("Method(3);", LiveOut("usedInCatch"));
+        context.Validate("Method(4);", LiveIn("usedInCatch"));
         context.ValidateExit();
     }
 
@@ -1394,6 +1400,220 @@ public void Finally_Nested_NoInstructionBetweenFinally_LiveIn()
         context.ValidateExit();
     }
 
+    [TestMethod]
+    public void NestedFinally_LiveOut_OuterCatch()
+    {
+        const string code = """
+            var usedInOuterCatch = 0;
+            Method(0);
+            try
+            {
+                Method(1);
+                try
+                {
+                    Method(2);
+                }
+                finally
+                {
+                    usedInOuterCatch = 2;
+                    Method(3);
+                }
+            }
+            catch (Exception ex)
+            {
+                Method(usedInOuterCatch);
+                Method(4);
+            }
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("usedInOuterCatch"));
+        context.Validate("Method(1);", LiveIn("usedInOuterCatch"), LiveOut("usedInOuterCatch"));
+        context.Validate("Method(2);", LiveIn("usedInOuterCatch"), LiveOut("usedInOuterCatch"));
+        context.Validate("Method(3);", LiveOut("usedInOuterCatch"));
+        context.Validate("Method(4);", LiveIn("usedInOuterCatch"));
+        context.ValidateExit();
+    }
+
+    [TestMethod]
+    public void NestedFinally_LiveOut_NestedTryInOuterCatch()
+    {
+        const string code = """
+            var usedInOuterTry = 0;
+            Method(0);
+            try
+            {
+                Method(1);
+                try
+                {
+                    Method(2);
+                }
+                finally
+                {
+                    usedInOuterTry = 2;
+                    Method(3);
+                }
+            }
+            catch
+            {
+                try
+                {
+                    Method(usedInOuterTry);
+                    Method(4);
+                }
+                catch
+                {
+                    Method(5);
+                }
+            }
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("usedInOuterTry"));
+        context.Validate("Method(1);", LiveIn("usedInOuterTry"), LiveOut("usedInOuterTry"));
+        context.Validate("Method(2);", LiveIn("usedInOuterTry"), LiveOut("usedInOuterTry"));
+        context.Validate("Method(3);", LiveOut("usedInOuterTry"));
+        context.Validate("Method(4);", LiveIn("usedInOuterTry"));
+        context.Validate("Method(5);");
+        context.ValidateExit();
+    }
+
+    [TestMethod]
+    public void NestedFinally_LiveOut_NestedFinallyInOuterCatch()
+    {
+        const string code = """
+            var usedInOuterFinally = 0;
+            Method(0);
+            try
+            {
+                Method(usedInOuterFinally);
+                Method(1);
+                try
+                {
+                    Method(2);
+                }
+                finally
+                {
+                    usedInOuterFinally = 2; // Compliant - used in outer finally
+                    Method(3);
+                }
+            }
+            catch
+            {
+                try
+                {
+                    Method(4);
+                }
+                finally
+                {
+                    Method(usedInOuterFinally);
+                    Method(5);
+                }
+            }
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("usedInOuterFinally"));
+        context.Validate("Method(1);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(2);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(3);", LiveOut("usedInOuterFinally"));
+        context.Validate("Method(4);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(5);", LiveIn("usedInOuterFinally"));
+        context.ValidateExit();
+    }
+
+    [TestMethod]
+    public void NestedFinally_LiveOut_NestedFinallyInOuter_ConsecutiveCatch()
+    {
+        const string code = """
+            var usedInOuterFinally = 0;
+            Method(0);
+            try
+            {
+                Method(usedInOuterFinally);
+                Method(1);
+                try
+                {
+                    Method(2);
+                }
+                finally
+                {
+                    usedInOuterFinally = 2; // Compliant - used in outer finally
+                    Method(3);
+                }
+            }
+            catch(NotImplementedException)
+            {
+                Method(4);
+            }
+            catch
+            {
+                try
+                {
+                    Method(5);
+                }
+                finally
+                {
+                    Method(6);
+                    Method(usedInOuterFinally);
+                }
+            }
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("usedInOuterFinally"));
+        context.Validate("Method(1);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(2);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(3);", LiveOut("usedInOuterFinally"));
+        context.Validate("Method(4);");
+        context.Validate("Method(5);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(6);", LiveIn("usedInOuterFinally"));
+        context.ValidateExit();
+    }
+
+    [TestMethod]
+    public void NestedFinally_LiveOutNestedFinallyInOuter_FinallyHasLocalLifetime()
+    {
+        const string code = """
+            var usedInOuterFinally = 0;
+            Method(0);
+            try
+            {
+                Method(usedInOuterFinally);
+                Method(1);
+                try
+                {
+                    Method(2);
+                }
+                finally
+                {
+                    var t = usedInOuterFinally  > 1 ? 1 : 0; // This causes LocalLifetimeRegion to be generated
+                }
+            }
+            catch
+            {
+                try
+                {
+                    Method(4);
+                }
+                finally
+                {
+                    Method(5);
+                    Method(usedInOuterFinally);
+                }
+            }
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("usedInOuterFinally"));
+        context.Validate("Method(1);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(2);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("t = usedInOuterFinally  > 1 ? 1 : 0", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(4);", LiveIn("usedInOuterFinally"), LiveOut("usedInOuterFinally"));
+        context.Validate("Method(5);", LiveIn("usedInOuterFinally"));
+        context.ValidateExit();
+    }
+
     [TestMethod]
     public void Finally_ForEach_LiveIn()
     {

diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs
@@ -1815,5 +1815,35 @@ void VariableReassignedInCatch()
         }
     }
 
+    void VariableUsedInOuterFinally(bool condition)
+    {
+        var usedInOuterFinally = 0;
+        try
+        {
+            usedInOuterFinally = 1;
+            Method(0);
+            try
+            {
+                Method(1);
+            }
+            finally
+            {
+                usedInOuterFinally = 2; // Compliant - used in outer finally
+                Method(2);
+            }
+        }
+        catch (Exception ex)
+        {
+            try
+            {
+                Method(3);
+            }
+            finally
+            {
+                Method(usedInOuterFinally);
+            }
+        }
+    }
+
     void Method(int arg) { }
 }