Fix false positive for delegate

src/EffectiveCSharp.Analyzers/ExpressCallbacksWithDelegatesAnalyzer.cs

@@ -55,35 +55,59 @@ private static void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context)
 
     private static bool ShouldWarnForMethod(InvocationExpressionSyntax invocationExpr, SemanticModel semanticModel)
     {
-#pragma warning disable S3267 // Loops should be simplified with "LINQ" expressions
-        foreach (ArgumentSyntax argument in invocationExpr.ArgumentList.Arguments)
+        for (int i = 0; i < invocationExpr.ArgumentList.Arguments.Count; i++)
         {
+            ArgumentSyntax argument = invocationExpr.ArgumentList.Arguments[i];
             ITypeSymbol? argumentType = semanticModel.GetTypeInfo(argument.Expression).Type;
 
             // Check if the argument is a delegate type
-            if (IsDelegateType(argumentType))
+            if (!IsDelegateType(argumentType))
             {
-                // Check if the delegate is passed to another method or assigned to a field/property
-                SyntaxNode? parent = argument.Expression.Parent;
-                while (parent != null)
-                {
-                    if (parent is ArgumentSyntax or AssignmentExpressionSyntax)
-                    {
-                        return true;
-                    }
-
-                    parent = parent.Parent;
-                }
-
-                // Additional checks for specific scenarios can be added here
+                continue;
+            }
+
+            // Check if the delegate is being combined (multicast) within the method
+            MethodDeclarationSyntax? parentMethod = invocationExpr.FirstAncestorOrSelf<MethodDeclarationSyntax>();
+            if (parentMethod != null && IsDelegateMulticast(argument.Expression, parentMethod, semanticModel))
+            {
+                return true;
             }
         }
-#pragma warning restore S3267 // Loops should be simplified with "LINQ" expressions
 
         // No issues detected
         return false;
     }
 
