From b6d9d2baa15bce9adcf0aa8ad9616fd9b69f2cea Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Wed, 27 Sep 2023 15:17:10 +0800 Subject: [PATCH 1/3] DisposeFieldsAnalyzer: Recognize (indirect) recursive methods. --- ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 59 +++++++++++++++++++ ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 23 +++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs index 34106a31..89ad2065 100644 --- a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -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 _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 _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); + } } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index bdd26b58..11344a03 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -199,6 +199,7 @@ private static void AnalyzeAssignedButNotDisposed( private static void AssignedIn(Parameters parameters, HashSet assignments, IEnumerable methods) { + parameters.ResetMethodCallVisits(); foreach (var method in methods) { AssignedIn(parameters, assignments, method); @@ -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); @@ -362,6 +363,7 @@ private static bool IsDisposableTypeNotRequiringToBeDisposed(ITypeSymbol typeSym private static void DisposedIn(Parameters parameters, HashSet disposals, IEnumerable methods) { + parameters.ResetMethodCallVisits(); foreach (var method in methods) { DisposedIn(parameters, disposals, method); @@ -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); @@ -521,6 +523,7 @@ private sealed class Parameters private readonly INamedTypeSymbol type; private readonly ImmutableHashSet disposeMethods; private readonly HashSet names; + private readonly HashSet visitedMethods = new(SymbolEqualityComparer.Default); public Parameters(SemanticModel model, INamedTypeSymbol type, ImmutableHashSet disposeMethods, HashSet names) { @@ -541,6 +544,22 @@ 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) + { + if (this.IsLocalMethodCall(invocationExpression, out calledMethod) && + !this.visitedMethods.Contains(calledMethod)) + { + this.visitedMethods.Add(calledMethod); + return true; + } + + return false; + } + public bool IsDisposalOf( InvocationExpressionSyntax invocationExpression, ExpressionSyntax? conditionalTarget, From 3fc4980adec2e95cda1cfaad2f639ca076adfa9d Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Wed, 27 Sep 2023 15:35:31 +0800 Subject: [PATCH 2/3] NonNullabkeFieldsSuppressor: Recognize (indirect) recursive methods. --- ...rPropertyIsUninitializedSuppressorTests.cs | 34 +++++++++++++++++++ ...ieldOrPropertyIsUninitializedSuppressor.cs | 30 ++++++++++------ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/nunit.analyzers.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs b/src/nunit.analyzers.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs index f6c43527..8e7b4ec8 100644 --- a/src/nunit.analyzers.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs +++ b/src/nunit.analyzers.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs @@ -401,5 +401,39 @@ public void Test() RoslynAssert.Suppressed(suppressor, testCode); } + + [Test] + public void ShouldDealWithRecursiveMethods() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + private Dictionary _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); + } } } diff --git a/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs b/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs index 9e82444b..e7fcb19b 100644 --- a/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs +++ b/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs @@ -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 visitedMethods = new(); + if (IsAssignedIn(model, classDeclaration, visitedMethods, method, fieldOrPropertyName)) { context.ReportSuppression(Suppression.Create(NullableFieldOrPropertyInitializedInSetUp, diagnostic)); } @@ -119,17 +120,18 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) private static bool IsAssignedIn( SemanticModel model, ClassDeclarationSyntax classDeclaration, + HashSet 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; @@ -138,21 +140,22 @@ private static bool IsAssignedIn( private static bool IsAssignedIn( SemanticModel model, ClassDeclarationSyntax classDeclaration, + HashSet 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. @@ -163,12 +166,13 @@ private static bool IsAssignedIn( private static bool IsAssignedIn( SemanticModel model, ClassDeclarationSyntax classDeclaration, + HashSet visitedMethods, SyntaxList statements, string fieldOrPropertyName) { foreach (var statement in statements) { - if (IsAssignedIn(model, classDeclaration, statement, fieldOrPropertyName)) + if (IsAssignedIn(model, classDeclaration, visitedMethods, statement, fieldOrPropertyName)) return true; } @@ -178,6 +182,7 @@ private static bool IsAssignedIn( private static bool IsAssignedIn( SemanticModel model, ClassDeclarationSyntax classDeclaration, + HashSet visitedMethods, InvocationExpressionSyntax invocationExpression, string fieldOrPropertyName) { @@ -190,7 +195,11 @@ 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.Contains(method)) + { + visitedMethods.Add(method); + return IsAssignedIn(model, classDeclaration, visitedMethods, method, fieldOrPropertyName); + } } return false; @@ -199,6 +208,7 @@ private static bool IsAssignedIn( private static bool IsAssignedIn( SemanticModel model, ClassDeclarationSyntax classDeclaration, + HashSet visitedMethods, ExpressionSyntax? expressionStatement, string fieldOrPropertyName) { @@ -245,7 +255,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; } From ab8167dd78b6749dbf310417fa4634c32e1d299b Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Thu, 28 Sep 2023 08:54:28 +0800 Subject: [PATCH 3/3] Code review simplification. --- ...NullableFieldOrPropertyIsUninitializedSuppressor.cs | 3 +-- .../DisposeFieldsAndPropertiesInTearDownAnalyzer.cs | 10 ++-------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs b/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs index e7fcb19b..b5988072 100644 --- a/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs +++ b/src/nunit.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs @@ -195,9 +195,8 @@ private static bool IsAssignedIn( if (method?.Parent == classDeclaration) { // We only get here if the method is in our source code and our class. - if (!visitedMethods.Contains(method)) + if (visitedMethods.Add(method)) { - visitedMethods.Add(method); return IsAssignedIn(model, classDeclaration, visitedMethods, method, fieldOrPropertyName); } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index 11344a03..988433a1 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -550,14 +550,8 @@ public bool IsLocalNotYetVisitedMethodCall( InvocationExpressionSyntax invocationExpression, [NotNullWhen(true)] out IMethodSymbol? calledMethod) { - if (this.IsLocalMethodCall(invocationExpression, out calledMethod) && - !this.visitedMethods.Contains(calledMethod)) - { - this.visitedMethods.Add(calledMethod); - return true; - } - - return false; + return this.IsLocalMethodCall(invocationExpression, out calledMethod) && + this.visitedMethods.Add(calledMethod); } public bool IsDisposalOf(