CA1001: Types that own disposable fields should be disposable (micros…

…oft#3860)

Co-authored-by: Marco Rossignoli <marco.rossignoli@gmail.com>

.editorconfig

@@ -218,6 +218,9 @@ dotnet_diagnostic.RS0041.severity = none # not default, decreased severity becau
 # CA1824: Mark assemblies with NeutralResourcesLanguageAttribute
 dotnet_diagnostic.CA1824.severity = warning # not default, increased severity to ensure it is applied
 
+# CA1001: Types that own disposable fields should be disposable
+dotnet_diagnostic.CA1001.severity = warning # not default, increased severity to ensure it is applied
+
 # CA1304: Specify CultureInfo
 dotnet_diagnostic.CA1304.severity = warning # not default, increased severity to ensure it is applied
 

TestPlatform.sln

@@ -8,6 +8,9 @@ EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.TestPlatform.CoreUtilities", "src\Microsoft.TestPlatform.CoreUtilities\Microsoft.TestPlatform.CoreUtilities.csproj", "{50C00046-0DA3-4B5C-9F6F-7BE1145E156A}"
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{B27FAFDF-2DBA-4AB0-BA85-FD5F21D359D6}"
+	ProjectSection(SolutionItems) = preProject
+		test\.editorconfig = test\.editorconfig
+	EndProjectSection
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.TestPlatform.CoreUtilities.UnitTests", "test\Microsoft.TestPlatform.CoreUtilities.UnitTests\Microsoft.TestPlatform.CoreUtilities.UnitTests.csproj", "{01409D95-A5F1-4EBE-94B1-909D5D2D0DC3}"
 EndProject

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs

@@ -34,7 +34,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.DataCollector;
 /// we don't know how the user will implement the datacollector and they could send file out of events(wrong usage, no more expected sequential access AddAttachment->GetAttachments),
 /// so we prefer protect every collection. This not means that outcome will be "always correct"(file attached in a correct way) but at least we avoid exceptions.
 /// </summary>
-internal class DataCollectionAttachmentManager : IDataCollectionAttachmentManager
+internal class DataCollectionAttachmentManager : IDataCollectionAttachmentManager, IDisposable
 {
     private readonly object _attachmentTaskLock = new();
 
@@ -356,4 +356,17 @@ private void LogError(string errorMessage, Uri collectorUri, string collectorFri
         _messageSink?.SendMessage(args);
     }
 
+    public void Dispose()
+    {
+        Dispose(disposing: true);
+        GC.SuppressFinalize(this);
+    }
+
+    protected virtual void Dispose(bool disposing)
+    {
+        if (disposing)
+        {
+            _cancellationTokenSource.Dispose();
+        }
+    }
 }

src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs

@@ -25,6 +25,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
 /// The test plugin cache.
 /// </summary>
 /// <remarks>Making this a singleton to offer better unit testing.</remarks>
+[SuppressMessage("Design", "CA1001:Types that own disposable fields should be disposable", Justification = "Would cause a breaking change if users are inheriting this class and implement IDisposable")]
 public class TestPluginCache
 {
     private readonly Dictionary<string, Assembly?> _resolvedAssemblies = new();

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Net.Sockets;
 using System.Threading;
@@ -16,6 +17,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
 /// <summary>
 /// Communication client implementation over sockets.
 /// </summary>
+[SuppressMessage("Design", "CA1001:Types that own disposable fields should be disposable", Justification = "Would cause a breaking change if users are inheriting this class and implement IDisposable")]
 public class SocketClient : ICommunicationEndPoint
 {
     private readonly CancellationTokenSource _cancellation;

src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Net;
 using System.Net.Sockets;
@@ -19,6 +20,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
 /// <summary>
 /// Facilitates communication using sockets
 /// </summary>
+[SuppressMessage("Design", "CA1001:Types that own disposable fields should be disposable", Justification = "Would cause a breaking change if users are inheriting this class and implement IDisposable")]
 public class SocketCommunicationManager : ICommunicationManager
 {
     /// <summary>

src/Microsoft.TestPlatform.CommunicationUtilities/SocketServer.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Net.Sockets;
 using System.Threading;
@@ -16,6 +17,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
 /// <summary>
 /// Communication server implementation over sockets.
 /// </summary>
+[SuppressMessage("Design", "CA1001:Types that own disposable fields should be disposable", Justification = "Would cause a breaking change if users are inheriting this class and implement IDisposable")]
 public class SocketServer : ICommunicationEndPoint
 {
     private readonly CancellationTokenSource _cancellation;

src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/DataCollectorAttachmentProcessorWrapper.cs

@@ -27,7 +27,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestRunAttachments
 /// It tries to load the extension and it receives calls from the DataCollectorAttachmentProcessorAppDomain that
 /// acts as a proxy for the main AppDomain(the runner one).
 /// </summary>
-internal class DataCollectorAttachmentProcessorRemoteWrapper : MarshalByRefObject
+internal sealed class DataCollectorAttachmentProcessorRemoteWrapper : MarshalByRefObject, IDisposable
 {
     private readonly AnonymousPipeServerStream _pipeServerStream = new(PipeDirection.Out, HandleInheritability.None);
     private readonly object _pipeClientLock = new();

src/vstest.console/Internal/ProgressIndicator.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System;
 using System.Globalization;
 
 using Microsoft.VisualStudio.TestPlatform.Utilities;
@@ -12,7 +13,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Internal;
 /// <summary>
 /// Indicates the test run progress
 /// </summary>
-internal class ProgressIndicator : IProgressIndicator
+internal sealed class ProgressIndicator : IProgressIndicator, IDisposable
 {
     private readonly object _syncObject = new();
     private int _dotCounter;
@@ -115,4 +116,9 @@ private void Timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
             }
         }
     }
+
+    public void Dispose()
+    {
+        _timer?.Dispose();
+    }
 }

test/.editorconfig

@@ -15,6 +15,9 @@ dotnet_diagnostic.IDE0060.severity = warning
 # CA1018: Mark attributes with AttributeUsageAttribute
 dotnet_diagnostic.CA1018.severity = warning
 
+# CA1001: Types that own disposable fields should be disposable
+dotnet_diagnostic.CA1001.severity = silent # Disabled on tests as it does not matter
+
 #### C# Coding Conventions ####
 
 #### .NET Formatting Rules ####