NET-798 Fix S1854 FN: Support &&, ||, ?? and ??=

SonarSource · Dec 12, 2024 · ba3012a · ba3012a
1 parent 0a1a183
commit ba3012a
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 40 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs b/analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
@@ -71,14 +71,13 @@ 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;
     }
 
     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<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);
             }
         }
 
@@ -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/...Analyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.FlowCaptureOperation.cs b/...Analyzer.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/...s/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs b/...s/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.LocalFunction.cs
@@ -221,22 +221,44 @@ void LocalFunction()
     }
 
     [TestMethod]
-    public void LocalFunctionInvocation_Recursive_LiveIn()
+    public void LocalFunctionInvocation_Recursive_LiveIn_WithoutFlowCapture()
     {
         var code = """
             var variable = 42;
             if (boolParameter)
                 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_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
@@ -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();
         }