S4158 FP: Collection filled in a for loop reported to be empty (#7447)

SonarSource · Jun 16, 2023 · ecb72c6 · ecb72c6
1 parent 2006031
commit ecb72c6
Show file tree

Hide file tree

Showing 5 changed files with 179 additions and 16 deletions.
diff --git a/...nalyzer.CSharp/SymbolicExecution/Sonar/Analyzers/EmptyCollectionsShouldNotBeEnumerated.cs b/...nalyzer.CSharp/SymbolicExecution/Sonar/Analyzers/EmptyCollectionsShouldNotBeEnumerated.cs
@@ -18,7 +18,7 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using SonarAnalyzer.SymbolicExecution.Sonar.Constraints;
+using SonarAnalyzer.SymbolicExecution.Constraints;
 
 namespace SonarAnalyzer.SymbolicExecution.Sonar.Analyzers
 {

diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/CollectionConstraint.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/CollectionConstraint.cs
@@ -18,9 +18,7 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using SonarAnalyzer.SymbolicExecution.Constraints;
-
-namespace SonarAnalyzer.SymbolicExecution.Sonar.Constraints;
+namespace SonarAnalyzer.SymbolicExecution.Constraints;
 
 internal sealed class CollectionConstraint : SymbolicConstraint
 {

diff --git a/...r.Common/SymbolicExecution/Roslyn/RuleChecks/EmptyCollectionsShouldNotBeEnumeratedBase.cs b/...r.Common/SymbolicExecution/Roslyn/RuleChecks/EmptyCollectionsShouldNotBeEnumeratedBase.cs
@@ -20,8 +20,7 @@
 
 using System.Collections;
 using System.Collections.ObjectModel;
-using System.Linq;
-using SonarAnalyzer.SymbolicExecution.Sonar.Constraints;
+using SonarAnalyzer.SymbolicExecution.Constraints;
 
 namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks;
 
@@ -122,9 +121,9 @@ protected override ProgramState PreProcessSimple(SymbolicContext context)
                 ? context.State.SetOperationConstraint(objectCreation, constraint)
                 : context.State;
         }
-        else if (IsEmptyArray(operation))
+        else if (operation.AsArrayCreation() is { } arrayCreation)
         {
-            return context.State.SetOperationConstraint(operation, CollectionConstraint.Empty);
+            return context.State.SetOperationConstraint(operation, arrayCreation.DimensionSizes.Any(x => x.ConstantValue.Value is 0) ? CollectionConstraint.Empty : CollectionConstraint.NotEmpty);
         }
         else if (operation.AsInvocation() is { } invocation)
         {
@@ -134,6 +133,10 @@ protected override ProgramState PreProcessSimple(SymbolicContext context)
         {
             return ProcessMethod(context, methodReference.Method, methodReference.Instance);
         }
+        else if (operation.AsPropertyReference() is { } propertyReference && PropertyReferenceConstraint(context.State, propertyReference) is { } constraint)
+        {
+            return context.State.SetOperationConstraint(operation, constraint);
+        }
         else
         {
             return context.State;
@@ -148,14 +151,6 @@ public override void ExecutionCompleted()
         }
     }
 
-    private static CollectionConstraint CollectionCreationConstraint(ProgramState state, IObjectCreationOperationWrapper objectCreation) =>
-        objectCreation.Arguments.SingleOrDefault(x => x.ToArgument().Parameter.Type.DerivesOrImplements(KnownType.System_Collections_IEnumerable)) is { } collectionArgument
-            ? state[collectionArgument]?.Constraint<CollectionConstraint>()
-            : CollectionConstraint.Empty;
-
-    private static bool IsEmptyArray(IOperation operation) =>
-        operation.AsArrayCreation()?.DimensionSizes.Any(x => x.ConstantValue.Value is 0) ?? false;
-
     private ProgramState ProcessMethod(SymbolicContext context, IMethodSymbol method, IOperation instance)
     {
         var state = context.State;
@@ -184,4 +179,23 @@ private ProgramState ProcessMethod(SymbolicContext context, IMethodSymbol method
         }
         return state;
     }
+
+    private static CollectionConstraint CollectionCreationConstraint(ProgramState state, IObjectCreationOperationWrapper objectCreation) =>
+        objectCreation.Arguments.SingleOrDefault(x => x.ToArgument().Parameter.Type.DerivesOrImplements(KnownType.System_Collections_IEnumerable)) is { } collectionArgument
+            ? state[collectionArgument]?.Constraint<CollectionConstraint>()
+            : CollectionConstraint.Empty;
+
+    private static NumberConstraint PropertyReferenceConstraint(ProgramState state, IPropertyReferenceOperationWrapper propertyReference)
+    {
+        if (propertyReference.Property.Name is nameof(Array.Length) or nameof(List<int>.Count)
+            && state.ResolveCapture(propertyReference.Instance).TrackedSymbol() is { } symbol
+            && state[symbol]?.Constraint<CollectionConstraint>() is { } collection)
+        {
+            return collection == CollectionConstraint.Empty ? NumberConstraint.From(0) : NumberConstraint.From(1, null);
+        }
+        else
+        {
+            return null;
+        }
+    }
 }
diff --git a/...tTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.CSharp8.cs b/...tTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.CSharp8.cs
@@ -153,3 +153,63 @@ public interface IDecoder
 {
     bool Convert(out int i3, out int i4);
 }
+
+// https://github.com/SonarSource/sonar-dotnet/issues/6179
+public class Repro_6179
+{
+    public void FilledInLoop_FromAnotherCollection_FromArray()
+    {
+        var data = new[] { 0, 0, 1, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 0, 0 };
+        var list = new List<List<int>>();
+        List<int> currentBin = null;
+        for (var i = 0; i < data.Length; i++)
+        {
+            if (data[i] == 0)
+            {
+                if (currentBin != null)
+                {
+                    list.Add(currentBin);
+                    currentBin = null;
+                }
+
+                continue;
+            }
+
+            currentBin ??= new List<int>();
+            currentBin.Add(i);
+        }
+
+        for (var i = 0; i < list.Count; i++)
+        {
+            list.ForEach(x => { }); // Compliant
+        }
+    }
+
+    public void FilledInLoop_FromAnotherCollection_FromList()
+    {
+        var data = new List<int> { 0, 0, 1, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 0, 0 };
+        var list = new List<List<int>>();
+        List<int> currentBin = null;
+        for (var i = 0; i < data.Count; i++)
+        {
+            if (data[i] == 0)
+            {
+                if (currentBin != null)
+                {
+                    list.Add(currentBin);
+                    currentBin = null;
+                }
+
+                continue;
+            }
+
+            currentBin ??= new List<int>();
+            currentBin.Add(i);
+        }
+
+        for (var i = 0; i < list.Count; i++)
+        {
+            list.ForEach(x => { }); // Compliant
+        }
+    }
+}
diff --git a/...yzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.cs b/...yzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.cs
@@ -498,6 +498,97 @@ public void HigherRank_And_Jagged_Array()
         array5.Clone(); // Compliant
     }
 
