From 81c26ba1938b369a5ee1c49b590efae9a7d2bcd6 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Wed, 6 Sep 2023 09:11:01 +0800 Subject: [PATCH] DisposableFieldsSuppressor and Analyzer (#576) * Add DisposableFieldsSuppressor and DisposeFieldsInTearDownAnalyzer * Code review changes. * Add support for more cases * await * ConfigureAwait * try * finally * Close is recognized as Dispose method * Enhancements: * Ignore classes that are not a TestFixture * Ignore some types that don't need Disposing, e.g. Task * Only check fields assigned from constructors and methods, not from copy and other expressions. * Add support for properties and constructors/initializers * Add support for conditional Dispose * Add support for field with a TypeParameter type * Rename 'fields' with better name. * Added Parameters class so we don't need to pass in lots of fields. Add support for Release(field) and allow configuring extra release methods. * Changed to use single HashSet per operation. Previously it created several and then merged them. * Code review commit --- .editorconfig | 1 + documentation/NUnit1032.md | 64 ++ documentation/NUnit3001.md | 1 + documentation/NUnit3002.md | 1 + documentation/NUnit3003.md | 7 +- documentation/NUnit3004.md | 68 ++ documentation/index.md | 4 +- src/nunit.analyzers.sln | 2 + .../Constants/NUnitFrameworkConstantsTests.cs | 5 + ...tantiatedInternalClassesSuppressorTests.cs | 2 +- .../DisposableFieldsSuppressorTests.cs | 94 +++ ...ldsAndPropertiesInTearDownAnalyzerTests.cs | 712 ++++++++++++++++++ .../DocumentationTests.cs | 1 + src/nunit.analyzers.tests/TestHelpers.cs | 3 +- .../Constants/AnalyzerIdentifiers.cs | 2 + .../Constants/NUnitFrameworkConstants.cs | 6 + .../NUnit.Analyzers.Suppressions.ruleset | 1 + ...Suppressor.cs => TestFixtureSuppressor.cs} | 19 +- ...seFieldsAndPropertiesInTearDownAnalyzer.cs | 594 +++++++++++++++ ...eFieldsAndPropertiesInTearDownConstants.cs | 9 + .../Extensions/ITypeSymbolExtensions.cs | 17 + 21 files changed, 1603 insertions(+), 10 deletions(-) create mode 100644 documentation/NUnit1032.md create mode 100644 documentation/NUnit3004.md create mode 100644 src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs create mode 100644 src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs rename src/nunit.analyzers/DiagnosticSuppressors/{AvoidUninstantiatedInternalClassesSuppressor.cs => TestFixtureSuppressor.cs} (67%) create mode 100644 src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs create mode 100644 src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownConstants.cs diff --git a/.editorconfig b/.editorconfig index aee4fad4..a493a9cd 100644 --- a/.editorconfig +++ b/.editorconfig @@ -54,6 +54,7 @@ csharp_space_between_square_brackets = false csharp_indent_block_contents = true csharp_indent_braces = false csharp_indent_case_contents = true +csharp_indent_case_contents_when_block = false csharp_indent_switch_labels = true csharp_indent_labels = flush_left diff --git a/documentation/NUnit1032.md b/documentation/NUnit1032.md new file mode 100644 index 00000000..15bb3c7d --- /dev/null +++ b/documentation/NUnit1032.md @@ -0,0 +1,64 @@ +# NUnit1032 + +## An IDisposable field/property should be Disposed in a TearDown method + +| Topic | Value +| :-- | :-- +| Id | NUnit1032 +| Severity | Error +| Enabled | True +| Category | Structure +| Code | [DisposeFieldsAndPropertiesInTearDownAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs) + +## Description + +An IDisposable field/property should be Disposed in a TearDown method. + +## Motivation + +Not Diposing fields/properties can cause memory leaks or failing tests. + +## How to fix violations + +Dispose any fields/properties that are initialized in `SetUp` or `Test` methods in a `TearDown` method. +Fields/Properties that are initialized in `OneTimeSetUp`, or with initializers or in constructors +must be disposed in `OneTimeTearDown`. + + +## Configure severity + +### Via ruleset file + +Configure the severity per project, for more info see [MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022). + +### Via .editorconfig file + +```ini +# NUnit1032: An IDisposable field/property should be Disposed in a TearDown method +dotnet_diagnostic.NUnit1032.severity = chosenSeverity +``` + +where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`. + +### Via #pragma directive + +```csharp +#pragma warning disable NUnit1032 // An IDisposable field/property should be Disposed in a TearDown method +Code violating the rule here +#pragma warning restore NUnit1032 // An IDisposable field/property should be Disposed in a TearDown method +``` + +Or put this at the top of the file to disable all instances. + +```csharp +#pragma warning disable NUnit1032 // An IDisposable field/property should be Disposed in a TearDown method +``` + +### Via attribute `[SuppressMessage]` + +```csharp +[System.Diagnostics.CodeAnalysis.SuppressMessage("Structure", + "NUnit1032:An IDisposable field/property should be Disposed in a TearDown method", + Justification = "Reason...")] +``` + diff --git a/documentation/NUnit3001.md b/documentation/NUnit3001.md index 69049b7d..d1b210df 100644 --- a/documentation/NUnit3001.md +++ b/documentation/NUnit3001.md @@ -99,6 +99,7 @@ To disable the rule for a project, you need to add a + ``` diff --git a/documentation/NUnit3002.md b/documentation/NUnit3002.md index 1435bd26..5f95aa5e 100644 --- a/documentation/NUnit3002.md +++ b/documentation/NUnit3002.md @@ -69,6 +69,7 @@ To disable the rule for a project, you need to add a + ``` diff --git a/documentation/NUnit3003.md b/documentation/NUnit3003.md index fe978a85..0e7fb822 100644 --- a/documentation/NUnit3003.md +++ b/documentation/NUnit3003.md @@ -1,6 +1,6 @@ # NUnit3003 -## Class is a NUnit TestFixture and called by reflection +## Class is an NUnit TestFixture and is instantiated using reflection | Topic | Value | :-- | :-- @@ -8,7 +8,7 @@ | Severity | Info | Enabled | True | Category | Suppressor -| Code | [AvoidUninstantiatedInternalClassesSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressor.cs) +| Code | [TestFixtureSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs) ## Description @@ -38,6 +38,7 @@ To disable the rule for a project, you need to add a + ``` @@ -57,7 +58,7 @@ For more info about rulesets see [MSDN](https://learn.microsoft.com/en-us/visual This is currently not working. Waiting for [Roslyn](https://github.com/dotnet/roslyn/issues/49727) ```ini -# NUnit3003: Class is a NUnit TestFixture and called by reflection +# NUnit3003: Class is an NUnit TestFixture and is instantiated using reflection dotnet_diagnostic.NUnit3003.severity = none ``` diff --git a/documentation/NUnit3004.md b/documentation/NUnit3004.md new file mode 100644 index 00000000..c152a646 --- /dev/null +++ b/documentation/NUnit3004.md @@ -0,0 +1,68 @@ +# NUnit3004 + +## Field should be Disposed in TearDown or OneTimeTearDown method + +| Topic | Value +| :-- | :-- +| Id | NUnit3004 +| Severity | Info +| Enabled | True +| Category | Suppressor +| Code | [TestFixtureSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs) + +## Description + +Field/Property is Disposed in TearDown or OneTimeTearDown method + +## Motivation + +The Roslyn analyzer fires [CA1001](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1001) +for classes that have [`IDisposable`](https://learn.microsoft.com/en-us/dotnet/api/system.idisposable) members, but itself is not `IDisposable`. + +Many NUnit tests initialize fields in tests or a `SetUp` method and then `Dispose` them in the `TearDown` method. + +## How to fix violations + +Ensure that all `IDisposable` fields have a `Dispose` call in the `TearDown` method. + + +## Configure severity + +The rule has no severity, but can be disabled. + +### Via ruleset file + +To disable the rule for a project, you need to add a +[ruleset file](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/NUnit.Analyzers.Suppressions.ruleset) + +```xml + + + + + + + + + +``` + +and add it to the project like: + +```xml + + NUnit.Analyzers.Suppressions.ruleset + +``` + +For more info about rulesets see [MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022). + +### Via .editorconfig file + +This is currently not working. Waiting for [Roslyn](https://github.com/dotnet/roslyn/issues/49727) + +```ini +# NUnit3004: Field should be Disposed in TearDown or OneTimeTearDown method +dotnet_diagnostic.NUnit3004.severity = none +``` + diff --git a/documentation/index.md b/documentation/index.md index a0216c20..f09207af 100644 --- a/documentation/index.md +++ b/documentation/index.md @@ -50,6 +50,7 @@ Rules which enforce structural requirements on the test code. | [NUnit1029](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1029.md) | The number of parameters provided by the TestCaseSource does not match the number of parameters in the Test method | :white_check_mark: | :exclamation: | :x: | | [NUnit1030](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1030.md) | The type of parameter provided by the TestCaseSource does not match the type of the parameter in the Test method | :white_check_mark: | :exclamation: | :x: | | [NUnit1031](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1031.md) | The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: | +| [NUnit1032](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1032.md) | An IDisposable field/property should be Disposed in a TearDown method | :white_check_mark: | :exclamation: | :x: | ### Assertion Rules (NUnit2001 - ) @@ -113,4 +114,5 @@ Rules which suppress compiler errors based on context. Note that these rules are | :-- | :-- | :--: | :--: | :--: | | [NUnit3001](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3001.md) | Expression was checked in an Assert.NotNull, Assert.IsNotNull or Assert.That call | :white_check_mark: | :information_source: | :x: | | [NUnit3002](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3002.md) | Field/Property is initialized in SetUp or OneTimeSetUp method | :white_check_mark: | :information_source: | :x: | -| [NUnit3003](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3003.md) | Class is a NUnit TestFixture and called by reflection | :white_check_mark: | :information_source: | :x: | +| [NUnit3003](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3003.md) | Class is an NUnit TestFixture and is instantiated using reflection | :white_check_mark: | :information_source: | :x: | +| [NUnit3004](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3004.md) | Field should be Disposed in TearDown or OneTimeTearDown method | :white_check_mark: | :information_source: | :x: | diff --git a/src/nunit.analyzers.sln b/src/nunit.analyzers.sln index 274995f3..e1625c04 100644 --- a/src/nunit.analyzers.sln +++ b/src/nunit.analyzers.sln @@ -43,6 +43,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit1029.md = ..\documentation\NUnit1029.md ..\documentation\NUnit1030.md = ..\documentation\NUnit1030.md ..\documentation\NUnit1031.md = ..\documentation\NUnit1031.md + ..\documentation\NUnit1032.md = ..\documentation\NUnit1032.md ..\documentation\NUnit2001.md = ..\documentation\NUnit2001.md ..\documentation\NUnit2002.md = ..\documentation\NUnit2002.md ..\documentation\NUnit2003.md = ..\documentation\NUnit2003.md @@ -93,6 +94,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit3001.md = ..\documentation\NUnit3001.md ..\documentation\NUnit3002.md = ..\documentation\NUnit3002.md ..\documentation\NUnit3003.md = ..\documentation\NUnit3003.md + ..\documentation\NUnit3004.md = ..\documentation\NUnit3004.md ..\README.md = ..\README.md EndProjectSection EndProject diff --git a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs index 45dcef86..60279f5a 100644 --- a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs +++ b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs @@ -120,6 +120,11 @@ public sealed class NUnitFrameworkConstantsTests (nameof(NUnitFrameworkConstants.NameOfValueSourceAttribute), nameof(ValueSourceAttribute)), (nameof(NUnitFrameworkConstants.NameOfValuesAttribute), nameof(ValuesAttribute)), + (nameof(NUnitFrameworkConstants.NameOfOneTimeSetUpAttribute), nameof(OneTimeSetUpAttribute)), + (nameof(NUnitFrameworkConstants.NameOfOneTimeTearDownAttribute), nameof(OneTimeTearDownAttribute)), + (nameof(NUnitFrameworkConstants.NameOfSetUpAttribute), nameof(SetUpAttribute)), + (nameof(NUnitFrameworkConstants.NameOfTearDownAttribute), nameof(TearDownAttribute)), + (nameof(NUnitFrameworkConstants.NameOfExpectedResult), nameof(TestAttribute.ExpectedResult)), (nameof(NUnitFrameworkConstants.NameOfConstraintExpressionAnd), nameof(EqualConstraint.And)), diff --git a/src/nunit.analyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs b/src/nunit.analyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs index d2bd946e..54cd00af 100644 --- a/src/nunit.analyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs +++ b/src/nunit.analyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs @@ -51,7 +51,7 @@ public void TestSubtract() } "; - private static readonly DiagnosticSuppressor suppressor = new AvoidUninstantiatedInternalClassesSuppressor(); + private static readonly DiagnosticSuppressor suppressor = new TestFixtureSuppressor(); private DiagnosticAnalyzer analyzer; [OneTimeSetUp] diff --git a/src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs b/src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs new file mode 100644 index 00000000..93c68ea0 --- /dev/null +++ b/src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs @@ -0,0 +1,94 @@ +using System; +using System.IO; +using System.Reflection; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; +using NUnit.Analyzers.DiagnosticSuppressors; +using NUnit.Framework; + +namespace NUnit.Analyzers.Tests.DiagnosticSuppressors +{ + public class DisposableFieldsSuppressorTests + { + private static readonly DiagnosticSuppressor suppressor = new TestFixtureSuppressor(); + private DiagnosticAnalyzer analyzer; + + [OneTimeSetUp] + public void OneTimeSetUp() + { + // Find the NetAnalyzers assembly (note version should match the one referenced) + string netAnalyzersPath = Path.Combine(PathHelper.GetNuGetPackageDirectory(), + "microsoft.codeanalysis.netanalyzers/7.0.4/analyzers/dotnet/cs/Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dll"); + Assembly netAnalyzerAssembly = Assembly.LoadFrom(netAnalyzersPath); + Type analyzerType = netAnalyzerAssembly.GetType("Microsoft.CodeQuality.CSharp.Analyzers.ApiDesignGuidelines.CSharpTypesThatOwnDisposableFieldsShouldBeDisposableAnalyzer", true)!; + this.analyzer = (DiagnosticAnalyzer)Activator.CreateInstance(analyzerType)!; + + this.analyzer = new DefaultEnabledAnalyzer(this.analyzer); + } + + [Test] + public async Task FieldNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + private IDisposable? field; + + [Test] + public void Test() + { + field = new DummyDisposable(); + Assert.That(field, Is.Not.Null); + } + + private sealed class DummyDisposable : IDisposable + { + public void Dispose() {} + } + "); + + // This rule doesn't care. Actual checking is done in DisposeFieldsInTearDownAnalyzer + await TestHelpers.Suppressed(this.analyzer, suppressor, testCode).ConfigureAwait(true); + } + + [TestCase(NUnitFrameworkConstants.NameOfTearDownAttribute, "")] + [TestCase(NUnitFrameworkConstants.NameOfOneTimeTearDownAttribute, "this.")] + public async Task FieldDisposed(string attribute, string prefix) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$" + private IDisposable? field1; + private IDisposable? field2; + + [{attribute}] + public void CleanUp() + {{ + {prefix}field1?.Dispose(); + if ({prefix}field2 is not null) + {{ + {prefix}field2.Dispose(); + }} + }} + + [Test] + public void Test1() + {{ + {prefix}field1 = new DummyDisposable(); + Assert.That({prefix}field1, Is.Not.Null); + }} + + [Test] + public void Test2() + {{ + {prefix}field2 = new DummyDisposable(); + Assert.That({prefix}field2, Is.Not.Null); + }} + + private sealed class DummyDisposable : IDisposable + {{ + public void Dispose() {{}} + }} + "); + + await TestHelpers.Suppressed(this.analyzer, suppressor, testCode).ConfigureAwait(true); + } + } +} diff --git a/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs new file mode 100644 index 00000000..78da5790 --- /dev/null +++ b/src/nunit.analyzers.tests/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs @@ -0,0 +1,712 @@ +using Gu.Roslyn.Asserts; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; +using NUnit.Analyzers.DisposeFieldsInTearDown; +using NUnit.Framework; + +namespace NUnit.Analyzers.Tests.DisposeFieldsInTearDown +{ + public sealed class DisposeFieldsAndPropertiesInTearDownAnalyzerTests + { + private const string DummyDisposable = @" + sealed class DummyDisposable : IDisposable + { + public void Dispose() {} + }"; + + // CA2000 allows transfer of ownership using ICollection.Add + private const string Disposer = @" + private sealed class Disposer : IDisposable + { + public void Dispose() {} + + public T? Add(T? resource) => resource; + } + "; + + private static readonly DiagnosticAnalyzer analyzer = new DisposeFieldsAndPropertiesInTearDownAnalyzer(); + private static readonly ExpectedDiagnostic expectedDiagnostic = + ExpectedDiagnostic.Create(AnalyzerIdentifiers.FieldIsNotDisposedInTearDown); + + [Test] + public void AnalyzeWhenFieldIsDisposedInCorrectMethod( + [Values("", "OneTime")] string attributePrefix, + [Values("", "this.")] string prefix) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable? field; + + [{attributePrefix}SetUp] + public void SetUpMethod() + {{ + {prefix}field = new DummyDisposable(); + }} + + [{attributePrefix}TearDown] + public void TearDownMethod() + {{ + {prefix}field?.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldIsConditionallyDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private object field = new DummyDisposable(); + + [OneTimeTearDown] + public void TearDownMethod() + {{ + if (field is IDisposable disposable) + disposable.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldWithInitializerIsDisposedInOneTimeTearDownMethod() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable field = new DummyDisposable(); + + [OneTimeTearDown] + public void TearDownMethod() + {{ + field.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldSetInConstructorIsDisposedInOneTimeTearDownMethod() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable field; + + public TestClass() + {{ + field = new DummyDisposable(); + }} + + [OneTimeTearDown] + public void TearDownMethod() + {{ + field.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [TestCase("", "OneTime")] + [TestCase("OneTime", "")] + public void AnalyzeWhenFieldIsDisposedInWrongMethod(string attributePrefix1, string attributePrefix2) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable? ↓field; + + [{attributePrefix1}SetUp] + public void SetUpMethod() + {{ + field = new DummyDisposable(); + }} + + [{attributePrefix2}TearDown] + public void TearDownMethod() + {{ + field?.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [TestCase("SetUp")] + [TestCase("Test")] + public void AnalyzeWhenFieldIsNotDisposed(string attribute) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private object? ↓field; + + [{attribute}] + public void SomeMethod() + {{ + field = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + field = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [TestCase("SetUp")] + [TestCase("Test")] + public void AnalyzeWhenFieldTypeParameterIsNotDisposed(string attribute) + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public class TestClass + where T : IDisposable, new() + {{ + private T? ↓field; + + [{attribute}] + public void SomeMethod() + {{ + field = new T(); + }} + + {DummyDisposable} + }} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenFieldWithInitializerIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private object? ↓field = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + field = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenFieldSetInConstructorIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private object? ↓field; + + public TestClass() => field = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + field = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenPropertyBackingFieldIsDisposed( + [Values("field", "Property")] string fieldOrProperty) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable? field; + + private IDisposable? Property + {{ + get => field; + set => this.field = value; + }} + + [SetUp] + public void SetUpMethod() + {{ + {fieldOrProperty} = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + {fieldOrProperty}?.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldIsDisposedInSpecialExtensionMethod() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public class TestClass + {{ + private object? field; + + [SetUp] + public void SetUpMethod() + {{ + field = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + field.DisposeIfDisposable(); + }} + + {DummyDisposable} + }} + + public static class DisposableExtensions + {{ + public static void DisposeIfDisposable(this T instance) + {{ + if (instance is IDisposable disposable) + disposable.Dispose(); + }} + }} + "); + + const string AnalyzerConfig = "dotnet_diagnostic.NUnit1032.additional_dispose_methods = DisposeIfDisposable"; + Settings settings = Settings.Default.WithAnalyzerConfig(AnalyzerConfig); + RoslynAssert.Valid(analyzer, testCode, settings); + } + + [Test] + public void AnalyzeWhenFieldIsDisposedInSpecialMethodWithParameter() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private object? field; + + [SetUp] + public void SetUpMethod() + {{ + field = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + Release(field); + }} + + private void Release(T instance) + {{ + if (instance is IDisposable disposable) + disposable.Dispose(); + }} + + {DummyDisposable} + "); + + const string AnalyzerConfig = "dotnet_diagnostic.NUnit1032.additional_dispose_methods = Release"; + Settings settings = Settings.Default.WithAnalyzerConfig(AnalyzerConfig); + RoslynAssert.Valid(analyzer, testCode, settings); + } + + [Test] + public void AnalyzeWhenFieldIsDisposedUsingFactoryWithParameter() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public class TestClass + {{ + private IFactory _factory; + private DummyDisposable? _field; + + public TestClass(IFactory factory) => _factory = factory; + + [SetUp] + public void SetUpMethod() => _field = _factory.Create(); + + [TearDown] + public void TearDownMethod() => _factory.Release(_field!); + }} + + public {DummyDisposable} + + public interface IFactory + {{ + T Create(); + void Release(T instance); + }} + "); + + const string AnalyzerConfig = "dotnet_diagnostic.NUnit1032.additional_dispose_methods = Release"; + Settings settings = Settings.Default.WithAnalyzerConfig(AnalyzerConfig); + RoslynAssert.Valid(analyzer, testCode, settings); + } + + [Test] + public void AnalyzeWhenPropertyIsDisposedInCorrectMethod( + [Values("", "OneTime")] string attributePrefix, + [Values("", "this.")] string prefix) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + protected IDisposable? Property {{ get; private set; }} + + [{attributePrefix}SetUp] + public void SetUpMethod() + {{ + {prefix}Property = new DummyDisposable(); + }} + + [{attributePrefix}TearDown] + public void TearDownMethod() + {{ + {prefix}Property?.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenPropertyWithInitializerIsDisposedInOneTimeTearDownMethod() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable Property {{ get; }} = new DummyDisposable(); + + [OneTimeTearDown] + public void TearDownMethod() + {{ + Property.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenPropertySetInConstructorIsDisposedInOneTimeTearDownMethod() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable Property {{ get; }} + + public TestClass() + {{ + Property = new DummyDisposable(); + }} + + [OneTimeTearDown] + public void TearDownMethod() + {{ + Property.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [TestCase("", "OneTime")] + [TestCase("OneTime", "")] + public void AnalyzeWhenPropertyIsDisposedInWrongMethod(string attributePrefix1, string attributePrefix2) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected IDisposable? Property {{ get; private set; }} + + [{attributePrefix1}SetUp] + public void SetUpMethod() + {{ + Property = new DummyDisposable(); + }} + + [{attributePrefix2}TearDown] + public void TearDownMethod() + {{ + Property?.Dispose(); + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [TestCase("SetUp")] + [TestCase("Test")] + public void AnalyzeWhenPropertyIsNotDisposed(string attribute) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected object? Property {{ get; private set; }} + + [{attribute}] + public void SomeMethod() + {{ + Property = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + Property = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenPropertyWithInitializerIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected object? Property {{ get; private set; }} = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + Property = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenPropertydSetInConstructorIsNotDisposed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + ↓protected object? Property {{ get; private set; }} + + public TestClass() => Property = new DummyDisposable(); + + [TearDown] + public void TearDownMethod() + {{ + Property = null; + }} + + {DummyDisposable} + "); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [TestCase("")] + [TestCase("OneTime")] + public void AnalyzeWhenFieldIsDisposedUsingDisposer(string attributePrefix) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private Disposer? disposer; + private object? field; + + [{attributePrefix}SetUp] + public void SetUpMethod() + {{ + disposer = new Disposer(); + field = disposer.Add(new DummyDisposable()); + }} + + [{attributePrefix}TearDown] + public void TearDownMethod() + {{ + disposer?.Dispose(); + }} + + {DummyDisposable} + + {Disposer} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldIsDisposedInTryOrFinallyStatement() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable? field1; + private IDisposable? field2; + + [SetUp] + public void SetUpMethod() + {{ + field1 = new DummyDisposable(); + field2 = new DummyDisposable(); + }} + + [TearDown] + public void TearDownMethod() + {{ + try + {{ + field1?.Dispose(); + }} + finally + {{ + field2?.Dispose(); + }} + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [TestCase("Dispose")] + [TestCase("Close")] + public void AnalyzeWhenFieldIsDisposedUsing(string method) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private DummyWriter? field; + + [SetUp] + public void SetUpMethod() + {{ + field = new DummyWriter(); + }} + + [TearDown] + public void TearDownMethod() + {{ + field?.{method}(); + }} + + private sealed class DummyWriter : IDisposable + {{ + public void Dispose() {{}} + + public void Close() => Dispose(); + }}"); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenFieldIsCopied() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + private NestedDisposable? field1; + private IDisposable? field2; + + [SetUp] + public void SetUpMethod() + { + field1 = new NestedDisposable(); + field2 = field1.Member; + } + + [TearDown] + public void TearDownMethod() + { + field1?.Dispose(); + } + + private sealed class NestedDisposable : IDisposable + { + public IDisposable Member { get; } = new DummyDisposable(); + + public void Dispose() => Member.Dispose(); + } + " + DummyDisposable); + + RoslynAssert.Valid(analyzer, testCode); + } + + [TestCase("new Task(() => { })")] + [TestCase("new System.IO.MemoryStream(128)")] + [TestCase("new System.IO.StringReader(\"NUnit.Analyzers\")")] + public void AnalyzeWhenFieldDoesNotNeedDisposing(string expressionSyntax) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private IDisposable? field; + + [SetUp] + public void SetUpMethod() + {{ + field = {expressionSyntax}; + }} + + {DummyDisposable} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + + [Test] + public void AnalyzeWhenClassIsNotAnNUnitTestFixture() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public sealed class NotATestClass + {{ + private IDisposable? field; + + public void SomeMethod() + {{ + field = new DummyDisposable(); + }} + + {DummyDisposable} + }} + "); + + RoslynAssert.Valid(analyzer, testCode); + } + +#if !NETFRAMEWORK + [Test] + public void AnalyzeWhenFieldIsAsyncDisposable( + [Values("DisposeAsync", "CloseAsync")] string method, + [Values("", "this.")] string prefix, + [Values("", ".ConfigureAwait(false)")] string configureAwait) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + private DummyAsyncWriter? field; + + [SetUp] + public void SetUpMethod() + {{ + field = new DummyAsyncWriter(); + }} + + [TearDown] + public async Task TearDownMethod() + {{ + if (field is not null) + await {prefix}field.{method}(){configureAwait}; + }} + + private sealed class DummyAsyncWriter : IAsyncDisposable + {{ + public ValueTask DisposeAsync() => default(ValueTask); + + public Task CloseAsync() => DisposeAsync().AsTask(); + }}"); + + RoslynAssert.Valid(analyzer, testCode); + } +#endif + } +} diff --git a/src/nunit.analyzers.tests/DocumentationTests.cs b/src/nunit.analyzers.tests/DocumentationTests.cs index e4ea0e87..b473b778 100644 --- a/src/nunit.analyzers.tests/DocumentationTests.cs +++ b/src/nunit.analyzers.tests/DocumentationTests.cs @@ -514,6 +514,7 @@ [ruleset file](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.an + ``` diff --git a/src/nunit.analyzers.tests/TestHelpers.cs b/src/nunit.analyzers.tests/TestHelpers.cs index 652536e7..13b7bf31 100644 --- a/src/nunit.analyzers.tests/TestHelpers.cs +++ b/src/nunit.analyzers.tests/TestHelpers.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Immutable; +using System.Linq; using System.Threading.Tasks; using Gu.Roslyn.Asserts; using Microsoft.CodeAnalysis; @@ -28,7 +29,7 @@ internal static Compilation CreateCompilation(string? code = null, Settings? set internal static async Task SuppressedOrNot(DiagnosticAnalyzer analyzer, DiagnosticSuppressor suppressor, string code, bool isSuppressed, Settings? settings = null) { string id = analyzer.SupportedDiagnostics[0].Id; - Assert.That(suppressor.SupportedSuppressions[0].SuppressedDiagnosticId, Is.EqualTo(id)); + Assert.That(suppressor.SupportedSuppressions.Select(x => x.SuppressedDiagnosticId), Does.Contain(id)); settings ??= Settings.Default; settings = settings.WithCompilationOptions(Settings.Default.CompilationOptions.WithWarningOrError(analyzer.SupportedDiagnostics)); diff --git a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs index d4daff02..91787eb6 100644 --- a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs +++ b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs @@ -35,6 +35,7 @@ internal static class AnalyzerIdentifiers internal const string TestCaseSourceMismatchInNumberOfTestMethodParameters = "NUnit1029"; internal const string TestCaseSourceMismatchWithTestMethodParameterType = "NUnit1030"; internal const string ValuesParameterTypeMismatchUsage = "NUnit1031"; + internal const string FieldIsNotDisposedInTearDown = "NUnit1032"; #endregion Structure @@ -95,6 +96,7 @@ internal static class AnalyzerIdentifiers internal const string DereferencePossibleNullReference = "NUnit3001"; internal const string NonNullableFieldOrPropertyIsUninitialized = "NUnit3002"; internal const string AvoidUninstantiatedInternalClasses = "NUnit3003"; + internal const string TypesThatOwnDisposableFieldsShouldBeDisposable = "NUnit3004"; #endregion } diff --git a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs index 6fab5f2e..6be82ff4 100644 --- a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs +++ b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs @@ -107,6 +107,7 @@ public static class NUnitFrameworkConstants public const string FullNameOfTypeISimpleTestBuilder = "NUnit.Framework.Interfaces.ISimpleTestBuilder"; public const string FullNameOfTypeValuesAttribute = "NUnit.Framework.ValuesAttribute"; public const string FullNameOfTypeValueSourceAttribute = "NUnit.Framework.ValueSourceAttribute"; + public const string FullNameOfTypeIParameterDataSource = "NUnit.Framework.Interfaces.IParameterDataSource"; public const string FullNameOfTypeTestCaseData = "NUnit.Framework.TestCaseData"; public const string FullNameOfTypeTestCaseParameters = "NUnit.Framework.Internal.TestCaseParameters"; @@ -142,6 +143,11 @@ public static class NUnitFrameworkConstants public const string NameOfValuesAttribute = "ValuesAttribute"; public const string NameOfValueSourceAttribute = "ValueSourceAttribute"; + public const string NameOfOneTimeSetUpAttribute = "OneTimeSetUpAttribute"; + public const string NameOfOneTimeTearDownAttribute = "OneTimeTearDownAttribute"; + public const string NameOfSetUpAttribute = "SetUpAttribute"; + public const string NameOfTearDownAttribute = "TearDownAttribute"; + public const string NameOfExpectedResult = "ExpectedResult"; public const string NameOfActualParameter = "actual"; diff --git a/src/nunit.analyzers/DiagnosticSuppressors/NUnit.Analyzers.Suppressions.ruleset b/src/nunit.analyzers/DiagnosticSuppressors/NUnit.Analyzers.Suppressions.ruleset index 5f017b78..5d58fe34 100644 --- a/src/nunit.analyzers/DiagnosticSuppressors/NUnit.Analyzers.Suppressions.ruleset +++ b/src/nunit.analyzers/DiagnosticSuppressors/NUnit.Analyzers.Suppressions.ruleset @@ -4,5 +4,6 @@ + diff --git a/src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressor.cs b/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs similarity index 67% rename from src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressor.cs rename to src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs index 48fe9f86..05a44ee1 100644 --- a/src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressor.cs +++ b/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs @@ -11,15 +11,20 @@ namespace NUnit.Analyzers.DiagnosticSuppressors { [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class AvoidUninstantiatedInternalClassesSuppressor : DiagnosticSuppressor + public sealed class TestFixtureSuppressor : DiagnosticSuppressor { internal static readonly SuppressionDescriptor AvoidUninstantiatedInternalTestFixtureClasses = new( id: AnalyzerIdentifiers.AvoidUninstantiatedInternalClasses, suppressedDiagnosticId: "CA1812", - justification: "Class is a NUnit TestFixture and called by reflection"); + justification: "Class is an NUnit TestFixture and is instantiated using reflection"); + + internal static readonly SuppressionDescriptor TypesThatOwnDisposableFieldsShouldHaveATearDown = new( + id: AnalyzerIdentifiers.TypesThatOwnDisposableFieldsShouldBeDisposable, + suppressedDiagnosticId: "CA1001", + justification: "Field should be Disposed in TearDown or OneTimeTearDown method"); public override ImmutableArray SupportedSuppressions { get; } = - ImmutableArray.Create(AvoidUninstantiatedInternalTestFixtureClasses); + ImmutableArray.Create(AvoidUninstantiatedInternalTestFixtureClasses, TypesThatOwnDisposableFieldsShouldHaveATearDown); public override void ReportSuppressions(SuppressionAnalysisContext context) { @@ -47,7 +52,13 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) if (typeSymbol is not null && typeSymbol.GetMembers().OfType().Any(m => m.IsTestRelatedMethod(context.Compilation))) { - context.ReportSuppression(Suppression.Create(AvoidUninstantiatedInternalTestFixtureClasses, diagnostic)); + foreach (var suppression in this.SupportedSuppressions) + { + if (diagnostic.Descriptor.Id == suppression.SuppressedDiagnosticId) + { + context.ReportSuppression(Suppression.Create(suppression, diagnostic)); + } + } } } } diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs new file mode 100644 index 00000000..59ffe7c0 --- /dev/null +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs @@ -0,0 +1,594 @@ +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; +using NUnit.Analyzers.Extensions; + +namespace NUnit.Analyzers.DisposeFieldsInTearDown +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class DisposeFieldsAndPropertiesInTearDownAnalyzer : DiagnosticAnalyzer + { +#if NETSTANDARD2_0_OR_GREATER + private static readonly char[] AdditionalDisposalMethodsSeparators = { ',', ';', ' ' }; +#endif + + // Methods that are considered to be Dispoing an instance. + private static readonly ImmutableHashSet StandardDisposeMethods = ImmutableHashSet.Create( + "Dispose", + "DisposeAsync", + "Close", + "CloseAsync"); + + // Types that even though they are IDisposable, don't need to be Disposed. + private static readonly ImmutableHashSet DisposableTypeNotRequiringToBeDisposed = ImmutableHashSet.Create( + "System.Threading.Tasks.Task", + "System.IO.MemoryStream", + "System.IO.StringReader"); + + private static readonly DiagnosticDescriptor fieldIsNotDisposedInTearDown = DiagnosticDescriptorCreator.Create( + id: AnalyzerIdentifiers.FieldIsNotDisposedInTearDown, + title: DisposeFieldsAndPropertiesInTearDownConstants.FieldOrPropertyIsNotDisposedInTearDownTitle, + messageFormat: DisposeFieldsAndPropertiesInTearDownConstants.FieldOrPropertyIsNotDisposedInTearDownMessageFormat, + category: Categories.Structure, + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: DisposeFieldsAndPropertiesInTearDownConstants.FieldOrPropertyIsNotDisposedInTearDownDescription); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(fieldIsNotDisposedInTearDown); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeDisposableFields, SyntaxKind.ClassDeclaration); + } + + private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context) + { + var classDeclaration = (ClassDeclarationSyntax)context.Node; + + SemanticModel? model = context.SemanticModel; + + var typeSymbol = model.GetDeclaredSymbol(classDeclaration, context.CancellationToken); + if (typeSymbol is null) + { + return; + } + + if (typeSymbol.IsDisposable()) + { + // If the type is Disposable, the CA2000 Analyzer will conflict, so bail out. + return; + } + + if (!typeSymbol.GetMembers().OfType().Any(m => m.IsTestRelatedMethod(context.Compilation))) + { + // Not a TestFixture, CA1812 should have picked this up. + return; + } + + var fieldDeclarations = classDeclaration.Members + .OfType() + .Select(x => x.Declaration) + .SelectMany(x => x.Variables); + + var propertyDeclarations = classDeclaration.Members + .OfType() + .Where(x => x.AccessorList is not null); + + HashSet symbolsWithDisposableInitializers = new(); + + Dictionary symbols = new(); + foreach (var field in fieldDeclarations) + { + symbols.Add(field.Identifier.Text, field); + if (field.Initializer is not null && NeedsDisposal(model, field.Initializer.Value)) + { + symbolsWithDisposableInitializers.Add(field.Identifier.Text); + } + } + + 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 (symbols.Count == 0) + { + // No fields or properties to consider. + return; + } + + ImmutableHashSet disposeMethods = StandardDisposeMethods; + +#if NETSTANDARD2_0_OR_GREATER + // Are there any additional methods configured that are considers Dispose Methods + // e.g. DisposeIfDisposeable or Release + AnalyzerConfigOptions options = context.Options.AnalyzerConfigOptionsProvider.GetOptions(classDeclaration.SyntaxTree); + if (options.TryGetValue("dotnet_diagnostic.NUnit1032.additional_dispose_methods", out string? value)) + { + disposeMethods = disposeMethods.Union(value.Split(AdditionalDisposalMethodsSeparators, StringSplitOptions.RemoveEmptyEntries)); + } +#endif + + HashSet symbolNames = new(symbols.Keys); + + Parameters parameters = new(model, typeSymbol, disposeMethods, symbolNames); + + ImmutableArray members = typeSymbol.GetMembers(); + var methods = members.OfType().Where(m => !m.IsStatic).ToArray(); + var oneTimeTearDownMethods = methods.Where(m => HasAttribute(m, NUnitFrameworkConstants.NameOfOneTimeTearDownAttribute)).ToImmutableHashSet(SymbolEqualityComparer.Default); + var oneTimeSetUpMethods = methods.Where(m => m.MethodKind == MethodKind.Constructor || HasAttribute(m, NUnitFrameworkConstants.NameOfOneTimeSetUpAttribute)).ToImmutableHashSet(SymbolEqualityComparer.Default); + var setUpMethods = methods.Where(m => HasAttribute(m, NUnitFrameworkConstants.NameOfSetUpAttribute)).ToImmutableHashSet(SymbolEqualityComparer.Default); + var tearDownMethods = methods.Where(m => HasAttribute(m, NUnitFrameworkConstants.NameOfTearDownAttribute)).ToImmutableHashSet(SymbolEqualityComparer.Default); + + var setUpAndTearDownMethods = oneTimeSetUpMethods.Union(oneTimeTearDownMethods).Union(setUpMethods).Union(tearDownMethods); + var otherMethods = methods.Where(m => m.DeclaredAccessibility != Accessibility.Private && !setUpAndTearDownMethods.Contains(m)); + + // Fields assigned in a OneTimeSetUp method must be disposed in a OneTimeTearDown method + AnalyzeAssignedButNotDisposed(context, symbols, parameters, + NUnitFrameworkConstants.NameOfOneTimeTearDownAttribute, oneTimeSetUpMethods, oneTimeTearDownMethods, symbolsWithDisposableInitializers); + + // Fields assigned in a SetUp method must be disposed in a TearDown method + AnalyzeAssignedButNotDisposed(context, symbols, parameters, + NUnitFrameworkConstants.NameOfTearDownAttribute, setUpMethods, tearDownMethods); + + // Fields assignd in any method, must be (conditionally) disposed in TearDown method. + // If the field is disposed in the method itself, why is it a field? + AnalyzeAssignedButNotDisposed(context, symbols, parameters, + NUnitFrameworkConstants.NameOfTearDownAttribute, otherMethods, tearDownMethods); + } + + private static bool HasAttribute(IMethodSymbol method, string attributeName) + { + // Look for attribute not only on the method itself but + // also on the base method in case this is an override. + IEnumerable attributes = Enumerable.Empty(); + for (IMethodSymbol? declaredMethod = method; + declaredMethod is not null; + declaredMethod = declaredMethod.OverriddenMethod) + { + attributes = attributes.Concat(declaredMethod.GetAttributes()); + } + + return attributes.Any(x => x.AttributeClass?.Name == attributeName); + } + + private static void AnalyzeAssignedButNotDisposed( + SyntaxNodeAnalysisContext context, + Dictionary symbols, + Parameters parameters, + string where, + IEnumerable setUpMethods, + IEnumerable tearDownMethods, + HashSet? assignedWithInitializers = null) + { + HashSet assignedInSetUpMethods = assignedWithInitializers ?? new(); + HashSet disposedInTearDownMethods = new(); + + AssignedIn(parameters, assignedInSetUpMethods, setUpMethods); + DisposedIn(parameters, disposedInTearDownMethods, tearDownMethods); + + assignedInSetUpMethods.ExceptWith(disposedInTearDownMethods); + + foreach (var assignedButNotDisposed in assignedInSetUpMethods) + { + SyntaxNode syntaxNode = symbols[assignedButNotDisposed]; + + context.ReportDiagnostic(Diagnostic.Create( + fieldIsNotDisposedInTearDown, + syntaxNode.GetLocation(), + syntaxNode is PropertyDeclarationSyntax ? "property" : "field", + assignedButNotDisposed, + where)); + } + } + + #region AssignedIn + + private static void AssignedIn(Parameters parameters, HashSet assignments, IEnumerable methods) + { + foreach (var method in methods) + { + AssignedIn(parameters, assignments, method); + } + } + + private static void AssignedIn(Parameters parameters, HashSet assignments, IMethodSymbol symbol) + { + BaseMethodDeclarationSyntax? method = + symbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as BaseMethodDeclarationSyntax; + + if (method is not null) + AssignedIn(parameters, assignments, method); + } + + private static void AssignedIn(Parameters parameters, HashSet assignments, BaseMethodDeclarationSyntax method) + { + if (method.ExpressionBody is not null) + { + AssignedIn(parameters, assignments, method.ExpressionBody.Expression); + } + else if (method.Body is not null) + { + AssignedIn(parameters, assignments, method.Body); + } + } + + private static void AssignedIn(Parameters parameters, HashSet assignments, ExpressionSyntax expression) + { + if (expression is AssignmentExpressionSyntax assignmentExpression) + { + // We only deal with simple assignments, not tuple or deconstruct + string? name = GetIdentifier(assignmentExpression.Left); + if (name is not null && parameters.HasSymbolFor(name)) + { + if (NeedsDisposal(parameters.Model, assignmentExpression.Right)) + { + assignments.Add(name); + } + } + } + else if (expression is InvocationExpressionSyntax invocationExpression) + { + string? method = GetIdentifier(invocationExpression.Expression); + if (method is not null) + { + if (parameters.IsLocalMethodCall(invocationExpression, out IMethodSymbol? calledMethod)) + { + // We are calling a local method on our class, keep looking for assignments. + AssignedIn(parameters, assignments, calledMethod); + } + } + } + } + + private static void AssignedIn(Parameters parameters, HashSet assignments, StatementSyntax statement) + { + switch (statement) + { + case ExpressionStatementSyntax expressionStatement: + AssignedIn(parameters, assignments, expressionStatement.Expression); + break; + + case IfStatementSyntax ifStatement: + // We don't care about the condition + AssignedIn(parameters, assignments, ifStatement.Statement); + if (ifStatement.Else is not null) + AssignedIn(parameters, assignments, ifStatement.Else.Statement); + break; + + case BlockSyntax block: + AssignedIn(parameters, assignments, block.Statements); + break; + + case SwitchStatementSyntax switchStatement: + foreach (var caseStatements in switchStatement.Sections.Select(x => x.Statements)) + { + AssignedIn(parameters, assignments, caseStatements); + } + + break; + + case TryStatementSyntax tryStatement: + AssignedIn(parameters, assignments, tryStatement.Block); + if (tryStatement.Finally is not null) + AssignedIn(parameters, assignments, tryStatement.Finally.Block); + break; + + default: + // Anything assigned in a loop is bad as it overrides previous assignments. + break; + } + } + + private static void AssignedIn(Parameters parameters, HashSet assignments, SyntaxList statements) + { + foreach (var statement in statements) + { + AssignedIn(parameters, assignments, statement); + } + } + + private static bool NeedsDisposal(SemanticModel model, ExpressionSyntax expression) + { + if (IsPossibleDisposableCreation(expression)) + { + ITypeSymbol? instanceType = model.GetTypeInfo(expression).Type; + return instanceType is not null && + instanceType.IsDisposable() && + !IsDisposableTypeNotRequiringToBeDisposed(instanceType); + } + + return false; + } + + private static bool IsPossibleDisposableCreation(ExpressionSyntax expression) + { + if (expression is ObjectCreationExpressionSyntax) + return true; + + if (expression is InvocationExpressionSyntax invocationExpression) + { + // Make one exemption, if the value is returned from a 'xxx.Add()' call. + // It is then assumed that owner ship is transferred to that 'collection'. + // This matches the (undocumented) CA2000 behaviour. + // Although we don't actually check if the class implements ICollection. + // https://github.com/dotnet/roslyn-analyzers/blob/main/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs#L465-L499 + return invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccessExpression || + !memberAccessExpression.Name.Identifier.Text.StartsWith("Add", StringComparison.Ordinal); + } + + return false; + } + + private static bool IsDisposableTypeNotRequiringToBeDisposed(ITypeSymbol typeSymbol) + { + return DisposableTypeNotRequiringToBeDisposed.Contains(typeSymbol.GetFullMetadataName()); + } + + #endregion + + #region DisposedIn + + private static void DisposedIn(Parameters parameters, HashSet disposals, IEnumerable methods) + { + foreach (var method in methods) + { + DisposedIn(parameters, disposals, method); + } + } + + private static void DisposedIn(Parameters parameters, HashSet disposals, IMethodSymbol symbol) + { + MethodDeclarationSyntax? method = + symbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as MethodDeclarationSyntax; + + if (method is not null) + DisposedIn(parameters, disposals, method); + } + + private static void DisposedIn(Parameters parameters, HashSet disposals, MethodDeclarationSyntax method) + { + if (method.ExpressionBody is not null) + { + DisposedIn(parameters, disposals, method.ExpressionBody.Expression); + } + else if (method.Body is not null) + { + DisposedIn(parameters, disposals, method.Body); + } + } + + private static void DisposedIn(Parameters parameters, HashSet disposals, ExpressionSyntax expression) + { + if (expression is AwaitExpressionSyntax awaitExpression) + { + expression = awaitExpression.Expression; + if (expression is InvocationExpressionSyntax awaitInvocationExpression && + awaitInvocationExpression.Expression is MemberAccessExpressionSyntax awaitMemberAccessExpression && + awaitMemberAccessExpression.Name.Identifier.Text == "ConfigureAwait") + { + expression = awaitMemberAccessExpression.Expression; + } + } + + if (expression is InvocationExpressionSyntax invocationExpression) + { + if (parameters.IsDisposalOf(invocationExpression, null, out string? disposedSymbol)) + { + disposals.Add(disposedSymbol); + } + else if (parameters.IsLocalMethodCall(invocationExpression, out IMethodSymbol? calledMethod)) + { + // We are calling a local method on our class, keep looking for disposals. + DisposedIn(parameters, disposals, calledMethod); + } + } + else if (expression is ConditionalAccessExpressionSyntax conditionalAccessExpression && + conditionalAccessExpression.WhenNotNull is InvocationExpressionSyntax conditionalInvocationExpression && + parameters.IsDisposalOf(conditionalInvocationExpression, conditionalAccessExpression.Expression, out string? disposedSymbol)) + { + disposals.Add(disposedSymbol); + } + } + + private static void DisposedIn(Parameters parameters, HashSet disposals, StatementSyntax statement) + { + switch (statement) + { + case ExpressionStatementSyntax expressionStatement: + DisposedIn(parameters, disposals, expressionStatement.Expression); + break; + + case IfStatementSyntax ifStatement: + // Check for: + // if (field is IDisposable disposable) + // disposable.Dispose(); + if (ifStatement.Condition is IsPatternExpressionSyntax isPatternExpression && + isPatternExpression.Pattern is DeclarationPatternSyntax declarationPattern && + declarationPattern.Type is IdentifierNameSyntax identifierName && + identifierName.Identifier.Text.EndsWith("Disposable", StringComparison.Ordinal) && + declarationPattern.Designation is SingleVariableDesignationSyntax singleVariableDesignation) + { + string? member = GetIdentifier(isPatternExpression.Expression); + if (member is not null && parameters.HasSymbolFor(member)) + { + string variable = singleVariableDesignation.Identifier.Text; + HashSet disposedSymbols = new(); + DisposedIn(parameters.With(variable), disposedSymbols, ifStatement.Statement); + if (disposedSymbols.Contains(variable)) + { + disposals.Add(member); + } + } + } + + // In other cases we don't care about the condition + DisposedIn(parameters, disposals, ifStatement.Statement); + if (ifStatement.Else is not null) + DisposedIn(parameters, disposals, ifStatement.Else.Statement); + break; + + case BlockSyntax block: + DisposedIn(parameters, disposals, block.Statements); + break; + + case SwitchStatementSyntax switchStatement: + foreach (var caseStatements in switchStatement.Sections.Select(x => x.Statements)) + { + DisposedIn(parameters, disposals, caseStatements); + } + + break; + + case TryStatementSyntax tryStatement: + DisposedIn(parameters, disposals, tryStatement.Block); + if (tryStatement.Finally is not null) + DisposedIn(parameters, disposals, tryStatement.Finally.Block); + break; + + default: + break; + } + } + + private static void DisposedIn(Parameters parameters, HashSet disposals, SyntaxList statements) + { + foreach (var statement in statements) + { + DisposedIn(parameters, disposals, statement); + } + } + + #endregion + + private static string? GetIdentifier(ExpressionSyntax expression) + { +#if NETSTANDARD2_0_OR_GREATER + // Account for 'Release(field!)' + if (expression is PostfixUnaryExpressionSyntax postfixUnaryExpression && + postfixUnaryExpression.IsKind(SyntaxKind.SuppressNullableWarningExpression)) + { + expression = postfixUnaryExpression.Operand; + } +#endif + + if (expression is IdentifierNameSyntax identifierName) + { + return identifierName.Identifier.Text; + } + else if (expression is MemberAccessExpressionSyntax memberAccessExpression && + memberAccessExpression.Expression is ThisExpressionSyntax) + { + return memberAccessExpression.Name.Identifier.Text; + } + + return null; + } + + private sealed class Parameters + { + private readonly INamedTypeSymbol type; + private readonly ImmutableHashSet disposeMethods; + private readonly HashSet names; + + public Parameters(SemanticModel model, INamedTypeSymbol type, ImmutableHashSet disposeMethods, HashSet names) + { + this.Model = model; + this.type = type; + this.disposeMethods = disposeMethods; + this.names = names; + } + + public SemanticModel Model { get; } + + public bool IsLocalMethodCall( + InvocationExpressionSyntax invocationExpression, + [NotNullWhen(true)] out IMethodSymbol? calledMethod) + { + calledMethod = this.Model.GetSymbolInfo(invocationExpression).Symbol as IMethodSymbol; + return calledMethod is not null && + SymbolEqualityComparer.Default.Equals(calledMethod.ContainingType, this.type); + } + + public bool IsDisposalOf( + InvocationExpressionSyntax invocationExpression, + ExpressionSyntax? conditionalTarget, + [NotNullWhen(true)] out string? symbol) + { + SeparatedSyntaxList arguments = invocationExpression.ArgumentList.Arguments; + if (arguments.Count > 1) + { + symbol = null; + return false; + } + + SimpleNameSyntax? calledMethod = GetCalledMethod(invocationExpression.Expression, out ExpressionSyntax? target); + if (calledMethod is null || !this.IsDisposeMethod(calledMethod)) + { + symbol = null; + return false; + } + + if (arguments.Count == 0 && (target is not null || conditionalTarget is not null)) + { + // This must be `diposable(?).DisposeMethod()` + symbol = GetIdentifier(target ?? conditionalTarget!); + } + else if (arguments.Count == 1) + { + // This must be a `(factory.)DiposeMethod(disposable)` + symbol = GetIdentifier(arguments[0].Expression); + } + else + { + symbol = null; + } + + return symbol is not null && this.HasSymbolFor(symbol); + } + + public bool HasSymbolFor(string name) => this.names.Contains(name); + + public bool IsDisposeMethod(SimpleNameSyntax name) => this.disposeMethods.Contains(name.Identifier.Text); + + public Parameters With(string name) => new(this.Model, this.type, this.disposeMethods, new HashSet() { name }); + + private static SimpleNameSyntax? GetCalledMethod(ExpressionSyntax expression, out ExpressionSyntax? target) + { + // Get the called method name. + if (expression is MemberAccessExpressionSyntax memberAccessExpression) + { + target = memberAccessExpression.Expression is ThisExpressionSyntax ? null : memberAccessExpression.Expression; + return memberAccessExpression.Name; + } + else if (expression is SimpleNameSyntax simpleName) + { + target = null; + return simpleName; + } + else if (expression is MemberBindingExpressionSyntax memberBindingExpression) + { + target = null; + return memberBindingExpression.Name; + } + else + { + target = null; + return null; + } + } + } + } +} diff --git a/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownConstants.cs b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownConstants.cs new file mode 100644 index 00000000..04453dbf --- /dev/null +++ b/src/nunit.analyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownConstants.cs @@ -0,0 +1,9 @@ +namespace NUnit.Analyzers.DisposeFieldsInTearDown +{ + internal static class DisposeFieldsAndPropertiesInTearDownConstants + { + internal const string FieldOrPropertyIsNotDisposedInTearDownTitle = "An IDisposable field/property should be Disposed in a TearDown method"; + internal const string FieldOrPropertyIsNotDisposedInTearDownDescription = "An IDisposable field/property should be Disposed in a TearDown method."; + internal const string FieldOrPropertyIsNotDisposedInTearDownMessageFormat = "The {0} {1} should be Disposed in a method annotated with [{2}]"; + } +} diff --git a/src/nunit.analyzers/Extensions/ITypeSymbolExtensions.cs b/src/nunit.analyzers/Extensions/ITypeSymbolExtensions.cs index d75e59ec..1048a7cf 100644 --- a/src/nunit.analyzers/Extensions/ITypeSymbolExtensions.cs +++ b/src/nunit.analyzers/Extensions/ITypeSymbolExtensions.cs @@ -194,5 +194,22 @@ internal static bool IsIEnumerable(this ITypeSymbol @this, out ITypeSymbol? elem return false; } + + /// + /// Return value indicates whether type implements IDisposable. + /// + internal static bool IsDisposable(this ITypeSymbol @this) + { + if (@this is ITypeParameterSymbol typeParameter) + return typeParameter.ConstraintTypes.Any(t => t.IsDisposable()); + + return @this.GetFullMetadataName().IsDisposable() || + @this.AllInterfaces.Any(i => i.GetFullMetadataName().IsDisposable()); + } + + internal static bool IsDisposable(this string fullName) + { + return fullName is "System.IDisposable" or "System.IAsyncDisposable"; + } } }