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
nunit · Sep 6, 2023 · 81c26ba · 81c26ba
1 parent fde37e8
commit 81c26ba
Show file tree

Hide file tree

Showing 21 changed files with 1,603 additions and 10 deletions.
diff --git 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
@@ -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`.
+
+<!-- start generated config severity -->
+## 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...")]
+```
+<!-- end generated config severity -->
diff --git a/documentation/NUnit3001.md b/documentation/NUnit3001.md
@@ -99,6 +99,7 @@ To disable the rule for a project, you need to add a
     <Rule Id="NUnit3001" Action="Info" /> <!-- Possible Null Reference -->
     <Rule Id="NUnit3002" Action="Info" /> <!-- NonNullableField/Property is Uninitialized -->
     <Rule Id="NUnit3003" Action="Info" /> <!-- Avoid Uninstantiated Internal Classes -->
+    <Rule Id="NUnit3004" Action="Info" /> <!-- Types that own disposable fields should be disposable -->
   </Rules>
 </RuleSet>
 ```

diff --git a/documentation/NUnit3002.md b/documentation/NUnit3002.md
@@ -69,6 +69,7 @@ To disable the rule for a project, you need to add a
     <Rule Id="NUnit3001" Action="Info" /> <!-- Possible Null Reference -->
     <Rule Id="NUnit3002" Action="Info" /> <!-- NonNullableField/Property is Uninitialized -->
     <Rule Id="NUnit3003" Action="Info" /> <!-- Avoid Uninstantiated Internal Classes -->
+    <Rule Id="NUnit3004" Action="Info" /> <!-- Types that own disposable fields should be disposable -->
   </Rules>
 </RuleSet>
 ```

diff --git a/documentation/NUnit3003.md b/documentation/NUnit3003.md
@@ -1,14 +1,14 @@
 # NUnit3003
 
-## Class is a NUnit TestFixture and called by reflection
+## Class is an NUnit TestFixture and is instantiated using reflection
 
 | Topic    | Value
 | :--      | :--
 | Id       | NUnit3003
 | 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
     <Rule Id="NUnit3001" Action="Info" /> <!-- Possible Null Reference -->
     <Rule Id="NUnit3002" Action="Info" /> <!-- NonNullableField/Property is Uninitialized -->
     <Rule Id="NUnit3003" Action="Info" /> <!-- Avoid Uninstantiated Internal Classes -->
+    <Rule Id="NUnit3004" Action="Info" /> <!-- Types that own disposable fields should be disposable -->
   </Rules>
 </RuleSet>
 ```
@@ -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
 ```
 <!-- end generated config severity -->
diff --git a/documentation/NUnit3004.md 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.
+
+<!-- start generated config severity -->
+## 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
+<?xml version="1.0" encoding="utf-8"?>
+<RuleSet Name="NUnit.Analyzer Suppressions" Description="DiagnosticSuppression Rules" ToolsVersion="12.0">
+  <Rules AnalyzerId="DiagnosticSuppressors" RuleNamespace="NUnit.NUnitAnalyzers">
+    <Rule Id="NUnit3001" Action="Info" /> <!-- Possible Null Reference -->
+    <Rule Id="NUnit3002" Action="Info" /> <!-- NonNullableField/Property is Uninitialized -->
+    <Rule Id="NUnit3003" Action="Info" /> <!-- Avoid Uninstantiated Internal Classes -->
+    <Rule Id="NUnit3004" Action="Info" /> <!-- Types that own disposable fields should be disposable -->
+  </Rules>
+</RuleSet>
+```
+
+and add it to the project like:
+
+```xml
+<PropertyGroup>
+  <CodeAnalysisRuleSet>NUnit.Analyzers.Suppressions.ruleset</CodeAnalysisRuleSet>
+</PropertyGroup>
+```
+
+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
+```
+<!-- end generated config severity -->
diff --git 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
@@ -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
@@ -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/...nalyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs b/...nalyzers.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
@@ -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);
+        }
+    }
+}