Improve EPC30 reliability: only warn on provably non-terminating recursion

docs/Rules/EPC30.md

@@ -4,18 +4,62 @@ This analyzer detects when a method calls itself recursively, which can lead to
 
 ## Description
 
-The analyzer warns when a method calls itself recursively. While recursion is sometimes necessary and appropriate, accidental recursion can cause stack overflow errors and infinite loops.
+The analyzer warns when a method calls itself recursively in a way that is provably **non-terminating**. It reports two situations:
+
+- The recursive call is reached unconditionally (nothing can terminate the method first).
+- The recursive call is guarded only by an *invariant* condition — one composed purely of unchanged value parameters and constants (e.g. `if (b) Foo(b);` or `if (n > 0) Foo(n);`). Once such a branch is taken it is taken forever, so the recursion never ends.
+
+To avoid false positives, the analyzer does **not** warn when:
+
+- A conditional that can terminate the method (e.g. `if (something) return;` or a conditional `throw`) appears before the call.
+- Termination depends on instance state, a property, or a method call (e.g. `if (_done) return;`, `if (Flag) Foo();`).
+- An argument is changed before the call (e.g. `Foo(n - 1)`, `n--; Foo(n);`), or a `ref` parameter is modified before the call.
+- The guard is anything we cannot prove invariant (a `switch`, a non-trivial loop, etc.).
+- The call is in a `catch` or `finally` block (those run conditionally). A call in a `try` *body*, however, is still reached unconditionally and is reported.
+
+When in doubt, the analyzer favors *not* reporting to keep false positives low.
 
 ## Code that triggers the analyzer
 
 ```csharp
 public class Example
 {    
-    // Suspicious: might be accidental recursion
+    // Suspicious: unconditional self-recursion with no base case
     public void ProcessData()
     {
         // Some processing...
-        ProcessData(); // ❌ EPC30 - Calls itself without obvious base case
+        ProcessData(); // ❌ EPC30 - Calls itself unconditionally
+    }
+
+    // Suspicious: the guard 'b' is passed unchanged, so this recurses forever once taken
+    public void Loop(bool b)
+    {
+        if (b) Loop(b); // ❌ EPC30 - Invariant guard => infinite recursion
+    }
+}
+```
+
+## Code that does NOT trigger the analyzer
+
+```csharp
+public class Example
+{
+    // OK: a conditional can terminate the recursion before the call.
+    public void Foo(int n)
+    {
+        n++;
+        if (n > 10) return;
+        Foo(n); // ✅ no warning
+    }
+
+    // OK: the argument changes, so the guard is not invariant.
+    public void Bar(int n)
+    {
+        if (n > 0)
+        {
+            n--;
+            Bar(n); // ✅ no warning
+        }
     }
 }
 ```

samples/ErrorProne.Samples/CoreAnalyzers/RecursiveCallSample.cs

@@ -0,0 +1,52 @@
+using System;
+
+namespace ErrorProne.Samples.CoreAnalyzers;
+
+public class RecursiveCallSample
+{
+    // EPC30: clearly unconditional self-recursion -> warns.
+    public void AlwaysRecurses()
+    {
+        AlwaysRecurses(); // ❌ EPC30
+    }
+
+    // EPC30: the guard 'b' is passed unchanged, so once taken it recurses forever.
+    public void InvariantGuard(bool b)
+    {
+        if (b) InvariantGuard(b); // ❌ EPC30
+    }
+
+    // OK: a conditional can terminate the recursion before the call.
+    public void ConditionalEarlyReturn(int n)
+    {
+        n++;
+        if (n > 10) return;
+        ConditionalEarlyReturn(n); // ✅ no warning
+    }
+
+    // OK: the argument changes, so the guard is not invariant.
+    public void DecreasingArgument(int n)
+    {
+        if (n > 0)
+        {
+            n--;
+            DecreasingArgument(n); // ✅ no warning
+        }
+    }
+
+    private bool _done;
+
+    // OK: termination depends on instance state.
+    public void DependsOnInstanceState()
+    {
+        if (_done) return;
+        DependsOnInstanceState(); // ✅ no warning
+    }
+
+    // OK: a proper base case with a changing argument.
+    public int Factorial(int n)
+    {
+        if (n <= 1) return 1;
+        return n * Factorial(n - 1); // ✅ no warning
+    }
+}

src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj

@@ -23,6 +23,7 @@
         * Add EPC39: QuadraticEnumerationAnalyzer (disabled by default).
         * Add EPC40: PrivateMethodMultipleEnumerationAnalyzer (disabled by default).
         * Add EPC41: FormatMethodArgumentsAnalyzer to validate arguments passed to string.Format-like methods.