+    private static bool IsDelegateMulticast(ExpressionSyntax delegateExpression, MethodDeclarationSyntax parentMethod, SemanticModel semanticModel)
+    {
+        BlockSyntax? blockSyntax = parentMethod.Body;
+
+        if (blockSyntax == null)
+        {
+            return false;
+        }
+
+        ISymbol? delegateSymbol = semanticModel.GetSymbolInfo(delegateExpression).Symbol;
+
+        // Analyze if the delegate is involved in a multicast operation
+        foreach (AssignmentExpressionSyntax? syntax in blockSyntax.DescendantNodes().OfType<AssignmentExpressionSyntax>())
+        {
+            if (!syntax.OperatorToken.IsKind(SyntaxKind.PlusEqualsToken))
+            {
+                continue;
+            }
+
+            ISymbol? leftSymbol = semanticModel.GetSymbolInfo(syntax.Left).Symbol;
+
+            if (leftSymbol != null && leftSymbol.Equals(delegateSymbol, SymbolEqualityComparer.IncludeNullability))
+            {
+                return true; // The delegate is being combined/multicasted
+            }
+        }
+
+        return false;
+    }
+
     private static bool IsDelegateType(ITypeSymbol? typeSymbol)
     {
         if (typeSymbol == null)

src/EffectiveCSharp.Analyzers/SquiggleCop.Baseline.yaml

@@ -1088,7 +1088,7 @@
 - {Id: S3263, Title: 'Static fields should appear in the order they must be initialized ', Category: Major Bug, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3264, Title: Events should be invoked, Category: Major Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3265, Title: Non-flags enums should not be used in bitwise operations, Category: Critical Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
-- {Id: S3267, Title: Loops should be simplified with "LINQ" expressions, Category: Minor Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: true}
+- {Id: S3267, Title: Loops should be simplified with "LINQ" expressions, Category: Minor Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3329, Title: Cipher Block Chaining IVs should be unpredictable, Category: Critical Vulnerability, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3330, Title: Creating cookies without the "HttpOnly" flag is security-sensitive, Category: Minor Security Hotspot, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3343, Title: Caller information parameters should come at the end of the parameter list, Category: Major Bug, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}

src/tools/Dogfood/SquiggleCop.Baseline.yaml

@@ -1096,7 +1096,7 @@
 - {Id: S3263, Title: 'Static fields should appear in the order they must be initialized ', Category: Major Bug, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3264, Title: Events should be invoked, Category: Major Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3265, Title: Non-flags enums should not be used in bitwise operations, Category: Critical Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
-- {Id: S3267, Title: Loops should be simplified with "LINQ" expressions, Category: Minor Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: true}
+- {Id: S3267, Title: Loops should be simplified with "LINQ" expressions, Category: Minor Code Smell, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3329, Title: Cipher Block Chaining IVs should be unpredictable, Category: Critical Vulnerability, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3330, Title: Creating cookies without the "HttpOnly" flag is security-sensitive, Category: Minor Security Hotspot, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
 - {Id: S3343, Title: Caller information parameters should come at the end of the parameter list, Category: Major Bug, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}

tests/EffectiveCSharp.Analyzers.Tests/ExpressCallbacksAsDelegateTests.cs

@@ -26,9 +26,9 @@ public void Test()
                 List<int> numbers = Enumerable.Range(1, 200).ToList();
                 var oddNumbers = numbers.Find(n=> n % 2 == 1);
                 var test = numbers.TrueForAll(n=> n < 50);
-            
+
                 numbers.RemoveAll(n => n % 2 == 0);
-            
+
                 numbers.ForEach(item => Console.WriteLine(item));
               }
             }
@@ -49,7 +49,7 @@ await Verifier.VerifyAnalyzerAsync(
             public class MyClass
             {
               List<MyClass> container = new();
-            
+
               public void LengthyOperation(Func<bool> predicate)
               {
                 foreach(MyClass item in container)
@@ -61,11 +61,11 @@ public void LengthyOperation(Func<bool> predicate)
                     }
                 }
               }
-              
+
               public void DoLengthyOperation() {}
               public bool CheckWithUser() => true;
               public bool CheckWithSystem() => true;
-              
+
               public void CheckThenDo()
               {
                 Func<bool> cp = () => CheckWithUser();
@@ -91,26 +91,26 @@ await Verifier.VerifyAnalyzerAsync(
             public class MyClass
             {
               List<MyClass> container = new();
-            
+
               public void LengthyOperation(Func<bool> predicate)
               {
                 bool canContinue = true;
                 foreach(MyClass item in container)
                 {
                   item.DoLengthyOperation();
-                  
+
                   foreach(Func<bool> pr in predicate.GetInvocationList())
                   {
                     canContinue &= pr();
-                    
+
                     if (!canContinue)
                     {
                       return;
                     }
                   }
                 }
               }
-              
+
               public void DoLengthyOperation() {}
             }
             """,
@@ -125,12 +125,12 @@ await Verifier.VerifyAnalyzerAsync(
             public class CustomCollection<T>
             {
                 private List<T> _items = new List<T>();
-            
+
                 public void Add(T item)
                 {
                     _items.Add(item);
                 }
-            
+
                 public void ProcessItems(Action<T> callback)
                 {
                     foreach (var item in _items)
@@ -139,7 +139,7 @@ public void ProcessItems(Action<T> callback)
                     }
                 }
             }
-            
+
             public class TestClass
             {
                 public void TestMethod()
@@ -148,7 +148,7 @@ public void TestMethod()
                     collection.Add(1);
                     collection.Add(2);
                     collection.Add(3);
-            
+
                     collection.ProcessItems(item => Console.WriteLine(item));
                 }
             }
@@ -164,7 +164,7 @@ await Verifier.VerifyAnalyzerAsync(
             public class MultiDelegateHandler
             {
                 private List<int> _items = new List<int> { 1, 2, 3, 4, 5 };
-            
+
                 public void HandleItems(Action<int> actionCallback, Predicate<int> predicateCallback)
                 {
                     foreach (var item in _items)
@@ -200,7 +200,7 @@ await Verifier.VerifyAnalyzerAsync(
             public class TransformProcessor
             {
                 private List<int> _items = new List<int> { 1, 2, 3, 4, 5 };
-            
+
                 public void ProcessItems(Func<int, string> transformCallback, Action<string> actionCallback)
                 {
                     foreach (var item in _items)
@@ -225,4 +225,53 @@ public void TestMethod()
             """,
             ReferenceAssemblyCatalog.Latest);
     }
+
+    [Fact]
+    public async Task Private_Delegate()
+    {
+        // The analyzer SHOULD NOT trigger in this scenario.
+        // The delegate is being used as intended, without being
+        // passed around or combined in a manner that could cause
+        // issues. The code is simple and straightforward, adhering
+        // to safe and typical usage of delegates for callbacks.
+        await Verifier.VerifyAnalyzerAsync(
+            """
+            public class C
+            {
+                private Func<char, bool> _predicate = null;
+
+                public C()
+                {
+                    _predicate = char.IsLower;
+                }
+
+                public bool M(string s)
+                {
+                    return SpanExtensions.All(s.AsSpan(), _predicate);
+                }
+
+                public bool N(string s)
+                {
+                    return s.AsSpan().All(_predicate);
+                }
+            }
+
+            public static class SpanExtensions
+            {
+                public static bool All(this ReadOnlySpan<char> source, Func<char, bool> predicate)
+                {
+                    for (var i = 0; i < source.Length; i++)
+                    {
+                        if (!predicate(source[i]))
+                        {
+                            return false;
+                        }
+                    }
+
+                    return true;
+                }
+            }
+            """,
+            ReferenceAssemblyCatalog.Latest);
+    }
 }