Fix S1854 FN: Support &&, ||, ?? and ??= (#9536)

SonarSource · Jul 30, 2024 · 2c9c49a · 2c9c49a
1 parent 8bb1030
commit 2c9c49a
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 37 deletions.
diff --git a/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json b/analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json
@@ -1,5 +1,11 @@
 {
   "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
@@ -75,7 +75,7 @@ protected override IEnumerable<BasicBlock> 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;
     }
 
@@ -245,18 +245,11 @@ private sealed class RoslynState : State
         public RoslynState(RoslynLiveVariableAnalysis owner) =>
             this.owner = owner;
 
-        public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block, Dictionary<CaptureId, List<ISymbol>> 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);
             }
         }
 
@@ -271,6 +264,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;
@@ -294,7 +290,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);
             }
@@ -359,7 +355,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/...Analyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs b/...Analyzer.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"), 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();
     }
 
@@ -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", "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();
     }
 
@@ -301,12 +301,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/...s/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs b/...s/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs
@@ -233,14 +233,36 @@ 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"));
         context.Validate("boolParameter", LiveIn("boolParameter"), LiveOut("variable"));
         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
@@ -648,11 +648,22 @@ 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
+
+            DeadStores lst;
+            lst = new DeadStores  // Noncompliant
+            {
+                Property = 42
+            };
+            lst = new DeadStores
+            {
+                Property = 42
+            };
+            lst.ToString();
         }
 
         private void SimpleAssignment()