SE: Move CollectionConstraint from S4158 to the engine (part 6) (#8733)

analyzers/its/expected/Net5/S2589-Net5-net5.0.json

@@ -53,6 +53,12 @@
       "Message": "Change this condition so that it does not always evaluate to \u0027True\u0027.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S3440.cs#L11",
       "Location": "Line 11 Position 17-18"
+    },
+    {
+      "Id": "S2589",
+      "Message": "Change this condition so that it does not always evaluate to \u0027True\u0027.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S3981.cs#L12",
+      "Location": "Line 12 Position 17-62"
     }
   ]
 }

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Argument.cs

@@ -44,7 +44,9 @@ private static ProgramState ProcessArgument(ProgramState state, IArgumentOperati
         {
             state = state.SetSymbolConstraint(notNullSymbol, ObjectConstraint.NotNull);
         }
-        if (argument.WrappedOperation.TrackedSymbol(state) is { } symbol
+        // Enumerable is static class with extensions, so the instance is passed as argument, but it does not get mutated
+        if (!argument.Parameter.ContainingType.Is(KnownType.System_Linq_Enumerable)
+            && argument.WrappedOperation.TrackedSymbol(state) is { } symbol
             && state[symbol] is { } symbolValue)
         {
             state = state.SetSymbolValue(symbol, symbolValue.WithoutConstraint<CollectionConstraint>());

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/ArrayCreation.cs

@@ -28,6 +28,6 @@ protected override IArrayCreationOperationWrapper Convert(IOperation operation)
         IArrayCreationOperationWrapper.FromOperation(operation);
 
     protected override ProgramState Process(SymbolicContext context, IArrayCreationOperationWrapper operation) =>
-        context.SetOperationConstraint(CollectionTracker.ArrayCreationConstraint(operation))
+        CollectionTracker.LearnFrom(context.State, operation)
             .SetOperationConstraint(operation, ObjectConstraint.NotNull);
 }

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/CollectionTracker.cs

@@ -44,8 +44,8 @@ internal static class CollectionTracker
         KnownType.System_Collections_Generic_IDictionary_TKey_TValue,
         KnownType.System_Collections_Immutable_IImmutableDictionary_TKey_TValue);
 
-    public static readonly HashSet<string> AddMethods = new()
-    {
+    private static readonly HashSet<string> AddMethods =
+    [
         nameof(ICollection<int>.Add),
         nameof(List<int>.AddRange),
         nameof(List<int>.Insert),
@@ -57,10 +57,10 @@ internal static class CollectionTracker
         nameof(Stack<int>.Push),
         nameof(Collection<int>.Insert),
         "TryAdd"
-    };
+    ];
 
-    public static readonly HashSet<string> RemoveMethods = new()
-    {
+    private static readonly HashSet<string> RemoveMethods =
+    [
         nameof(ICollection<int>.Remove),
         nameof(List<int>.RemoveAll),
         nameof(List<int>.RemoveAt),
@@ -70,31 +70,41 @@ internal static class CollectionTracker
         nameof(HashSet<int>.RemoveWhere),
         nameof(Queue<int>.Dequeue),
         nameof(Stack<int>.Pop)
-    };
+    ];
 
     private static readonly HashSet<string> AddOrRemoveMethods = [.. AddMethods, .. RemoveMethods];
 
-    public static CollectionConstraint ObjectCreationConstraint(ProgramState state, IObjectCreationOperationWrapper operation)
+    public static ProgramState LearnFrom(ProgramState state, IObjectCreationOperationWrapper operation)
     {
         if (operation.Type.IsAny(CollectionTypes))
         {
-            return operation.Arguments.SingleOrDefault(IsEnumerable) is { } argument
-                ? state.Constraint<CollectionConstraint>(argument)
-                : CollectionConstraint.Empty;
+            if (operation.Arguments.SingleOrDefault(IsEnumerable) is { } argument)
+            {
+                return state.Constraint<CollectionConstraint>(argument) is { } constraint
+                    ? state.SetOperationConstraint(operation, constraint)
+                    : state;
+            }
+            else
+            {
+                return state.SetOperationConstraint(operation, CollectionConstraint.Empty);
+            }
         }
         else
         {
-            return null;
+            return state;
         }
 
         static bool IsEnumerable(IOperation operation) =>
             operation.ToArgument().Parameter.Type.DerivesOrImplements(KnownType.System_Collections_IEnumerable);
     }
 
-    public static CollectionConstraint ArrayCreationConstraint(IArrayCreationOperationWrapper operation) =>
-        operation.DimensionSizes.Any(x => x.ConstantValue.Value is 0)
+    public static ProgramState LearnFrom(ProgramState state, IArrayCreationOperationWrapper operation)
+    {
+        var constraint = operation.DimensionSizes.Any(x => x.ConstantValue.Value is 0)
             ? CollectionConstraint.Empty
             : CollectionConstraint.NotEmpty;
+        return state.SetOperationConstraint(operation, constraint);
+    }
 
     public static ProgramState LearnFrom(ProgramState state, IMethodReferenceOperationWrapper operation) =>
         operation.Instance is not null
@@ -132,4 +142,69 @@ public static ProgramState LearnFrom(ProgramState state, IPropertyReferenceOpera
         }
         return state;
     }
