Fix S1854 FP: Value used in finally should LiveIn after throw

SonarSource · Jun 17, 2024 · 6ad0010 · 6ad0010
1 parent 93d54f7
commit 6ad0010
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 3 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
@@ -90,9 +90,15 @@ private void BuildBranches(BasicBlock block)
         }
         if (block.IsEnclosedIn(ControlFlowRegionKind.Try))
         {
+            var catchesAll = false;
             foreach (var catchOrFilterRegion in block.Successors.SelectMany(CatchOrFilterRegions))
             {
                 AddBranch(block, Cfg.Blocks[catchOrFilterRegion.FirstBlockOrdinal]);
+                catchesAll = catchesAll || (catchOrFilterRegion.Kind == ControlFlowRegionKind.Catch && IsCatchAllType(catchOrFilterRegion.ExceptionType));
+            }
+            if (!catchesAll && block.EnclosingRegion(ControlFlowRegionKind.TryAndFinally)?.NestedRegion(ControlFlowRegionKind.Finally) is { } finallyRegion)
+            {
+                AddBranch(block, Cfg.Blocks[finallyRegion.FirstBlockOrdinal]);
             }
         }
     }
@@ -141,6 +147,10 @@ private static IEnumerable<ControlFlowRegion> CatchOrFilterRegions(ControlFlowRe
         }
     }
 
+    private static bool IsCatchAllType(ITypeSymbol exceptionType) =>
+        exceptionType.SpecialType == SpecialType.System_Object  // catch { ... }
+        || exceptionType is { ContainingNamespace: { Name: nameof(System), ContainingNamespace.IsGlobalNamespace: true }, Name: nameof(Exception) };  // catch(Exception) { ... }
+
     private static ISymbol OriginalDeclaration(IOperation originalOperation)
     {
         if (originalOperation.IsAnyKind(OperationKindEx.MethodBody, OperationKindEx.Block, OperationKindEx.ConstructorBody))

diff --git a/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs b/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs
@@ -742,12 +742,74 @@ public void Finally_WithThrowStatementInTry_LiveOut()
             """;
         var context = CreateContextCS(code);
         context.ValidateEntry();
-        context.Validate("Method(0);"/*Should be:, LiveOut("variable")*/);
-        context.Validate("Method(1);"/*Should be:, LiveIn("variable"), LiveOut("variable")*/);
+        context.Validate("Method(0);", LiveOut("variable"));
+        context.Validate("Method(1);", LiveIn("variable"), LiveOut("variable"));
         context.Validate("Method(variable);", LiveIn("variable"));
         context.Validate("Method(2);");
     }
 
+    [TestMethod]
+    public void Finally_WithThrowStatementInTry_LiveOut_FilteredCatch()
+    {
+        var code = """
+            int variable = 42;
+            Method(0);
+            try
+            {
+                Method(1);
+                throw new Exception();
+            }
+            catch when (Property == 42) { }     // FilterAndHandlerRegion
+            catch (FormatException) { }
+            catch (ArgumentException ex) { }
+            finally
+            {
+                Method(variable);
+            }
+            Method(2);
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);", LiveOut("variable"));
+        context.Validate("Method(1);", LiveIn("variable"), LiveOut("variable"));
+        context.Validate("Method(variable);", LiveIn("variable"));
+        context.Validate("Method(2);");
+    }
+
+    [DataTestMethod]
+    [DataRow("catch")]
+    [DataRow("catch (Exception)")]
+    [DataRow("catch (Exception ex)")]
+    public void Finally_WithThrowStatementInTry_LiveOut_CatchAll(string catchAll)
+    {
+        var code = $$"""
+            int variable = 42;
+            Method(0);
+            try
+            {
+                Method(1);
+                throw new Exception();
+            }
+            {{catchAll}}
+            {
+                Method(2);
+                variable = 0; 
+            }
+            finally
+            {
+                Method(variable);
+            }
+            Method(3);
+            """;
+        var context = CreateContextCS(code);
+        context.ValidateEntry();
+        context.Validate("Method(0);");
+        context.Validate("Method(1);");
+        context.Validate("Method(2);", LiveOut("variable"));
+        context.Validate("Method(variable);", LiveIn("variable"));
+        context.Validate("Method(3);");
+    }
+
     [TestMethod]
     public void Finally_NotLiveIn_NotLiveOut()
     {

diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs
@@ -1398,6 +1398,8 @@ public static void CreateDirectory(string directory)
     // https://github.com/SonarSource/sonar-dotnet/issues/4948
     public class Repro_4948
     {
+        private bool condition;
+
         public void UsedInFinally()
         {
             int value = 42; // Compliant, Muted
@@ -1437,7 +1439,7 @@ public void UsedInFinally_NestedInLambda()
 
         public void UsedInFinally_Throw()
         {
-            var value = 42; // Noncompliant FP related to LVA
+            var value = 42; // Compliant 
             try
             {
                 throw new Exception();
@@ -1448,6 +1450,44 @@ public void UsedInFinally_Throw()
             }
         }
 
+        public void UsedInFinally_Throw_FilteredCatch()
+        {
+            var value = 42; // Compliant 
+            try
+            {
+                throw new Exception();
+            }
+            catch (FormatException)
+            {
+                value = 42;
+            }
+            catch when (condition)
+            {
+                value = 42;
+            }
+            finally
+            {
+                Use(value);
+            }
+        }
+
+        public void UsedInFinally_Throw_CatchAll()
+        {
+            var value = 42; // FN
+            try
+            {
+                throw new Exception();
+            }
+            catch
+            {
+                value = 0;
+            }
+            finally
+            {
+                Use(value);
+            }
+        }
+
         private void SomethingThatCanThrow() { }
         private void Use(int arg) { }
     }