+    public void LearnConditions_Size(bool condition)
+    {
+        List<int> isNull = null;
+        var empty = new List<int>();
+        var notEmpty = new List<int>() { 1, 2, 3 };
+
+        if ((isNull ?? empty).Count == 0)
+        {
+            empty.Clear();  // Noncompliant
+        }
+
+        if ((isNull ?? notEmpty).Count == 0)
+        {
+            empty.Clear();  // Compliant, unreachable
+        }
+
+        if ((condition ? empty : empty).Count == 0)
+        {
+            empty.Clear();  // Noncompliant
+        }
+        else
+        {
+            empty.Clear();  // Compliant, unreachable
+        }
+
+        if (empty.Count == 0)
+        {
+            empty.Clear();  // Noncompliant
+        }
+        else
+        {
+            empty.Clear();  // Compliant, unreachable
+        }
+
+        if (empty.Count() == 0)
+        {
+            empty.Clear();  // Noncompliant
+        }
+        else
+        {
+            empty.Clear();  // Noncompliant FP, Should be Compliant, unreachable
+        }
+
+        if (Enumerable.Count(empty) == 0)
+        {
+            empty.Clear();  // Noncompliant
+        }
+        else
+        {
+            empty.Clear();  // Noncompliant FP, Should be Compliant, unreachable
+        }
+
+        notEmpty.Clear(); // Compliant, prevents LVA from throwing notEmpty away during reference capture
+    }
+
+    public void LearnConditions_Size_Array(bool condition)
+    {
+        int[] isNull = null;
+        var empty = new int[0];
+        var notEmpty = new[] { 1, 2, 3 };
+
+        if ((condition ? empty : empty).Length == 0)
+        {
+            empty.Clone();  // Noncompliant
+        }
+        else
+        {
+            empty.Clone();  // Compliant, unreachable
+        }
+
+        if (empty.Length == 0)
+        {
+            empty.Clone();  // Noncompliant
+        }
+        else
+        {
+            empty.Clone();  // Compliant, unreachable
+        }
+
+        if (empty.Count() == 0)
+        {
+            empty.Clone();  // Noncompliant
+        }
+        else
+        {
+            empty.Clone();  // Noncompliant FP, Should be Compliant, unreachable
+        }
+
+        notEmpty.Clone(); // Compliant, prevents LVA from throwing notEmpty away during reference capture
+    }
+
     void Foo(IEnumerable<int> items) { }
 }