+
+    public static ProgramState LearnFrom(ProgramState state, IInvocationOperationWrapper invocation)
+    {
+        if (EnumerableCountConstraint(state, invocation) is { } constraint)
+        {
+            return state.SetOperationConstraint(invocation, constraint);
+        }
+        if (invocation.Instance is { } instance && instance.Type.DerivesOrImplementsAny(CollectionTypes))
+        {
+            var targetMethod = invocation.TargetMethod;
+            var symbolValue = state[instance] ?? SymbolicValue.Empty;
+            if (AddMethods.Contains(targetMethod.Name))
+            {
+                return SetOperationAndSymbolValue(symbolValue.WithConstraint(CollectionConstraint.NotEmpty));
+            }
+            else if (RemoveMethods.Contains(targetMethod.Name))
+            {
+                return SetOperationAndSymbolValue(symbolValue.WithoutConstraint(CollectionConstraint.NotEmpty));
+            }
+            else if (targetMethod.Name == nameof(ICollection<int>.Clear))
+            {
+                return SetOperationAndSymbolValue(symbolValue.WithConstraint(CollectionConstraint.Empty));
+            }
+        }
+        return state;
+
+        ProgramState SetOperationAndSymbolValue(SymbolicValue value)
+        {
+            if (instance.TrackedSymbol(state) is { } symbol)
+            {
+                state = state.SetSymbolValue(symbol, value);
+            }
+            return state.SetOperationValue(instance, value);
+        }
+    }
+
+    private static NumberConstraint EnumerableCountConstraint(ProgramState state, IInvocationOperationWrapper invocation)
+    {
+        if (invocation.TargetMethod.Is(KnownType.System_Linq_Enumerable, nameof(Enumerable.Count)))
+        {
+            var instance = invocation.Instance ?? invocation.Arguments[0].ToArgument().Value;
+            if (instance.TrackedSymbol(state) is { } symbol
+                && state[symbol]?.Constraint<CollectionConstraint>() is { } collection)
+            {
+                if (collection == CollectionConstraint.Empty)
+                {
+                    return NumberConstraint.From(0);
+                }
+                else
+                {
+                    return HasFilteringPredicate()
+                        ? NumberConstraint.From(0, null) // nonEmpty.Count(predicate) can be Empty or NotEmpty
+                        : NumberConstraint.From(1, null);
+                }
+            }
+            else
+            {
+                return NumberConstraint.From(0, null);
+            }
+        }
+        return null;
+
+        bool HasFilteringPredicate() =>
+            invocation.Arguments.Any(x => x.ToArgument().Parameter.Type.Is(KnownType.System_Func_T_TResult));
+    }
 }

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.Enumerable.cs

@@ -24,7 +24,7 @@ namespace SonarAnalyzer.SymbolicExecution.Roslyn.OperationProcessors;
 
 internal sealed partial class Invocation
 {
-    private static readonly HashSet<string> EnumerableAndQueryableReturningNotNull = new()
+    private static readonly HashSet<string> ReturningNotNull = new()
     {
         nameof(Enumerable.Append),
         nameof(Enumerable.AsEnumerable),
@@ -71,28 +71,23 @@ internal sealed partial class Invocation
         nameof(Enumerable.Zip),
     };
 
-    private static ProgramState[] ProcessLinqEnumerableAndQueryable(SymbolicContext context, IInvocationOperationWrapper invocation)
+    private static ProgramState[] ProcessLinqEnumerableAndQueryable(ProgramState state, IInvocationOperationWrapper invocation)
     {
         var name = invocation.TargetMethod.Name;
-        if (EnumerableAndQueryableReturningNotNull.Contains(name))
+        if (ReturningNotNull.Contains(name))
         {
-            return context.SetOperationConstraint(ObjectConstraint.NotNull).ToArray();
+            return state.SetOperationConstraint(invocation, ObjectConstraint.NotNull).ToArray();
         }
-        else if (name == nameof(Enumerable.FirstOrDefault)   // ElementAtOrDefault is intentionally not supported. It's causing many FPs
-            || name == nameof(Enumerable.LastOrDefault)
-            || name == nameof(Enumerable.SingleOrDefault))
+        // ElementAtOrDefault is intentionally not supported. It's causing many FPs
+        else if (name is nameof(Enumerable.FirstOrDefault) or nameof(Enumerable.LastOrDefault) or nameof(Enumerable.SingleOrDefault))
         {
             return invocation.TargetMethod.ReturnType.IsReferenceType
-                ? new[]
-                {
-                    context.SetOperationConstraint(ObjectConstraint.Null),
-                    context.SetOperationConstraint(ObjectConstraint.NotNull),
-                }
-                : context.State.ToArray();
+                ? [state.SetOperationConstraint(invocation, ObjectConstraint.Null), state.SetOperationConstraint(invocation, ObjectConstraint.NotNull)]
+                : state.ToArray();
         }
         else
         {
-            return context.State.ToArray();
+            return state.ToArray();
         }
     }
 }

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