+        * Fix EPC30: only warn on provably non-terminating self-recursion -- unconditional calls or calls guarded only by invariant conditions (unchanged parameters/constants); do not warn when a conditional, instance-state, property, or changed argument can terminate the recursion.
         * Fix EPC30: do not warn on recursive calls made from nested lambdas, anonymous methods, or local functions (#318).
         * Fix EPC20: do not warn when the type defines a user-defined implicit conversion to string (#317).
       0.8.2

src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs

@@ -93,8 +93,10 @@ public void Foo(ref int x)
         }
 
         [Test]
-        public async Task WarnsOnConditionalRecursiveCall()
+        public async Task Warns_OnInvariantGuardedRecursiveCall()
         {
+            // The guard 'b' is passed unchanged to the call, so once the branch is taken it is
+            // taken forever -> provably infinite recursion.
             var test = @"
 class C {
     void Foo(bool b) {
@@ -106,7 +108,7 @@ void Foo(bool b) {
         }
 
         [Test]
-        public async Task WarnsOnConditionalRecursiveCall_With_Named_Parameters()
+        public async Task Warns_OnInvariantGuardedRecursiveCall_With_Named_Parameters()
         {
             var test = @"
 class C {
@@ -118,6 +120,245 @@ void Foo(bool b) {
             await Verify.VerifyAsync(test);
         }
 
+        [Test]
+        public async Task Warns_OnInvariantComparisonGuardedRecursiveCall()
+        {
+            var test = @"
+class C {
+    void Foo(int n) {
+        if (n > 0) [|Foo(n)|];
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task Warns_When_Call_Is_Inside_Ternary_With_Invariant_Condition()
+        {
+            var test = @"
+class C {
+    int Foo(bool b) {
+        return b ? [|Foo(b)|] : 0;
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task Warns_When_Call_Is_Inside_While_With_Invariant_Condition()
+        {
+            var test = @"
+class C {
+    void Foo(int n) {
+        while (n > 0) {
+            [|Foo(n)|];
+        }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Guard_Parameter_Is_Mutated()
+        {
+            // 'n' is decremented before the recursive call, so the guard is not invariant.
+            var test = @"
+class C {
+    void Foo(int n) {
+        if (n > 0) {
+            n--;
+            Foo(n);
+        }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Guard_Parameter_Is_Mutated_Via_Compound_Assignment()
+        {
+            var test = @"
+class C {
+    void Foo(int n) {
+        if (n > 0) {
+            n += 1;
+            Foo(n);
+        }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Guard_Parameter_Is_Mutated_Via_Deconstruction()
+        {
+            var test = @"
+class C {
+    void Foo(int n) {
+        if (n > 0) {
+            (n, var x) = (n - 1, 0);
+            Foo(n);
+        }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task Warns_When_Call_Is_In_Try_Body()
+        {
+            // A try body executes unconditionally, so unconditional recursion in it is still a bug.
+            var test = @"
+class C {
+    void Foo() {
+        try {
+            [|Foo()|];
+        } catch { }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Call_Is_In_Catch()
+        {
+            var test = @"
+using System;
+class C {
+    void Foo() {
+        try { } catch (Exception) { Foo(); }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Call_Is_In_Finally()
+        {
+            var test = @"
+class C {
+    void Foo() {
+        try { } finally { Foo(); }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Guard_Depends_On_Instance_Field()
+        {
+            var test = @"
+class C {
+    private bool _flag;
+    void Foo() {
+        if (_flag) Foo();
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Guard_Depends_On_Property()
+        {
+            var test = @"
+class C {
+    private bool Flag { get; set; }
+    void Foo() {
+        if (Flag) Foo();
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Conditional_Early_Return_Terminates()
+        {
+            // Issue: 'if (n > 10) return;' can terminate the recursion before the call.
+            var test = @"
+class C {
+    void Foo(int n) {
+        n++;
+        if (n > 10) return;
+        Foo(n);
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Conditional_Throw_Terminates()
+        {
+            var test = @"
+using System;
+class C {
+    void Foo(int n) {
+        if (n > 10) throw new InvalidOperationException();
+        Foo(n);
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Termination_Depends_On_Instance_State()
+        {
+            var test = @"
+class C {
+    private bool _done;
+    void Foo() {
+        if (_done) return;
+        Foo();
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task NoWarn_When_Call_Is_Inside_Switch()
+        {
+            // We don't attempt to prove invariance through switch statements, so we stay safe and
+            // do not warn.
+            var test = @"
+class C {
+    void Foo(int n) {
+        switch (n) {
+            case 0: Foo(n); break;
+        }
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
+        [Test]
+        public async Task Warns_When_Unconditional_After_StraightLine_Statements()
+        {
+            var test = @"
+using System;
+class C {
+    void Foo(int n) {
+        Console.WriteLine(n);
+        [|Foo(n)|];
+    }
+}
+";
+            await Verify.VerifyAsync(test);
+        }
+
         [Test]
         public async Task NoWarn_When_Different_Argument_Is_Passed()
         {