From 78a2e1293948953dbdf229ea3bce020ffcce3484 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Wed, 29 Nov 2023 15:42:42 +0800 Subject: [PATCH 1/4] Only check for fields/properties that can be assigned to. This removed the need to check all methods for classes that only have read-only constants. --- ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 37 ++++++++++++ ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 58 +++++++++++++++---- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs index 6275c457..534f1ad0 100644 --- a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -879,5 +879,42 @@ public void StaticFieldsAreDisposed(string modifier) RoslynAssert.Valid(analyzer, testCode); } + + [Test] + public void EarlyOutForNonSetableFieldsAndProperties() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public partial class TestFixture + { + public const double ToleranceConstant = 1E-10; + + public static readonly double ToleranceReadOnlyField = 1E-10; + + public static double ToleranceExpressionProperty => 1E-10; + + public static double ToleranceReadOnlyProperty { get; } = 1E-10; + + public static object ToleranceGetOnlyExpressionProperty + { + get => 1E-10; + } + + public static object ToleranceGetOnlyProperty + { + get + { + return 1E-10; + } + } + + [Test] + public void SomeTest() => throw new NotImplementedException(); + }"); + + int earlyOutCount = DisposeFieldsAndPropertiesInTearDownAnalyzer.EarlyOutCount; + RoslynAssert.Valid(analyzer, testCode); + Assert.That(DisposeFieldsAndPropertiesInTearDownAnalyzer.EarlyOutCount, Is.GreaterThan(earlyOutCount)); + } + } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index 14843e26..175de5a5 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -44,6 +44,8 @@ public sealed class DisposeFieldsAndPropertiesInTearDownAnalyzer : DiagnosticAna public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(fieldIsNotDisposedInTearDown); + internal static int EarlyOutCount { get; private set; } + public override void Initialize(AnalysisContext context) { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); @@ -72,14 +74,12 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) if (typeSymbol.IsInstancePerTestCaseFixture(context.Compilation) || !typeSymbol.IsTestFixture(context.Compilation)) { - // CA1001 should picked this up. Assuming it is enabled. + // CA1001 should pick this up. Assuming it is enabled. return; } var fieldDeclarations = classDeclaration.Members - .OfType() - .Select(x => x.Declaration) - .SelectMany(x => x.Variables); + .OfType(); var propertyDeclarations = classDeclaration.Members .OfType() @@ -88,25 +88,49 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) HashSet symbolsWithDisposableInitializers = new(); Dictionary symbols = new(); - foreach (var field in fieldDeclarations) + foreach (var fieldDeclaration in fieldDeclarations) { - symbols.Add(field.Identifier.Text, field); - if (field.Initializer is not null && NeedsDisposal(model, field.Initializer.Value)) + foreach (var field in fieldDeclaration.Declaration.Variables) { - symbolsWithDisposableInitializers.Add(field.Identifier.Text); + if (field.Initializer is not null) + { + if (NeedsDisposal(model, field.Initializer.Value)) + { + symbolsWithDisposableInitializers.Add(field.Identifier.Text); + symbols.Add(field.Identifier.Text, field); + } + else if (CanBeAssignedTo(fieldDeclaration)) + { + symbols.Add(field.Identifier.Text, field); + } + } + else + { + symbols.Add(field.Identifier.Text, field); + } } } foreach (var property in propertyDeclarations) { - symbols.Add(property.Identifier.Text, property); - if (property.Initializer is not null && NeedsDisposal(model, property.Initializer.Value)) - symbolsWithDisposableInitializers.Add(property.Identifier.Text); + if (property.Initializer is not null) + { + if (NeedsDisposal(model, property.Initializer.Value)) + { + symbolsWithDisposableInitializers.Add(property.Identifier.Text); + } + } + + if (CanBeAssignedTo(property)) + { + symbols.Add(property.Identifier.Text, property); + } } if (symbols.Count == 0) { // No fields or properties to consider. + EarlyOutCount++; return; } @@ -150,6 +174,18 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) NUnitFrameworkConstants.NameOfTearDownAttribute, otherMethods, tearDownMethods); } + private static bool CanBeAssignedTo(FieldDeclarationSyntax declaration) + { + return !declaration.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.ConstKeyword) || modifier.IsKind(SyntaxKind.ReadOnlyKeyword)); + } + + private static bool CanBeAssignedTo(PropertyDeclarationSyntax declaration) + { + AccessorListSyntax? accessorList = declaration.AccessorList; + return accessorList is not null && + accessorList.Accessors.Any(accessor => accessor.IsKind(SyntaxKind.SetAccessorDeclaration)); + } + private static bool HasAttribute(IMethodSymbol method, string attributeName) { // Look for attribute not only on the method itself but From a344155c0a46c5024af0bc18ca35ce7536a8f153 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Wed, 29 Nov 2023 15:43:45 +0800 Subject: [PATCH 2/4] Deal with partial NUnit classes. Do not check calls across compilation boundaries. --- ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 39 +++++++++++++++++++ ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 6 +++ 2 files changed, 45 insertions(+) diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs index 534f1ad0..6715ed15 100644 --- a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -916,5 +916,44 @@ public static object ToleranceGetOnlyProperty Assert.That(DisposeFieldsAndPropertiesInTearDownAnalyzer.EarlyOutCount, Is.GreaterThan(earlyOutCount)); } + [Test] + public void DoesNotThrowExceptionsWhenUsingPartialClasses() + { + var testCodePart1 = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public partial class TestFixture + {{ + private const double Tolerance = 1E-10; + + private static IDisposable? Property {{ get; set; }} + + [SetUp] + public void SetUp() + {{ + Property = new DummyDisposable(); + }} + + [TearDown] + public void TearDown() + {{ + Property?.Dispose(); + }} + + private static void SomeAsserts() => throw new InvalidOperationException(); + + {DummyDisposable} + }}"); + + var testCodePart2 = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public partial class TestFixture + { + [Test] + public void SomeTest() + { + SomeAsserts(); + } + }"); + + RoslynAssert.Valid(analyzer, testCodePart1, testCodePart2); + } } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index 175de5a5..7690d3e0 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -575,6 +575,12 @@ public bool IsLocalMethodCall( InvocationExpressionSyntax invocationExpression, [NotNullWhen(true)] out IMethodSymbol? calledMethod) { + if (this.Model.SyntaxTree != invocationExpression.SyntaxTree) + { + calledMethod = null; + return false; + } + calledMethod = this.Model.GetSymbolInfo(invocationExpression).Symbol as IMethodSymbol; return calledMethod is not null && SymbolEqualityComparer.Default.Equals(calledMethod.ContainingType, this.type); From 8d28475d6588f23f51eb35948118b650c029ac1a Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Mon, 4 Dec 2023 10:20:34 +0800 Subject: [PATCH 3/4] Align code for Properties and fields. Take into account that get only properties and readonly fields can be assigned to in a constructor. --- ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 60 ++++++++++++++++++- ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 29 +++++---- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs index 6715ed15..e127d0df 100644 --- a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -225,6 +225,26 @@ public void TearDownMethod() RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); } + [Test] + public void AnalyzeWhenReadOnlyFieldSetInConstructorIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private readonly object? ↓field; + + public TestClass() => field = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + Assert.That(field, Is.Not.Null); + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + [Test] public void FieldConditionallyAssignedInCalledLocalMethod() { @@ -528,7 +548,25 @@ public void TearDownMethod() } [Test] - public void AnalyzeWhenPropertydSetInConstructorIsNotDisposed() + public void AnalyzeWhenReadOnlyPropertyWithInitializerIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected object? Property {{ get; }} = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + Assert.That(Property, Is.Not.Null); + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenPropertySetInConstructorIsNotDisposed() { var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" ↓protected object? Property {{ get; private set; }} @@ -547,6 +585,26 @@ public void TearDownMethod() RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); } + [Test] + public void AnalyzeWhenReadOnlyPropertySetInConstructorIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected object? Property {{ get; }} + + public TestClass() => Property = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + Assert.That(Property, Is.Not.Null); + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + [TestCase("")] [TestCase("OneTime")] public void AnalyzeWhenFieldIsDisposedUsingDisposer(string attributePrefix) diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index 7690d3e0..60fbc4d1 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -85,6 +85,10 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) .OfType() .Where(x => x.AccessorList is not null); + bool hasConstructors = classDeclaration.Members + .OfType() + .Any(); + HashSet symbolsWithDisposableInitializers = new(); Dictionary symbols = new(); @@ -98,13 +102,11 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) { symbolsWithDisposableInitializers.Add(field.Identifier.Text); symbols.Add(field.Identifier.Text, field); - } - else if (CanBeAssignedTo(fieldDeclaration)) - { - symbols.Add(field.Identifier.Text, field); + continue; } } - else + + if (CanBeAssignedTo(fieldDeclaration, hasConstructors)) { symbols.Add(field.Identifier.Text, field); } @@ -118,10 +120,12 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) if (NeedsDisposal(model, property.Initializer.Value)) { symbolsWithDisposableInitializers.Add(property.Identifier.Text); + symbols.Add(property.Identifier.Text, property); + continue; } } - if (CanBeAssignedTo(property)) + if (CanBeAssignedTo(property, hasConstructors)) { symbols.Add(property.Identifier.Text, property); } @@ -174,16 +178,19 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) NUnitFrameworkConstants.NameOfTearDownAttribute, otherMethods, tearDownMethods); } - private static bool CanBeAssignedTo(FieldDeclarationSyntax declaration) + private static bool CanBeAssignedTo(FieldDeclarationSyntax declaration, bool hasConstructors) { - return !declaration.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.ConstKeyword) || modifier.IsKind(SyntaxKind.ReadOnlyKeyword)); + return !declaration.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.ConstKeyword)) && + (!declaration.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.ReadOnlyKeyword)) || hasConstructors); } - private static bool CanBeAssignedTo(PropertyDeclarationSyntax declaration) + private static bool CanBeAssignedTo(PropertyDeclarationSyntax declaration, bool hasConstructors) { AccessorListSyntax? accessorList = declaration.AccessorList; - return accessorList is not null && - accessorList.Accessors.Any(accessor => accessor.IsKind(SyntaxKind.SetAccessorDeclaration)); + if (accessorList is null) + return false; // Expression body property + + return hasConstructors || accessorList.Accessors.Any(accessor => accessor.IsKind(SyntaxKind.SetAccessorDeclaration)); } private static bool HasAttribute(IMethodSymbol method, string attributeName) From 989971453d861341169f8f7452005c5d4f9b143b Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Mon, 4 Dec 2023 11:36:31 +0800 Subject: [PATCH 4/4] Ignore explicit implementations of properties This to cope with cases where a field and an explicit property have the same name. --- ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 38 +++++++++++++++++++ ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 3 ++ 2 files changed, 41 insertions(+) diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs index e127d0df..76ce0981 100644 --- a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -1013,5 +1013,43 @@ public void SomeTest() RoslynAssert.Valid(analyzer, testCodePart1, testCodePart2); } + + [Test] + public void DoesNotThrowExceptionsWhenUsingExplicitProperties() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + [TestFixture] + public abstract class Test : IDataProvider + { + private DataReader DataReader; + + DataReader IDataProvider.DataReader + { + get => DataReader; + set => DataReader = value; + } + + protected Test() => DataReader = new DataReader(this); + + [OneTimeSetUp] + public void SetListener() => throw new NotImplementedException(); + } + + public interface IDataProvider + { + DataReader DataReader + { + get; + set; + } + } + + public sealed class DataReader + { + public DataReader(IDataProvider provider) => throw new NotImplementedException(); + }"); + + RoslynAssert.Valid(analyzer, testCode); + } } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs index 60fbc4d1..068eed30 100644 --- a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -115,6 +115,9 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) foreach (var property in propertyDeclarations) { + if (property.ExplicitInterfaceSpecifier is not null) + continue; + if (property.Initializer is not null) { if (NeedsDisposal(model, property.Initializer.Value))