@@ -42,9 +42,9 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp
         }
         else if (invocation.Instance.TrackedSymbol(state) is { } symbol && !IsNullableGetValueOrDefault(invocation))
         {
-            state = state.SetSymbolConstraint(symbol, ObjectConstraint.NotNull);
+            state = state.SetSymbolConstraint(symbol, ObjectConstraint.NotNull)
+                .SetOperationConstraint(invocation.Instance, ObjectConstraint.NotNull);
         }
-
         if (invocation.HasThisReceiver(state))
         {
             state = state.ResetFieldConstraints();
@@ -56,14 +56,16 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp
         {
             state = state.SetSymbolConstraint(instanceSymbol, ObjectConstraint.NotNull);
         }
+        state = CollectionTracker.LearnFrom(state, invocation);
+
         return invocation switch
         {
             _ when IsNullableGetValueOrDefault(invocation) => ProcessNullableGetValueOrDefault(context, invocation).ToArray(),
             _ when invocation.TargetMethod.Is(KnownType.Microsoft_VisualBasic_Information, "IsNothing") => ProcessInformationIsNothing(context, invocation),
             _ when invocation.TargetMethod.Is(KnownType.System_Diagnostics_Debug, nameof(Debug.Assert)) => ProcessDebugAssert(context, invocation),
             _ when invocation.TargetMethod.Is(KnownType.System_Object, nameof(ReferenceEquals)) => ProcessReferenceEquals(context, invocation),
             _ when invocation.TargetMethod.Is(KnownType.System_Nullable_T, "get_HasValue") => ProcessNullableHasValue(state, invocation),
-            _ when invocation.TargetMethod.ContainingType.IsAny(KnownType.System_Linq_Enumerable, KnownType.System_Linq_Queryable) => ProcessLinqEnumerableAndQueryable(context, invocation),
+            _ when invocation.TargetMethod.ContainingType.IsAny(KnownType.System_Linq_Enumerable, KnownType.System_Linq_Queryable) => ProcessLinqEnumerableAndQueryable(state, invocation),
             _ when invocation.TargetMethod.Name == nameof(Equals) => ProcessEquals(context, invocation),
             _ when invocation.TargetMethod.ContainingType.Is(KnownType.System_String) => ProcessSystemStringInvocation(state, invocation),
             _ => ProcessArgumentAttributes(state, invocation),

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/ObjectCreation.cs

@@ -46,10 +46,8 @@ protected override ProgramState Process(SymbolicContext context, IObjectCreation
         }
         else
         {
-            var newState = context.SetOperationConstraint(ObjectConstraint.NotNull);
-            return CollectionTracker.ObjectCreationConstraint(context.State, operation) is { } constraint
-                ? newState.SetOperationConstraint(operation, constraint)
-                : newState;
+            return CollectionTracker.LearnFrom(context.State, operation)
+                .SetOperationConstraint(operation, ObjectConstraint.NotNull);
         }
     }
 }

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RuleChecks/EmptyCollectionsShouldNotBeEnumeratedBase.cs

@@ -21,7 +21,6 @@
 using System.Collections;
 using System.Collections.ObjectModel;
 using SonarAnalyzer.SymbolicExecution.Constraints;
-using SonarAnalyzer.SymbolicExecution.Roslyn.OperationProcessors;
 
 namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks;
 
@@ -30,8 +29,8 @@ public abstract class EmptyCollectionsShouldNotBeEnumeratedBase : SymbolicRuleCh
     protected const string DiagnosticId = "S4158";
     protected const string MessageFormat = "Remove this call, the collection is known to be empty here.";
 
-    protected static readonly HashSet<string> RaisingMethods = new()
-    {
+    protected static readonly HashSet<string> RaisingMethods =
+    [
         nameof(IEnumerable<int>.GetEnumerator),
         nameof(ICollection.CopyTo),
         nameof(ICollection<int>.Clear),
@@ -82,16 +81,11 @@ public abstract class EmptyCollectionsShouldNotBeEnumeratedBase : SymbolicRuleCh
         nameof(Dictionary<int, int>.ContainsKey),
         nameof(Dictionary<int, int>.ContainsValue),
         nameof(Dictionary<int, int>.TryGetValue)
-    };
+    ];
 
     private readonly HashSet<IOperation> emptyAccess = new();
     private readonly HashSet<IOperation> nonEmptyAccess = new();
 
