Skip to content

Commit

Permalink
Merge pull request #604 from manfred-brands/issue602_StackOverflow
Browse files Browse the repository at this point in the history 
Issue602 stack overflow
  • Loading branch information
@mikkelbu
mikkelbu authored Oct 15, 2023
2 parents 7130e0b + ab8167d commit 1d21b00
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 12 deletions.
34 changes: 34 additions & 0 deletions ...s.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,5 +401,39 @@ public void Test()

RoslynAssert.Suppressed(suppressor, testCode);
}

[Test]
public void ShouldDealWithRecursiveMethods()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
private Dictionary<string, bool> _values = new();
private string ↓_lastOne;
[SetUp]
public void SetUp()
{
Recurse(""SetUp"");
}
[Test]
public void MinimalRepro()
{
Recurse(""help"");
}
private void Recurse(string one, bool keepGoing = true)
{
_values[one] = keepGoing;
if (!keepGoing)
{
return;
}
Recurse(one, false);
_lastOne = one;
}
", "using System.Collections.Generic;");

RoslynAssert.Suppressed(suppressor, testCode);
}
}
}
59 changes: 59 additions & 0 deletions ...DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -771,5 +771,64 @@ public void Test()
// InstancePerTestCase mean test should use IDisposable
RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void ShouldDealWithDirectRecursiveMethods()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
private Dictionary<string, bool> _values = new();
[Test]
public void MinimalRepro()
{
Recurse(""help"");
}
private void Recurse(string one, bool keepGoing = true)
{
_values[one] = keepGoing;
if (keepGoing)
{
Recurse(one, false);
}
}
", "using System.Collections.Generic;");

RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void ShouldDealWithIndirectRecursiveMethods()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
private Dictionary<string, string> _values = new();
[Test]
public void Indirect()
{
A(""help"");
}
private void A(string one, bool keepGoing = true)
{
if (keepGoing)
{
_values[one] = nameof(A);
B(one, false);
}
}
private void B(string one, bool keepGoing)
{
if (keepGoing)
{
_values[one] = nameof(B);
A(one, false);
}
}
", "using System.Collections.Generic;");

