From ba3012a80a48b2eb9d4276f16c9f90fcd73a3be9 Mon Sep 17 00:00:00 2001 From: Mary Georgiou <89914005+mary-georgiou-sonarsource@users.noreply.github.com> Date: Wed, 11 Dec 2024 16:22:46 +0100 Subject: [PATCH] NET-798 Fix S1854 FN: Support &&, ||, ?? and ??= --- .../RoslynLiveVariableAnalysis.cs | 21 ++++----- ...riableAnalysisTest.FlowCaptureOperation.cs | 45 ++++++++++--------- ...nLiveVariableAnalysisTest.LocalFunction.cs | 26 ++++++++++- .../TestCases/DeadStores.RoslynCfg.cs | 6 +-- 4 files changed, 58 insertions(+), 40 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs index 9591229dee2..b01e1b6b702 100644 --- a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs +++ b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs @@ -71,14 +71,13 @@ protected override IEnumerable Successors(BasicBlock block) => protected override State ProcessBlock(BasicBlock block) { var ret = new RoslynState(this); - ret.ProcessBlock(Cfg, block, flowCaptures); + ret.ProcessBlock(Cfg, block); return ret; } private void ResolveCaptures() { foreach (var flowCapture in Cfg.Blocks - .Where(x => x.EnclosingRegion.EnclosingRegionOrSelf(ControlFlowRegionKind.LocalLifetime) is not null) .SelectMany(x => x.OperationsAndBranchValue) .ToExecutionOrder() .Where(x => x.Instance.Kind == OperationKindEx.FlowCapture) @@ -241,18 +240,11 @@ private sealed class RoslynState : State public RoslynState(RoslynLiveVariableAnalysis owner) => this.owner = owner; - public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block, Dictionary> flowCaptureOperations) + public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block) { foreach (var operation in block.OperationsAndBranchValue.ToReversedExecutionOrder().Select(x => x.Instance)) { - if (operation.AsFlowCaptureReference() is { } flowCaptureReference && flowCaptureOperations.TryGetValue(flowCaptureReference.Id, out var symbols)) - { - UsedBeforeAssigned.UnionWith(symbols); - } - else - { - ProcessOperation(cfg, operation); - } + ProcessOperation(cfg, operation); } } @@ -267,6 +259,9 @@ private void ProcessOperation(ControlFlowGraph cfg, IOperation operation) case OperationKindEx.ParameterReference: ProcessParameterOrLocalReference(IParameterReferenceOperationWrapper.FromOperation(operation)); break; + case OperationKindEx.FlowCaptureReference: + ProcessParameterOrLocalReference(IFlowCaptureReferenceOperationWrapper.FromOperation(operation)); + break; case OperationKindEx.SimpleAssignment: ProcessSimpleAssignment(ISimpleAssignmentOperationWrapper.FromOperation(operation)); break; @@ -290,7 +285,7 @@ private void ProcessParameterOrLocalReference(IOperationWrapper reference) Assigned.UnionWith(symbols); UsedBeforeAssigned.ExceptWith(symbols); } - else if (!reference.IsAssignmentTarget()) + else if (!reference.IsAssignmentTarget() && reference.ToSonar().Parent?.Kind != OperationKindEx.FlowCapture) { UsedBeforeAssigned.UnionWith(symbols); } @@ -355,7 +350,7 @@ private void ProcessLocalFunction(ControlFlowGraph cfg, IMethodSymbol method) var localFunctionCfg = cfg.FindLocalFunctionCfgInScope(localFunction, owner.Cancel); foreach (var block in localFunctionCfg.Blocks.Reverse()) // Simplified approach, ignoring branching and try/catch/finally flows { - ProcessBlock(localFunctionCfg, block, []); + ProcessBlock(localFunctionCfg, block); } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs index aa368728912..3b248ece93e 100644 --- a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs +++ b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs @@ -37,8 +37,8 @@ public void FlowCaptrure_NullCoalescingAssignment() var context = CreateContextCS(code, additionalParameters: "string param"); context.ValidateEntry(LiveIn("param"), LiveOut("param")); context.Validate(context.Cfg.Blocks[1], LiveIn("param"), LiveOut("param")); - context.Validate(context.Cfg.Blocks[2], LiveIn("param"), LiveOut("param")); - context.Validate(context.Cfg.Blocks[3], LiveIn("param")); + context.Validate(context.Cfg.Blocks[2], LiveIn("param")); + context.Validate(context.Cfg.Blocks[3]); context.ValidateExit(); } @@ -210,20 +210,20 @@ public void FlowCaptrure_NullCoalescingOperator_ConsequentCalls_Assignment() { const string code = """var result = s1 ??= s2 = s3 ??= s4 ?? "End";"""; var context = CreateContextCS(code, additionalParameters: "string s1, string s2, string s3, string s4"); - context.ValidateEntry(LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); - context.Validate(context.Cfg.Blocks[1], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 1: #0=s1 - context.Validate(context.Cfg.Blocks[2], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 2: #1=#0; if #1 is null - context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1")); // 3: F: #2=#1 - context.Validate(context.Cfg.Blocks[4], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 4: T: #3=s2 - context.Validate(context.Cfg.Blocks[5], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 5: #4=s3 - context.Validate(context.Cfg.Blocks[6], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 6: #5=#4; if #5 is null - context.Validate(context.Cfg.Blocks[7], LiveIn("s1", "s2", "s3"), LiveOut("s1", "s2", "s3")); // 7: F: #6=#5 - context.Validate(context.Cfg.Blocks[8], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 8: T: #7=s4; if #7 is null - context.Validate(context.Cfg.Blocks[9], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 9: F: #8=#7 - context.Validate(context.Cfg.Blocks[10], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 10: #7=null; #8="End" - context.Validate(context.Cfg.Blocks[11], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3")); // 11: #6= (#4=#8) - context.Validate(context.Cfg.Blocks[12], LiveIn("s1", "s2", "s3"), LiveOut("s1")); // 12: #2= (#0 = (#3=#6) ) - context.Validate(context.Cfg.Blocks[13], LiveIn("s1")); // 13: result=#2 + context.ValidateEntry(LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4")); + context.Validate(context.Cfg.Blocks[1], LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4")); // 1: #0=s1 + context.Validate(context.Cfg.Blocks[2], LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4")); // 2: #1=#0; if #1 is null + context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1")); // 3: F: #2=#1 + context.Validate(context.Cfg.Blocks[4], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 4: T: #3=s2 + context.Validate(context.Cfg.Blocks[5], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 5: #4=s3 + context.Validate(context.Cfg.Blocks[6], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 6: #5=#4; if #5 is null + context.Validate(context.Cfg.Blocks[7], LiveIn("s3"), LiveOut("s3")); // 7: F: #6=#5 + context.Validate(context.Cfg.Blocks[8], LiveIn("s4"), LiveOut("s4")); // 8: T: #7=s4; if #7 is null + context.Validate(context.Cfg.Blocks[9], LiveIn("s4"), LiveOut("s4")); // 9: F: #8=#7 + context.Validate(context.Cfg.Blocks[10], LiveIn("s4"), LiveOut("s4")); // 10: #7=null; #8="End" + context.Validate(context.Cfg.Blocks[11], LiveIn("s4"), LiveOut("s3")); // 11: #6= (#4=#8) + context.Validate(context.Cfg.Blocks[12], LiveIn("s3"), LiveOut("s1")); // 12: #2= (#0 = (#3=#6) ) + context.Validate(context.Cfg.Blocks[13], LiveIn("s1")); // 13: result=#2 context.ValidateExit(); } @@ -297,12 +297,13 @@ public void FlowCaptrure_NullCoalescingOperator_Overwrite() */ const string code = """s1 = (s1 = "overwrite") ?? "value";"""; var context = CreateContextCS(code, additionalParameters: "string s1"); - context.ValidateEntry(LiveIn("s1"), LiveOut("s1")); - context.Validate(context.Cfg.Blocks[1], LiveIn("s1")); - context.Validate(context.Cfg.Blocks[2], LiveOut("s1")); // This should have LiveIn("s1") and LiveOut("s1") but #1 gets as value all the assignment operation. - context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1")); - context.Validate(context.Cfg.Blocks[4], LiveIn("s1"), LiveOut("s1")); - context.Validate(context.Cfg.Blocks[5], LiveIn("s1")); + // s1 is never read. The assignment returns its r-value, which is used for further calculation. + context.ValidateEntry(); + context.Validate(context.Cfg.Blocks[1]); + context.Validate(context.Cfg.Blocks[2]); + context.Validate(context.Cfg.Blocks[3]); + context.Validate(context.Cfg.Blocks[4]); + context.Validate(context.Cfg.Blocks[5]); context.ValidateExit(); } diff --git a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs index c2e141582d1..9589e40d0d9 100644 --- a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs +++ b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs @@ -221,7 +221,7 @@ void LocalFunction() } [TestMethod] - public void LocalFunctionInvocation_Recursive_LiveIn() + public void LocalFunctionInvocation_Recursive_LiveIn_WithoutFlowCapture() { var code = """ var variable = 42; @@ -229,7 +229,12 @@ public void LocalFunctionInvocation_Recursive_LiveIn() return; LocalFunction(10); - int LocalFunction(int cnt) => variable + (cnt == 0 ? 0 : LocalFunction(cnt - 1)); + int LocalFunction(int cnt) + { + if (cnt == 0) + return 0; + return variable + LocalFunction(cnt - 1); + } """; var context = CreateContextCS(code); context.ValidateEntry(LiveIn("boolParameter"), LiveOut("boolParameter")); @@ -237,6 +242,23 @@ public void LocalFunctionInvocation_Recursive_LiveIn() context.Validate("LocalFunction(10);", LiveIn("variable")); } + [TestMethod] + public void LocalFunctionInvocation_Recursive_LiveIn_FlowCapture() + { + var code = """ + var variable = 42; + if (boolParameter) + return; + LocalFunction(10); + + int LocalFunction(int cnt) => variable + (cnt == 0 ? 0 : LocalFunction(cnt - 1)); + """; + var context = CreateContextCS(code); + context.ValidateEntry(LiveIn("boolParameter"), LiveOut("boolParameter")); + context.Validate("boolParameter", LiveIn("boolParameter")); // should be LiveOut("variable") but FlowCaptures inside of local functions are not resolved + context.Validate("LocalFunction(10);"); // should be LiveIn("variable") + } + [TestMethod] public void LocalFunctionInvocation_Recursive_WhenAnalyzingLocalFunctionItself_LiveIn() { diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs index edf2932a50c..73443f06b5c 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs @@ -648,8 +648,8 @@ public void Unused() private void ConditionalEvaluation(bool b1, bool b2, object coalesce, object coalesceAssignment) { var x = false; // Compliant ignored value - x = true; // Roslyn CFG FN: Consequence of inaccurate LVA state below - x = b1 && b2; // Roslyn CFG FN: Branching with FlowCaptureOperation + x = true; // Noncompliant + x = b1 && b2; // Noncompliant x = b1 || b2; // Noncompliant coalesce = coalesce ?? "Value"; // Noncompliant coalesceAssignment ??= "Value"; // Noncompliant @@ -937,7 +937,7 @@ class ObjectInitializer void Method() { - var x = new ObjectInitializer(); // FN + var x = new ObjectInitializer(); // Noncompliant x = new ObjectInitializer { ID = 1 }; x.Method(); }