-    protected override ProgramState PreProcessSimple(SymbolicContext context) =>
-        context.Operation.Instance.AsInvocation() is { } invocation
-            ? ProcessInvocation(context, invocation)
-            : context.State;
-
     public override void ExecutionCompleted()
     {
         foreach (var operation in emptyAccess.Except(nonEmptyAccess))
@@ -100,82 +94,21 @@ public override void ExecutionCompleted()
         }
     }
 
-    private ProgramState ProcessInvocation(SymbolicContext context, IInvocationOperationWrapper invocation)
+    protected override ProgramState PreProcessSimple(SymbolicContext context)
     {
-        var targetMethod = invocation.TargetMethod;
-        if (targetMethod.Is(KnownType.System_Linq_Enumerable, nameof(Enumerable.Count))
-            && SizeConstraint(context.State, invocation.Instance ?? invocation.Arguments[0].ToArgument().Value, HasFilteringPredicate()) is { } constraint)
-        {
-            return context.SetOperationConstraint(constraint);
-        }
-        else if (invocation.Instance is { } instance)
+        if (context.Operation.Instance.AsInvocation() is { } invocation
+            && invocation.Instance is { } instance
+            && RaisingMethods.Contains(invocation.TargetMethod.Name))
         {
-            if (RaisingMethods.Contains(targetMethod.Name))
+            if (context.State[instance]?.HasConstraint(CollectionConstraint.Empty) is true)
             {
-                if (context.State[instance]?.HasConstraint(CollectionConstraint.Empty) is true)
-                {
-                    emptyAccess.Add(context.Operation.Instance);
-                }
-                else
-                {
-                    nonEmptyAccess.Add(context.Operation.Instance);
-                }
+                emptyAccess.Add(context.Operation.Instance);
             }
-            if (instance.Type.DerivesOrImplementsAny(CollectionTracker.CollectionTypes))
+            else
             {
-                return ProcessAddMethod(context.State, targetMethod, instance)
-                    ?? ProcessRemoveMethod(context.State, targetMethod, instance)
-                    ?? ProcessClearMethod(context.State, targetMethod, instance)
-                    ?? context.State;
+                nonEmptyAccess.Add(context.Operation.Instance);
             }
         }
         return context.State;
-
-        bool HasFilteringPredicate() =>
-            invocation.Arguments.Any(x => x.ToArgument().Parameter.Type.Is(KnownType.System_Func_T_TResult));
-    }
-
-    private static ProgramState ProcessAddMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
-        CollectionTracker.AddMethods.Contains(method.Name)
-            ? SetOperationAndSymbolConstraint(state, instance, CollectionConstraint.NotEmpty)
-            : null;
-
-    private static ProgramState ProcessRemoveMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
-        CollectionTracker.RemoveMethods.Contains(method.Name)
-            ? SetOperationAndSymbolValue(state, instance, (state[instance] ?? SymbolicValue.Empty).WithoutConstraint(CollectionConstraint.NotEmpty))
-            : null;
-
-    private static ProgramState ProcessClearMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
-        method.Name == nameof(ICollection<int>.Clear)
-            ? SetOperationAndSymbolConstraint(state, instance, CollectionConstraint.Empty)
-            : null;
-
-    private static ProgramState SetOperationAndSymbolConstraint(ProgramState state, IOperation instance, SymbolicConstraint constraint) =>
-        SetOperationAndSymbolValue(state, instance, (state[instance] ?? SymbolicValue.Empty).WithConstraint(constraint));
-
-    private static ProgramState SetOperationAndSymbolValue(ProgramState state, IOperation instance, SymbolicValue value)
-    {
-        state = state.SetOperationValue(instance, value);
-        if (instance.TrackedSymbol(state) is { } symbol)
-        {
-            state = state.SetSymbolValue(symbol, value);
-        }
-        return state;
-    }
-
-    private static NumberConstraint SizeConstraint(ProgramState state, IOperation instance, bool hasFilteringPredicate = false)
-    {
-        if (instance.TrackedSymbol(state) is { } symbol && state[symbol]?.Constraint<CollectionConstraint>() is { } collection)
-        {
-            if (collection == CollectionConstraint.Empty)
-            {
-                return NumberConstraint.From(0);
-            }
-            else if (!hasFilteringPredicate)    // nonEmpty.Count(predicate) can be Empty or NotEmpty
-            {
-                return NumberConstraint.From(1, null);
-            }
-        }
-        return instance.Type.DerivesOrImplementsAny(CollectionTracker.CollectionTypes) ? NumberConstraint.From(0, null) : null;
     }
 }