Skip to content

Commit d3333d0

Browse files
SergeyTeplyakovSergeyTeplyakov
authored
Improve recursive call analysis for ref parameters (#315)
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.
1 parent 9652517 commit d3333d0

File tree

3 files changed

+178
-6
lines changed

3 files changed

+178
-6
lines changed

‎src/Directory.Packages.props‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@
77
</PropertyGroup>
88
<ItemGroup>
99
<!-- Analyzers -->
10-
<PackageVersion Include="ErrorProne.NET.CoreAnalyzers" Version="0.6.1-beta.1" />
11-
<PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="4.14.0" />
10+
<PackageVersion Include="ErrorProne.NET.CoreAnalyzers" Version="0.8.0-beta.1" />
11+
<PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="4.13.0" />
1212
<PackageVersion Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="9.0.0" />
13-
<PackageVersion Include="Microsoft.CodeAnalysis" Version="4.14.0" />
14-
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.14.0" />
15-
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.14.0" />
13+
<PackageVersion Include="Microsoft.CodeAnalysis" Version="4.13.0" />
14+
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.13.0" />
15+
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.13.0" />
1616
<PackageVersion Include="ILRepack.Lib.MSBuild.Task" Version="2.0.43" />
1717
<PackageVersion Include="Nerdbank.GitVersioning" Version="3.7.115" />
1818
<PackageVersion Include="RuntimeContracts" Version="0.5.0" />

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,76 @@ void Foo() {
2222
await Verify.VerifyAsync(test);
2323
}
2424

25+
[Test]
26+
public async Task NoWarn_On_Different_Instance()
27+
{
28+
var test = @"
29+
public class Node
30+
{
31+
public void Foo() { Parent?.Foo();}
32+
public Node Parent { get; set; }
33+
}
34+
";
35+
await Verify.VerifyAsync(test);
36+
}
37+
38+
[Test]
39+
public async Task NoWarn_With_Ref_Parameter_When_Touched()
40+
{
41+
var test = @"
42+
public class Node
43+
{
44+
public void Foo(ref int x)
45+
{
46+
Bar(ref x);
47+
Foo(ref x);
48+
}
49+
50+
private void Bar(ref int x)
51+
{
52+
x++;
53+
}
54+
}
55+
";
56+
await Verify.VerifyAsync(test);
57+
}
58+
59+
[Test]
60+
public async Task NoWarn_With_Ref_Parameter_When_Changed()
61+
{
62+
var test = @"
63+
public class Node
64+
{
65+
public void Foo(ref int x)
66+
{
67+
x = 42;
68+
Foo(ref x);
69+
}
70+
71+
private void Bar(ref int x)
72+
{
73+
x++;
74+
}
75+
}
76+
";
77+
await Verify.VerifyAsync(test);
78+
}
79+
80+
[Test]
81+
public async Task Warn_With_Ref_Parameter_When_Not_Touched()
82+
{
83+
var test = @"
84+
public class Node
85+
{
86+
public void Foo(ref int x)
87+
{
88+
[|Foo(ref x)|];
89+
}
90+
}
91+
";
92+
await Verify.VerifyAsync(test);
93+
}
94+
2595
[Test]
2696
public async Task WarnsOnConditionalRecursiveCall()
2797
{

‎src/ErrorProne.NET.CoreAnalyzers/RecursiveCallAnalyzer.cs‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Collections.Generic;
12
using System.Collections.Immutable;
23
using System.Linq;
34
using Microsoft.CodeAnalysis;
@@ -24,6 +25,10 @@ private static void AnalyzeMethodBody(OperationAnalysisContext context)
2425
{
2526
var method = (IMethodSymbol)context.ContainingSymbol;
2627
var methodBody = (IMethodBodyOperation)context.Operation;
28+
29+
// Find all ref parameters that have been "touched" (modified or passed to other methods)
30+
var touchedRefParameters = GetTouchedRefParameters(methodBody, method);
31+
2732
foreach (var invocation in methodBody.Descendants().OfType<IInvocationOperation>())
2833
{
2934
// Check if all parameters are passed as-is
@@ -32,13 +37,22 @@ private static void AnalyzeMethodBody(OperationAnalysisContext context)
3237
// Checking that the method is the same.
3338
SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.OriginalDefinition, method.OriginalDefinition) &&
3439

40+
// Check if the method is being called on the same instance
41+
// For instance methods, we need to check if the receiver is 'this' or implicit (null)
42+
// For static methods, there's no instance to check
43+
IsCalledOnSameInstance(invocation, method) &&
44+
3545
// Checking if the parameters are passed as is.
3646
// It is possible to have a false positive here if the parameters are mutable.
3747
// But it is a very rare case, so we will ignore it for now.
3848
invocation.Arguments.Zip(method.Parameters, (arg, param) =>
3949
arg.Value is IParameterReferenceOperation paramRef &&
4050
SymbolEqualityComparer.Default.Equals(paramRef.Parameter, param)
41-
).All(b => b))
51+
).All(b => b) &&
52+
53+
// For ref parameters, check if they were touched before this call
54+
// If any ref parameter was touched, don't warn
55+
!HasTouchedRefParameterBeforeCall(invocation, method, touchedRefParameters))
4256
{
4357
context.ReportDiagnostic(Diagnostic.Create(
4458
DiagnosticDescriptors.EPC30,
@@ -47,5 +61,93 @@ arg.Value is IParameterReferenceOperation paramRef &&
4761
}
4862
}
4963
}
64+
65+
private static bool HasTouchedRefParameterBeforeCall(IInvocationOperation recursiveCall, IMethodSymbol method, HashSet<IParameterSymbol> touchedRefParameters)
66+
{
67+
// Check if any ref parameter in the recursive call was touched
68+
for (int i = 0; i < recursiveCall.Arguments.Length; i++)
69+
{
70+
var arg = recursiveCall.Arguments[i];
71+
var param = method.Parameters[i];
72+
73+
// If this is a ref parameter and it's passed as-is, check if it was touched
74+
if (param.RefKind == RefKind.Ref &&
75+
arg.Value is IParameterReferenceOperation paramRef &&
76+
SymbolEqualityComparer.Default.Equals(paramRef.Parameter, param) &&
77+
touchedRefParameters.Contains(param))
78+
{
79+
return true;
80+
}
81+
}
82+
83+
return false;
84+
}
85+
86+
private static HashSet<IParameterSymbol> GetTouchedRefParameters(IMethodBodyOperation methodBody, IMethodSymbol method)
87+
{
88+
var touchedParams = new HashSet<IParameterSymbol>(SymbolEqualityComparer.Default);
89+
90+
// Look for assignments to ref parameters
91+
foreach (var assignment in methodBody.Descendants().OfType<IAssignmentOperation>())
92+
{
93+
if (assignment.Target is IParameterReferenceOperation paramRef &&
94+
paramRef.Parameter.RefKind == RefKind.Ref)
95+
{
96+
touchedParams.Add(paramRef.Parameter);
97+
}
98+
}
99+
100+
// Look for ref parameters being passed to other methods
101+
foreach (var invocation in methodBody.Descendants().OfType<IInvocationOperation>())
102+
{
103+
// Skip the method itself to avoid checking recursive calls
104+
if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.OriginalDefinition, method.OriginalDefinition))
105+
{
106+
continue;
107+
}
108+
109+
foreach (var arg in invocation.Arguments)
110+
{
111+
// Check if a ref parameter is being passed as ref/out to another method
112+
if (arg.ArgumentKind == ArgumentKind.Explicit &&
113+
(arg.Parameter?.RefKind == RefKind.Ref || arg.Parameter?.RefKind == RefKind.Out) &&
114+
arg.Value is IParameterReferenceOperation paramRef &&
115+
paramRef.Parameter.RefKind == RefKind.Ref)
116+
{
117+
touchedParams.Add(paramRef.Parameter);
118+
}
119+
}
120+
}
121+
122+
return touchedParams;
123+
}
124+
125+
private static bool IsCalledOnSameInstance(IInvocationOperation invocation, IMethodSymbol containingMethod)
126+
{
127+
// For static methods, there's no instance to check, so any call to the same static method is recursive
128+
if (containingMethod.IsStatic)
129+
{
130+
return true;
131+
}
132+
133+
// For instance methods, check if the receiver is 'this' (implicit or explicit)
134+
var receiver = invocation.Instance;
135+
136+
// If receiver is null, it means it's an implicit 'this' call (e.g., just Foo() instead of this.Foo())
137+
if (receiver == null)
138+
{
139+
return true;
140+
}
141+
142+
// If receiver is an explicit 'this' reference
143+
if (receiver is IInstanceReferenceOperation instanceRef &&
144+
instanceRef.ReferenceKind == InstanceReferenceKind.ContainingTypeInstance)
145+
{
146+
return true;
147+
}
148+
149+
// Any other receiver (like Parent?.Foo()) means it's called on a different instance
150+
return false;
151+
}
50152
}
51153
}

0 commit comments

Comments
 (0)