From 95b89ace1eb0447614d99c7bc2bc918e39a7885c Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Fri, 2 Aug 2024 14:04:22 +0200 Subject: [PATCH] Revert "Fix S1854 FN: Support &&, ||, ?? and ??= (#9536)" This reverts commit 2c9c49a326b620ed7c8397e61a3fd59fcad89697. --- .../S1854-CSharpLatest-net8.0.json | 6 --- .../RoslynLiveVariableAnalysis.cs | 20 +++++---- ...riableAnalysisTest.FlowCaptureOperation.cs | 45 +++++++++---------- ...nLiveVariableAnalysisTest.LocalFunction.cs | 24 +--------- .../TestCases/DeadStores.RoslynCfg.cs | 15 +------ 5 files changed, 37 insertions(+), 73 deletions(-) diff --git a/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json b/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json index b4881f2675a..9aec1f2025d 100644 --- a/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json +++ b/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json @@ -1,11 +1,5 @@ { "Issues": [ - { - "Id": "S1854", - "Message": "Remove this useless assignment to local variable \u0027person\u0027.", - "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp11Features/RequiredMembers.cs#L19", - "Location": "Line 19 Position 13-40" - }, { "Id": "S1854", "Message": "Remove this useless assignment to local variable \u0027person\u0027.", diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs index 838930a8141..cc467b52fa2 100644 --- a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs +++ b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs @@ -75,7 +75,7 @@ protected override IEnumerable Successors(BasicBlock block) => protected override State ProcessBlock(BasicBlock block) { var ret = new RoslynState(this); - ret.ProcessBlock(Cfg, block); + ret.ProcessBlock(Cfg, block, flowCaptures); return ret; } @@ -245,11 +245,18 @@ private sealed class RoslynState : State public RoslynState(RoslynLiveVariableAnalysis owner) => this.owner = owner; - public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block) + public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block, Dictionary> flowCaptureOperations) { foreach (var operation in block.OperationsAndBranchValue.ToReversedExecutionOrder().Select(x => x.Instance)) { - ProcessOperation(cfg, operation); + if (operation.AsFlowCaptureReference() is { } flowCaptureReference && flowCaptureOperations.TryGetValue(flowCaptureReference.Id, out var symbols)) + { + UsedBeforeAssigned.UnionWith(symbols); + } + else + { + ProcessOperation(cfg, operation); + } } } @@ -264,9 +271,6 @@ 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 +294,7 @@ private void ProcessParameterOrLocalReference(IOperationWrapper reference) Assigned.UnionWith(symbols); UsedBeforeAssigned.ExceptWith(symbols); } - else if (!reference.IsAssignmentTarget() && reference.ToSonar().Parent?.Kind != OperationKindEx.FlowCapture) + else if (!reference.IsAssignmentTarget()) { UsedBeforeAssigned.UnionWith(symbols); } @@ -355,7 +359,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 5415b029630..272eed969e3 100644 --- a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs +++ b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs @@ -41,8 +41,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")); - context.Validate(context.Cfg.Blocks[3]); + context.Validate(context.Cfg.Blocks[2], LiveIn("param"), LiveOut("param")); + context.Validate(context.Cfg.Blocks[3], LiveIn("param")); context.ValidateExit(); } @@ -214,20 +214,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", "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.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.ValidateExit(); } @@ -301,13 +301,12 @@ public void FlowCaptrure_NullCoalescingOperator_Overwrite() */ const string code = """s1 = (s1 = "overwrite") ?? "value";"""; var context = CreateContextCS(code, additionalParameters: "string 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.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")); context.ValidateExit(); } diff --git a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs index 609515ec0a3..80ee21068bc 100644 --- a/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs +++ b/analyzers/tests/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs @@ -233,12 +233,7 @@ public void LocalFunctionInvocation_Recursive_LiveIn() return; LocalFunction(10); - int LocalFunction(int cnt) - { - if (cnt == 0) - return 0; - return variable + LocalFunction(cnt - 1); - } + int LocalFunction(int cnt) => variable + (cnt == 0 ? 0 : LocalFunction(cnt - 1)); """; var context = CreateContextCS(code); context.ValidateEntry(LiveIn("boolParameter"), LiveOut("boolParameter")); @@ -246,23 +241,6 @@ int LocalFunction(int cnt) context.Validate("LocalFunction(10);", LiveIn("variable")); } - [TestMethod] - public void LocalFunctionInvocation_Recursive_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 e7f56fbfff1..507318064f0 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs @@ -648,22 +648,11 @@ public void Unused() private void ConditionalEvaluation(bool b1, bool b2, object coalesce, object coalesceAssignment) { var x = false; // Compliant ignored value - x = true; // Noncompliant - x = b1 && b2; // Noncompliant + x = true; // Roslyn CFG FN: Consequence of inaccurate LVA state below + x = b1 && b2; // Roslyn CFG FN: Branching with FlowCaptureOperation x = b1 || b2; // Noncompliant coalesce = coalesce ?? "Value"; // Noncompliant coalesceAssignment ??= "Value"; // Noncompliant - - DeadStores lst; - lst = new DeadStores // Noncompliant - { - Property = 42 - }; - lst = new DeadStores - { - Property = 42 - }; - lst.ToString(); } private void SimpleAssignment()