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; + } } }