From 2da37bb8883122af5e57f4e69842e1bb4f896e4f Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Wed, 30 Jul 2025 16:21:46 -0700 Subject: [PATCH] Improve recursive call analysis for ref parameters Enhances RecursiveCallAnalyzer to avoid false positives when ref parameters are modified or passed to other methods before recursive calls. Adds logic to distinguish calls on different instances and updates tests to cover these scenarios. Plus downgraded the compiler packages because they cause issues when adopting them. --- src/Directory.Packages.props | 10 +- .../RecursiveCallAnalyzerTests.cs | 70 ++++++++++++ .../RecursiveCallAnalyzer.cs | 104 +++++++++++++++++- 3 files changed, 178 insertions(+), 6 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 10fc57b..6137556 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -7,12 +7,12 @@ - - + + - - - + + + diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs index c82e496..0a5c88c 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/RecursiveCallAnalyzerTests.cs @@ -22,6 +22,76 @@ void Foo() { await Verify.VerifyAsync(test); } + [Test] + public async Task NoWarn_On_Different_Instance() + { + var test = @" +public class Node +{ + public void Foo() { Parent?.Foo();} + public Node Parent { get; set; } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_With_Ref_Parameter_When_Touched() + { + var test = @" +public class Node +{ + public void Foo(ref int x) + { + Bar(ref x); + Foo(ref x); + } + + private void Bar(ref int x) + { + x++; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_With_Ref_Parameter_When_Changed() + { + var test = @" +public class Node +{ + public void Foo(ref int x) + { + x = 42; + Foo(ref x); + } + + private void Bar(ref int x) + { + x++; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task Warn_With_Ref_Parameter_When_Not_Touched() + { + var test = @" +public class Node +{ + public void Foo(ref int x) + { + [|Foo(ref x)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + [Test] public async Task WarnsOnConditionalRecursiveCall() { diff --git a/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs index 0cc6b23..5b1b8e9 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; @@ -24,6 +25,10 @@ private static void AnalyzeMethodBody(OperationAnalysisContext context) { var method = (IMethodSymbol)context.ContainingSymbol; var methodBody = (IMethodBodyOperation)context.Operation; + + // Find all ref parameters that have been "touched" (modified or passed to other methods) + var touchedRefParameters = GetTouchedRefParameters(methodBody, method); + foreach (var invocation in methodBody.Descendants().OfType()) { // Check if all parameters are passed as-is @@ -32,13 +37,22 @@ private static void AnalyzeMethodBody(OperationAnalysisContext context) // Checking that the method is the same. SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.OriginalDefinition, method.OriginalDefinition) && + // Check if the method is being called on the same instance + // For instance methods, we need to check if the receiver is 'this' or implicit (null) + // For static methods, there's no instance to check + IsCalledOnSameInstance(invocation, method) && + // Checking if the parameters are passed as is. // It is possible to have a false positive here if the parameters are mutable. // But it is a very rare case, so we will ignore it for now. invocation.Arguments.Zip(method.Parameters, (arg, param) => arg.Value is IParameterReferenceOperation paramRef && SymbolEqualityComparer.Default.Equals(paramRef.Parameter, param) - ).All(b => b)) + ).All(b => b) && + + // For ref parameters, check if they were touched before this call + // If any ref parameter was touched, don't warn + !HasTouchedRefParameterBeforeCall(invocation, method, touchedRefParameters)) { context.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.EPC30, @@ -47,5 +61,93 @@ arg.Value is IParameterReferenceOperation paramRef && } } } + + private static bool HasTouchedRefParameterBeforeCall(IInvocationOperation recursiveCall, IMethodSymbol method, HashSet touchedRefParameters) + { + // Check if any ref parameter in the recursive call was touched + for (int i = 0; i < recursiveCall.Arguments.Length; i++) + { + var arg = recursiveCall.Arguments[i]; + var param = method.Parameters[i]; + + // If this is a ref parameter and it's passed as-is, check if it was touched + if (param.RefKind == RefKind.Ref && + arg.Value is IParameterReferenceOperation paramRef && + SymbolEqualityComparer.Default.Equals(paramRef.Parameter, param) && + touchedRefParameters.Contains(param)) + { + return true; + } + } + + return false; + } + + private static HashSet GetTouchedRefParameters(IMethodBodyOperation methodBody, IMethodSymbol method) + { + var touchedParams = new HashSet(SymbolEqualityComparer.Default); + + // Look for assignments to ref parameters + foreach (var assignment in methodBody.Descendants().OfType()) + { + if (assignment.Target is IParameterReferenceOperation paramRef && + paramRef.Parameter.RefKind == RefKind.Ref) + { + touchedParams.Add(paramRef.Parameter); + } + } + + // Look for ref parameters being passed to other methods + foreach (var invocation in methodBody.Descendants().OfType()) + { + // Skip the method itself to avoid checking recursive calls + if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.OriginalDefinition, method.OriginalDefinition)) + { + continue; + } + + foreach (var arg in invocation.Arguments) + { + // Check if a ref parameter is being passed as ref/out to another method + if (arg.ArgumentKind == ArgumentKind.Explicit && + (arg.Parameter?.RefKind == RefKind.Ref || arg.Parameter?.RefKind == RefKind.Out) && + arg.Value is IParameterReferenceOperation paramRef && + paramRef.Parameter.RefKind == RefKind.Ref) + { + touchedParams.Add(paramRef.Parameter); + } + } + } + + return touchedParams; + } + + private static bool IsCalledOnSameInstance(IInvocationOperation invocation, IMethodSymbol containingMethod) + { + // For static methods, there's no instance to check, so any call to the same static method is recursive + if (containingMethod.IsStatic) + { + return true; + } + + // For instance methods, check if the receiver is 'this' (implicit or explicit) + var receiver = invocation.Instance; + + // If receiver is null, it means it's an implicit 'this' call (e.g., just Foo() instead of this.Foo()) + if (receiver == null) + { + return true; + } + + // If receiver is an explicit 'this' reference + if (receiver is IInstanceReferenceOperation instanceRef && + instanceRef.ReferenceKind == InstanceReferenceKind.ContainingTypeInstance) + { + return true; + } + + // Any other receiver (like Parent?.Foo()) means it's called on a different instance + return false; + } } }