Improve EPC30 reliability: only warn on provably non-terminating recursion#328
Conversation
…rsion
RecursiveCallAnalyzer now reports self-recursion only when it is provably non-terminating: either the call is unconditional, or it is guarded solely by an invariant condition (an if/ternary or top-tested while whose condition is composed purely of unchanged value parameters and constants), e.g. 'if (b) Foo(b);'.
This removes false positives such as conditional early-return termination ('if (n > 10) return; Foo(n);'), instance-state/property guards, and changed/mutated arguments ('n--; Foo(n);'), while still catching genuine infinite recursion. Adds tests, a sample, and docs/release-note updates.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the EPC30 RecursiveCallAnalyzer to become control-flow aware, aiming to report only self-recursion that is provably non-terminating (unconditional reachability or invariant-guarded reachability), and expands documentation/samples/tests accordingly.
Changes:
- Add reachability/invariance analysis (
ShouldReportRecursion, invariant-guard detection, and mutated-parameter tracking) to suppress EPC30 false positives. - Extend unit tests to cover unconditional, invariant-guarded, and various “no warn” scenarios.
- Add an EPC30 sample and update EPC30 documentation and release notes to reflect the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs |
Implements control-flow-aware recursion reporting via invariant-guard detection and termination/branching suppression. |
src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs |
Adds/renames test cases to validate new EPC30 reporting and suppression behavior. |
src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj |
Updates release notes text to describe the EPC30 behavior change. |
samples/ErrorProne.Samples/CoreAnalyzers/RecursiveCallSample.cs |
Adds a sample demonstrating new EPC30 “warn” and “no warn” cases. |
docs/Rules/EPC30.md |
Updates the rule documentation to describe “provably non-terminating” criteria and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (op) | ||
| { | ||
| 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; | ||
| } |
There was a problem hiding this comment.
Good catch on deconstruction — fixed in e588785. Note that compound (
+= 1) and coalesce (
??= ...) assignments were already covered: both ICompoundAssignmentOperation and ICoalesceAssignmentOperation derive from IAssignmentOperation, so the existing IAssignmentOperation { Target: IParameterReferenceOperation } case matches them. Deconstruction ((n, x) = ...) was the real gap because its target is a tuple; added explicit handling that collects every parameter written by the deconstruction.
| ILoopOperation or // for/foreach/while/do | ||
| ITryOperation or // try/catch/finally | ||
| ICoalesceOperation or // '??' | ||
| IConditionalAccessOperation; // '?.' |
There was a problem hiding this comment.
Agreed — fixed in e588785. A ry body is no longer treated as branching: recursion directly in a try body is reported again, while calls in catch/ inally remain suppressed (those paths are conditional). Added tests for all three.
…low try-body recursion - Remove duplicated <summary> tag in RecursiveCallAnalyzer XML docs. - GetMutatedParameters now also detects parameters mutated via deconstruction assignments (compound '+=' and coalesce '??=' were already covered by the IAssignmentOperation base type). - Treat a 'try' body as unconditional: recursion directly in a try body is reported again, while calls in 'catch'/'finally' remain suppressed. - Add tests for the above and update EPC30 docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
RecursiveCallAnalyzer(EPC30) previously warned on any self-call that passed all parameters as-is, ignoring control flow. That produced false positives whenever recursion was conditional or terminable. This PR makes the analyzer control-flow aware so it only reports recursion that is provably non-terminating.EPC30 now reports a self-call when either:
if/ternary or top-testedwhilewhose condition is composed purely of unchanged value parameters and constants/operators. Once such a branch is taken it is taken forever ⇒ infinite recursion (e.g.if (b) Foo(b);,if (n > 0) Foo(n);).Now warns
void Foo() { Foo(); }— unconditionalif (b) Foo(b);,if (n > 0) Foo(n);— invariant guardreturn b ? Foo(b) : 0;— invariant ternarywhile (n > 0) Foo(n);— invariant loop conditionNo longer warns (false positives removed)
n++; if (n > 10) return; Foo(n);— conditional early-return terminationif (_done) return; Foo();,if (Flag) Foo();— instance-state / property guardsif (n > 0) { n--; Foo(n); }— mutated argument (guard not invariant)switch, non-trivial loops, and touchedrefparametersWhen in doubt, the analyzer favors not reporting to keep false positives low.
Implementation
New helpers in
RecursiveCallAnalyzer.cs:ShouldReportRecursion,IsBranchingOperation,IsInvariantGuard,IsInvariantCondition,IsAllowedInvariantOperation,ContainsTerminatingOrBranchingFlow,GetMutatedParameters,DescendantsAndSelf. Parameters mutated anywhere in the body (assigned,++/--, or passed byref/out) cannot make a guard invariant, so decreasing-argument recursions stay suppressed. Existingref-parameter and nested-lambda suppression is unchanged.Tests
RecursiveCallAnalyzerTestsextended to 24 cases covering both the warning cases (unconditional + invariant-guarded) and the no-warn cases (terminating, instance-state, property, mutated-argument, switch).Also
samples/ErrorProne.Samples/CoreAnalyzers/RecursiveCallSample.csdocs/Rules/EPC30.mdand CodeFixes release notes