From 20f1cd20e5984630237194f94983b003dbd02971 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 20 Jun 2024 13:09:17 +0200 Subject: [PATCH 1/5] Fix S1854 FP: Value used in `catch` or `when` should LiveIn for all try blocks --- .../RoslynLiveVariableAnalysis.cs | 20 +++++++++---- ...iveVariableAnalysisTest.TryCatchFinally.cs | 28 ++++++++++++++++++- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs index 7ad0b47f127..4ba01f9227c 100644 --- a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs +++ b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs @@ -18,6 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using System.Runtime.CompilerServices; using SonarAnalyzer.CFG.Roslyn; namespace SonarAnalyzer.CFG.LiveVariableAnalysis; @@ -93,18 +94,25 @@ private void BuildBranches(BasicBlock block) var catchesAll = false; foreach (var catchOrFilterRegion in block.Successors.SelectMany(CatchOrFilterRegions)) { - AddBranch(block, Cfg.Blocks[catchOrFilterRegion.FirstBlockOrdinal]); + var catchOrFilterBlock = Cfg.Blocks[catchOrFilterRegion.FirstBlockOrdinal]; + AddBranch(block, catchOrFilterBlock); + AddPredecessorsOutsideRegion(catchOrFilterBlock); catchesAll = catchesAll || (catchOrFilterRegion.Kind == ControlFlowRegionKind.Catch && IsCatchAllType(catchOrFilterRegion.ExceptionType)); } if (!catchesAll && block.EnclosingRegion(ControlFlowRegionKind.TryAndFinally)?.NestedRegion(ControlFlowRegionKind.Finally) is { } finallyRegion) { 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); - } + AddPredecessorsOutsideRegion(finallyBlock); + } + } + + void AddPredecessorsOutsideRegion(BasicBlock destination) + { + // We assume that current block can throw in its first operation. Therefore predecessors outside this tryRegion need to be redirected to catch/filter/finally + foreach (var predecessor in block.Predecessors.Where(x => x.Source.Ordinal < tryRegion.FirstBlockOrdinal || x.Source.Ordinal > tryRegion.LastBlockOrdinal)) + { + AddBranch(predecessor.Source, destination); } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs index 677b4356a37..7fc945daedf 100644 --- a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs +++ b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs @@ -538,12 +538,38 @@ public void Catch_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);"); } + [TestMethod] + public void Catch_When_AssignedInTry_LiveOut() + { + var code = """ + int variable = 42; + Method(0); + try + { + Method(1); // Can throw + variable = 0; + } + catch when(variable == 0) + { + Method(2); + } + Method(3); + """; + var context = CreateContextCS(code); + context.ValidateEntry(); + context.Validate("Method(0);", LiveOut("variable")); + context.Validate("Method(1);", LiveOut("variable")); + context.Validate("variable == 0", LiveIn("variable")); + context.Validate("Method(2);"); + context.Validate("Method(3);"); + } + [TestMethod] public void Catch_Loop_Propagates_LiveIn() { From c4bdaed11169fbb5353105c62ebec144e88454aa Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 20 Jun 2024 14:24:47 +0200 Subject: [PATCH 2/5] Update FN UT --- .../SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs index d9941aef24e..325c9cf52f5 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs @@ -4166,9 +4166,9 @@ void Method() exception = e; } } - if (exception != null) // FN! + if (exception != null) // Noncompliant { - Console.WriteLine(exception); + Console.WriteLine(exception); // Secondary } Console.WriteLine(success); } From 54aca014a9eb0b1911ebae3a672ba9dbf77833b3 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 20 Jun 2024 14:24:52 +0200 Subject: [PATCH 3/5] Update ITs --- .../its/expected/Ember-MM/S2259-EmberAPI.json | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/analyzers/its/expected/Ember-MM/S2259-EmberAPI.json b/analyzers/its/expected/Ember-MM/S2259-EmberAPI.json index e01b6146380..c6191e8f63d 100644 --- a/analyzers/its/expected/Ember-MM/S2259-EmberAPI.json +++ b/analyzers/its/expected/Ember-MM/S2259-EmberAPI.json @@ -36,6 +36,24 @@ "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/Ember-MM/EmberAPI/clsAPIDatabase.vb#L280", "Location": "Line 280 Position 61-73" }, + { + "Id": "S2259", + "Message": "\u0027inList\u0027 is Nothing on at least one execution path.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/Ember-MM/EmberAPI/clsAPIScanner.vb#L1022", + "Location": "Line 1022 Position 60-66" + }, + { + "Id": "S2259", + "Message": "\u0027dList\u0027 is Nothing on at least one execution path.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/Ember-MM/EmberAPI/clsAPIScanner.vb#L926", + "Location": "Line 926 Position 52-57" + }, + { + "Id": "S2259", + "Message": "\u0027inList\u0027 is Nothing on at least one execution path.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/Ember-MM/EmberAPI/clsAPIScanner.vb#L995", + "Location": "Line 995 Position 56-62" + }, { "Id": "S2259", "Message": "\u0027movieName\u0027 is Nothing on at least one execution path.", From c5f8b6b8c87843be1c6b8ce025dac4886e156ce6 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 20 Jun 2024 15:15:40 +0200 Subject: [PATCH 4/5] Cleanup --- .../LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs index 4ba01f9227c..46148734a86 100644 --- a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs +++ b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs @@ -18,7 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -using System.Runtime.CompilerServices; using SonarAnalyzer.CFG.Roslyn; namespace SonarAnalyzer.CFG.LiveVariableAnalysis; From d01f05c01449a9f9c179f0ebb61b01e98bcdd418 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 20 Jun 2024 21:40:26 +0200 Subject: [PATCH 5/5] Review update: Space invader --- .../tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs index 04b6f8d46d6..041deae240f 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs @@ -1436,7 +1436,7 @@ public void UsedInFinally_NestedInLambda() { } } - + public void UsedInFinally_Throw() { var value = 42; // Compliant