RoslynAssert.Valid(analyzer, testCode);
}
}
}
29 changes: 19 additions & 10 deletions ...it.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
if (isSetup)
{
// Find (OneTime)SetUps method and check for assignment to this field.
if (IsAssignedIn(model, classDeclaration, method, fieldOrPropertyName))
HashSet<MethodDeclarationSyntax> visitedMethods = new();
if (IsAssignedIn(model, classDeclaration, visitedMethods, method, fieldOrPropertyName))
{
context.ReportSuppression(Suppression.Create(NullableFieldOrPropertyInitializedInSetUp, diagnostic));
}
Expand All @@ -119,17 +120,18 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
HashSet<MethodDeclarationSyntax> visitedMethods,
MethodDeclarationSyntax method,
string fieldOrPropertyName)
{
if (method.ExpressionBody is not null)
{
return IsAssignedIn(model, classDeclaration, method.ExpressionBody.Expression, fieldOrPropertyName);
return IsAssignedIn(model, classDeclaration, visitedMethods, method.ExpressionBody.Expression, fieldOrPropertyName);
}

if (method.Body is not null)
{
return IsAssignedIn(model, classDeclaration, method.Body, fieldOrPropertyName);
return IsAssignedIn(model, classDeclaration, visitedMethods, method.Body, fieldOrPropertyName);
}

return false;
Expand All @@ -138,21 +140,22 @@ private static bool IsAssignedIn(
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
HashSet<MethodDeclarationSyntax> visitedMethods,
StatementSyntax statement,
string fieldOrPropertyName)
{
switch (statement)
{
case ExpressionStatementSyntax expressionStatement:
return IsAssignedIn(model, classDeclaration, expressionStatement.Expression, fieldOrPropertyName);
return IsAssignedIn(model, classDeclaration, visitedMethods, expressionStatement.Expression, fieldOrPropertyName);

case BlockSyntax block:
return IsAssignedIn(model, classDeclaration, block.Statements, fieldOrPropertyName);
return IsAssignedIn(model, classDeclaration, visitedMethods, block.Statements, fieldOrPropertyName);

case TryStatementSyntax tryStatement:
return IsAssignedIn(model, classDeclaration, tryStatement.Block, fieldOrPropertyName) ||
return IsAssignedIn(model, classDeclaration, visitedMethods, tryStatement.Block, fieldOrPropertyName) ||
(tryStatement.Finally is not null &&
IsAssignedIn(model, classDeclaration, tryStatement.Finally.Block, fieldOrPropertyName));
IsAssignedIn(model, classDeclaration, visitedMethods, tryStatement.Finally.Block, fieldOrPropertyName));

default:
// Any conditional statement does not guarantee assignment.
Expand All @@ -163,12 +166,13 @@ private static bool IsAssignedIn(
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
HashSet<MethodDeclarationSyntax> visitedMethods,
SyntaxList<StatementSyntax> statements,
string fieldOrPropertyName)
{
foreach (var statement in statements)
{
if (IsAssignedIn(model, classDeclaration, statement, fieldOrPropertyName))
if (IsAssignedIn(model, classDeclaration, visitedMethods, statement, fieldOrPropertyName))
return true;
}

Expand All @@ -178,6 +182,7 @@ private static bool IsAssignedIn(
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
HashSet<MethodDeclarationSyntax> visitedMethods,
InvocationExpressionSyntax invocationExpression,
string fieldOrPropertyName)
{
Expand All @@ -190,7 +195,10 @@ private static bool IsAssignedIn(
if (method?.Parent == classDeclaration)
{
// We only get here if the method is in our source code and our class.
return IsAssignedIn(model, classDeclaration, method, fieldOrPropertyName);
if (visitedMethods.Add(method))
{
return IsAssignedIn(model, classDeclaration, visitedMethods, method, fieldOrPropertyName);
}
}

return false;
Expand All @@ -199,6 +207,7 @@ private static bool IsAssignedIn(
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
HashSet<MethodDeclarationSyntax> visitedMethods,
ExpressionSyntax? expressionStatement,
string fieldOrPropertyName)
{
Expand Down Expand Up @@ -245,7 +254,7 @@ memberAccessExpression.Expression is InvocationExpressionSyntax awaitedInvocatio
string? identifier = GetIdentifier(invocationExpression.Expression);

if (!string.IsNullOrEmpty(identifier) &&
IsAssignedIn(model, classDeclaration, invocationExpression, fieldOrPropertyName))
IsAssignedIn(model, classDeclaration, visitedMethods, invocationExpression, fieldOrPropertyName))
{
return true;
}
Expand Down
17 changes: 15 additions & 2 deletions ...zers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ private static void AnalyzeAssignedButNotDisposed(

private static void AssignedIn(Parameters parameters, HashSet<string> assignments, IEnumerable<IMethodSymbol> methods)
{
parameters.ResetMethodCallVisits();
foreach (var method in methods)
{
AssignedIn(parameters, assignments, method);
Expand Down Expand Up @@ -263,7 +264,7 @@ memberAccessExpression.Expression is InvocationExpressionSyntax awaitedInvocatio
string? method = GetIdentifier(invocationExpression.Expression);
if (method is not null)
{
if (parameters.IsLocalMethodCall(invocationExpression, out IMethodSymbol? calledMethod))
if (parameters.IsLocalNotYetVisitedMethodCall(invocationExpression, out IMethodSymbol? calledMethod))
{
// We are calling a local method on our class, keep looking for assignments.
AssignedIn(parameters, assignments, calledMethod);
Expand Down Expand Up @@ -362,6 +363,7 @@ private static bool IsDisposableTypeNotRequiringToBeDisposed(ITypeSymbol typeSym

private static void DisposedIn(Parameters parameters, HashSet<string> disposals, IEnumerable<IMethodSymbol> methods)
{
parameters.ResetMethodCallVisits();
foreach (var method in methods)
{
DisposedIn(parameters, disposals, method);
Expand Down Expand Up @@ -408,7 +410,7 @@ awaitInvocationExpression.Expression is MemberAccessExpressionSyntax awaitMember
{
disposals.Add(disposedSymbol);
}
else if (parameters.IsLocalMethodCall(invocationExpression, out IMethodSymbol? calledMethod))
else if (parameters.IsLocalNotYetVisitedMethodCall(invocationExpression, out IMethodSymbol? calledMethod))
{
// We are calling a local method on our class, keep looking for disposals.
DisposedIn(parameters, disposals, calledMethod);
Expand Down Expand Up @@ -521,6 +523,7 @@ private sealed class Parameters
private readonly INamedTypeSymbol type;
private readonly ImmutableHashSet<string> disposeMethods;
private readonly HashSet<string> names;
private readonly HashSet<IMethodSymbol> visitedMethods = new(SymbolEqualityComparer.Default);

public Parameters(SemanticModel model, INamedTypeSymbol type, ImmutableHashSet<string> disposeMethods, HashSet<string> names)
{
Expand All @@ -541,6 +544,16 @@ public bool IsLocalMethodCall(
SymbolEqualityComparer.Default.Equals(calledMethod.ContainingType, this.type);
}

public void ResetMethodCallVisits() => this.visitedMethods.Clear();

public bool IsLocalNotYetVisitedMethodCall(
InvocationExpressionSyntax invocationExpression,
[NotNullWhen(true)] out IMethodSymbol? calledMethod)
{
return this.IsLocalMethodCall(invocationExpression, out calledMethod) &&
this.visitedMethods.Add(calledMethod);
}

public bool IsDisposalOf(
InvocationExpressionSyntax invocationExpression,
ExpressionSyntax? conditionalTarget,
Expand Down

0 comments on commit 1d21b00

Please sign in to comment.