diff --git a/docs/Rules/EPC30.md b/docs/Rules/EPC30.md index 832b3ef..5e129b1 100644 --- a/docs/Rules/EPC30.md +++ b/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 + } } } ``` diff --git a/samples/ErrorProne.Samples/CoreAnalyzers/RecursiveCallSample.cs b/samples/ErrorProne.Samples/CoreAnalyzers/RecursiveCallSample.cs new file mode 100644 index 0000000..e1f1a2b --- /dev/null +++ b/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 + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj b/src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj index a09be0b..49a6aa2 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj +++ b/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 diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs index cc43e0c..9b50ece 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs +++ b/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() { diff --git a/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs index 31ff73b..e377839 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs @@ -28,6 +28,9 @@ private static void AnalyzeMethodBody(OperationAnalysisContext context) // Find all ref parameters that have been "touched" (modified or passed to other methods) var touchedRefParameters = GetTouchedRefParameters(methodBody, method); + + // Parameters that are mutated anywhere in the body cannot make a guard "invariant". + var mutatedParameters = GetMutatedParameters(methodBody); foreach (var invocation in methodBody.Descendants().OfType()) { @@ -59,7 +62,18 @@ arg.Value is IParameterReferenceOperation paramRef && // For ref parameters, check if they were touched before this call // If any ref parameter was touched, don't warn - !HasTouchedRefParameterBeforeCall(invocation, method, touchedRefParameters)) + !HasTouchedRefParameterBeforeCall(invocation, method, touchedRefParameters) && + + // Only warn when the recursive call is guaranteed to be reached and to + // recurse forever. That means either: + // * the call is unconditional (no branching could terminate the method first), or + // * the call is guarded only by "invariant" conditions -- conditions composed + // purely of unchanged value parameters and constants -- so taking the branch + // once guarantees taking it forever (e.g. 'if (b) Foo(b);'). + // Anything that could terminate the recursion (early returns, instance-state + // checks, mutated arguments, method calls in the guard, etc.) suppresses the + // diagnostic to avoid false positives. + ShouldReportRecursion(invocation, methodBody, mutatedParameters)) { context.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.EPC30, @@ -82,6 +96,176 @@ private static bool IsInsideNestedFunction(IOperation operation) return false; } + /// + /// Returns true when the recursive is guaranteed to be + /// reached and to recurse forever: either it is reached unconditionally, or every branching + /// construct enclosing it is an "invariant guard" (a condition that, once taken, is always + /// taken because it depends only on unchanged value parameters and constants). Any statement + /// before the call that could terminate or branch the method suppresses the diagnostic. + /// + private static bool ShouldReportRecursion( + IInvocationOperation call, + IMethodBodyOperation methodBody, + HashSet mutatedParameters) + { + IOperation child = call; + for (IOperation? parent = call.Parent; parent != null; parent = parent.Parent) + { + // The call is nested inside a branching construct. Only keep going if that construct + // is an invariant guard that still guarantees infinite recursion. + if (IsBranchingOperation(parent) && !IsInvariantGuard(parent, mutatedParameters)) + { + return false; + } + + // A 'try' body executes unconditionally, so recursion directly in it is still + // reachable. Only the 'catch'/'finally' paths are conditional, so suppress those. + if (parent is ITryOperation tryOperation && !ReferenceEquals(child, tryOperation.Body)) + { + return false; + } + + // For an enclosing block, any statement executed before the call that can terminate + // or branch the method (e.g. 'if (x) return;') means the recursion might not happen. + if (parent is IBlockOperation block) + { + foreach (var statement in block.Operations) + { + if (ReferenceEquals(statement, child)) + { + break; + } + + if (ContainsTerminatingOrBranchingFlow(statement)) + { + return false; + } + } + } + + if (ReferenceEquals(parent, methodBody)) + { + break; + } + + child = parent; + } + + return true; + } + + /// + /// Branching constructs that, when an operation is nested within them, mean the operation + /// does not execute unconditionally. + /// + private static bool IsBranchingOperation(IOperation operation) + { + return operation is + IConditionalOperation or // 'if' statement and '?:' ternary + ISwitchOperation or + ISwitchExpressionOperation or + ILoopOperation or // for/foreach/while/do + ICoalesceOperation or // '??' + IConditionalAccessOperation; // '?.' + } + + /// + /// Returns true if taking the branch represented by is + /// guaranteed to be taken again on the next recursion, i.e. its condition is "invariant". + /// Only simple if/ternary and top-tested while guards are considered; switches, + /// loops with side effects, try/catch and null-conditional operators are never treated as + /// invariant (we suppress to stay on the safe side). + /// + private static bool IsInvariantGuard(IOperation branching, HashSet mutatedParameters) + { + switch (branching) + { + case IConditionalOperation conditional: + return IsInvariantCondition(conditional.Condition, mutatedParameters); + + case IWhileLoopOperation { ConditionIsTop: true, ConditionIsUntil: false, Condition: { } condition }: + return IsInvariantCondition(condition, mutatedParameters); + + default: + return false; + } + } + + /// + /// A condition is invariant when it is composed purely of references to unchanged value + /// parameters, constants, comparisons and boolean/arithmetic operators. References to + /// instance state, properties, locals, method calls, or mutated parameters make it + /// non-invariant (so the recursion may terminate and we don't warn). + /// + private static bool IsInvariantCondition(IOperation condition, HashSet mutatedParameters) + { + foreach (var op in DescendantsAndSelf(condition)) + { + if (!IsAllowedInvariantOperation(op, mutatedParameters)) + { + return false; + } + } + + return true; + } + + private static bool IsAllowedInvariantOperation(IOperation operation, HashSet mutatedParameters) + { + switch (operation) + { + case IParameterReferenceOperation parameterReference: + // Only by-value parameters that are never mutated keep the condition invariant. + return parameterReference.Parameter.RefKind == RefKind.None && + !mutatedParameters.Contains(parameterReference.Parameter); + + case ILiteralOperation: + case IBinaryOperation: + case IUnaryOperation: + case IParenthesizedOperation: + case IConversionOperation: + return true; + + default: + return false; + } + } + + /// + /// Returns true if the statement subtree contains control flow that can terminate + /// or branch the method (conditionals, switches, loops, try, return, throw, break, + /// continue, goto). + /// + private static bool ContainsTerminatingOrBranchingFlow(IOperation statement) + { + foreach (var op in DescendantsAndSelf(statement)) + { + if (op is + IConditionalOperation or + ISwitchOperation or + ISwitchExpressionOperation or + ILoopOperation or + ITryOperation or + IReturnOperation or + IThrowOperation or + IBranchOperation) // break/continue/goto + { + return true; + } + } + + return false; + } + + private static IEnumerable DescendantsAndSelf(IOperation operation) + { + yield return operation; + foreach (var descendant in operation.Descendants()) + { + yield return descendant; + } + } + private static bool HasTouchedRefParameterBeforeCall(IInvocationOperation recursiveCall, IMethodSymbol method, HashSet touchedRefParameters) { // Check if any ref parameter in the recursive call was touched @@ -103,6 +287,50 @@ arg.Value is IParameterReferenceOperation paramRef && return false; } + /// + /// Returns every parameter that is mutated anywhere in the body: assigned to (including + /// compound '+=', coalesce '??=', and deconstruction assignments), incremented/decremented, + /// or passed by ref/out to another method. + /// + private static HashSet GetMutatedParameters(IMethodBodyOperation methodBody) + { + var mutated = new HashSet(SymbolEqualityComparer.Default); + + foreach (var op in methodBody.Descendants()) + { + switch (op) + { + // Deconstruction targets are tuples, e.g. '(n, x) = ...'; collect every + // parameter written by the deconstruction. + case IDeconstructionAssignmentOperation deconstruction: + foreach (var target in DescendantsAndSelf(deconstruction.Target)) + { + if (target is IParameterReferenceOperation deconstructedParam) + { + mutated.Add(deconstructedParam.Parameter); + } + } + break; + + // Covers simple '=', compound '+=' etc., and coalesce '??=' assignments. + case IAssignmentOperation { Target: IParameterReferenceOperation assignedParam }: + mutated.Add(assignedParam.Parameter); + break; + + case IIncrementOrDecrementOperation { Target: IParameterReferenceOperation incrementedParam }: + mutated.Add(incrementedParam.Parameter); + break; + + case IArgumentOperation { Value: IParameterReferenceOperation refArg } argument + when argument.Parameter?.RefKind is RefKind.Ref or RefKind.Out: + mutated.Add(refArg.Parameter); + break; + } + } + + return mutated; + } + private static HashSet GetTouchedRefParameters(IMethodBodyOperation methodBody, IMethodSymbol method) { var touchedParams = new HashSet(SymbolEqualityComparer.Default);