Fix S1854 FP: Value used in finally should LiveIn for all try blocks (#…

…9448)
SonarSource · Jun 20, 2024 · d9774fd · d9774fd
1 parent 33a85fe
commit d9774fd
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 7 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
@@ -88,7 +88,7 @@ private void BuildBranches(BasicBlock block)
                 BuildBranchesFinally(successor.Source, finallyRegion);
             }
         }
-        if (block.IsEnclosedIn(ControlFlowRegionKind.Try))
+        if (block.EnclosingNonLocalLifetimeRegion() is { Kind: ControlFlowRegionKind.Try } tryRegion)
         {
             var catchesAll = false;
             foreach (var catchOrFilterRegion in block.Successors.SelectMany(CatchOrFilterRegions))
@@ -98,7 +98,13 @@ private void BuildBranches(BasicBlock block)
             }
             if (!catchesAll && block.EnclosingRegion(ControlFlowRegionKind.TryAndFinally)?.NestedRegion(ControlFlowRegionKind.Finally) is { } finallyRegion)
             {
-                AddBranch(block, Cfg.Blocks[finallyRegion.FirstBlockOrdinal]);
+                var finallyBlock = Cfg.Blocks[finallyRegion.FirstBlockOrdinal];
+                AddBranch(block, finallyBlock);
+                // We assume that this block can throw in its first operation. Therefore predecessors outside this tryRegion need to be redirected to finally
+                foreach (var predecessor in block.Predecessors.Where(x => x.Source.Ordinal < tryRegion.FirstBlockOrdinal || x.Source.Ordinal > tryRegion.LastBlockOrdinal))
+                {
+                    AddBranch(predecessor.Source, finallyBlock);
+                }
             }
         }
     }

diff --git a/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs b/...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs
@@ -1031,7 +1031,7 @@ public void Finally_AssignedInTry_LiveOut()
             """;
         var context = CreateContextCS(code);
         context.ValidateEntry();
-        context.Validate("Method(0);"/*Should be:, LiveOut("variable")*/);
+        context.Validate("Method(0);", LiveOut("variable"));
         context.Validate("Method(1);", LiveOut("variable"));
         context.Validate("Method(variable);", LiveIn("variable"));
         context.Validate("Method(2);");

diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs
@@ -192,7 +192,7 @@ void calculateRate2(int a, int b)
                 x = 11; // FN, muted due to try/catch
                 x = 12;
                 Console.Write(x);
-                x = 13; // Compliant, Console.Write could throw and value is read fater try/catch
+                x = 13; // FN, muted due to try/catch
             }
             catch (Exception)
             {
@@ -1186,7 +1186,7 @@ public static void DeadStore(int[] array)
                 x = 11;     // FN, muted due to try/catch
                 x = 12;
                 Console.Write(x);
-                x = 13;     // Compliant, Console.Write can throw
+                x = 13;     // FN, muted due to try/catch
             }
             catch (Exception)
             {
@@ -1420,7 +1420,7 @@ public void UsedInFinally_NestedInLambda()
             {
                 Action a = () =>
                 {
-                    int value = 42; // Noncompliant FP
+                    int value = 42; // Compliant
                     try
                     {
                         SomethingThatCanThrow();
@@ -1436,7 +1436,7 @@ public void UsedInFinally_NestedInLambda()
             {
             }
         }
-
+        
         public void UsedInFinally_Throw()
         {
             var value = 42; // Compliant