Update S3881: Correct documentation and add more unit tests (#714)

SonarSource · Aug 24, 2017 · a4bbade · a4bbade
1 parent d4b94fc
commit a4bbade
Show file tree

Hide file tree

Showing 3 changed files with 109 additions and 6 deletions.
diff --git a/sonaranalyzer-dotnet/rspec/cs/S3881_c#.html b/sonaranalyzer-dotnet/rspec/cs/S3881_c#.html
@@ -10,10 +10,9 @@
   <li> The class should not implement <code>IDisposable</code> explicitly, e.g. the <code>Dispose()</code> method should be public. </li>
   <li> The class should contain <code>protected virtual void Dispose(bool)</code> method. This method allows the derived classes to correctly dispose
   the resources of this class. </li>
-  <li> The content of the <code>Dispose()</code> method should be a single invocation of <code>Dispose(true)</code>. </li>
+  <li> The content of the <code>Dispose()</code> method should be invocation of <code>Dispose(true)</code> followed by
+  <code>GC.SuppressFinalize(this)</code> </li>
   <li> If the class has a finalizer, i.e. a destructor, the only code in its body should be a single invocation of <code>Dispose(false)</code>. </li>
-  <li> If the class has a finalizer, an additional call to <code>GC.SuppressFinalize(this)</code> is required in the <code>Dispose()</code> method.
-  </li>
   <li> If the class inherits from a class that implements <code>IDisposable</code> it must call the <code>Dispose</code>, or
   <code>Dispose(bool)</code> method of the base class from within its own implementation of <code>Dispose</code> or <code>Dispose(bool)</code>,
   respectively. This ensures that all resources from the base class are properly released. </li>
@@ -79,6 +78,7 @@ <h2>Compliant Solution</h2>
     public void Dispose()
     {
         Dispose(true);
+        GC.SuppressFinalize(this);
     }
 
     protected virtual void Dispose(bool disposing)
@@ -126,5 +126,6 @@ <h2>See</h2>
   IDisposable </li>
   <li> <a href="http://pragmateek.com/c-scope-your-global-state-changes-with-idisposable-and-the-using-statement/">Pragma Geek</a> for additional
   usages of IDisposable, beyond releasing resources. </li>
+  <li> <a href="https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netframework-4.7">IDisposable documentation</a> </li>
 </ul>
 
diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3881.html b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3881.html
@@ -10,10 +10,9 @@
   <li> The class should not implement <code>IDisposable</code> explicitly, e.g. the <code>Dispose()</code> method should be public. </li>
   <li> The class should contain <code>protected virtual void Dispose(bool)</code> method. This method allows the derived classes to correctly dispose
   the resources of this class. </li>
-  <li> The content of the <code>Dispose()</code> method should be a single invocation of <code>Dispose(true)</code>. </li>
+  <li> The content of the <code>Dispose()</code> method should be invocation of <code>Dispose(true)</code> followed by
+  <code>GC.SuppressFinalize(this)</code> </li>
   <li> If the class has a finalizer, i.e. a destructor, the only code in its body should be a single invocation of <code>Dispose(false)</code>. </li>
-  <li> If the class has a finalizer, an additional call to <code>GC.SuppressFinalize(this)</code> is required in the <code>Dispose()</code> method.
-  </li>
   <li> If the class inherits from a class that implements <code>IDisposable</code> it must call the <code>Dispose</code>, or
   <code>Dispose(bool)</code> method of the base class from within its own implementation of <code>Dispose</code> or <code>Dispose(bool)</code>,
   respectively. This ensures that all resources from the base class are properly released. </li>
@@ -79,6 +78,7 @@ <h2>Compliant Solution</h2>
     public void Dispose()
     {
         Dispose(true);
+        GC.SuppressFinalize(this);
     }
 
     protected virtual void Dispose(bool disposing)
@@ -126,5 +126,6 @@ <h2>See</h2>
   IDisposable </li>
   <li> <a href="http://pragmateek.com/c-scope-your-global-state-changes-with-idisposable-and-the-using-statement/">Pragma Geek</a> for additional
   usages of IDisposable, beyond releasing resources. </li>
+  <li> <a href="https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netframework-4.7">IDisposable documentation</a> </li>
 </ul>
 
diff --git a/...alyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/ImplementIDisposableCorrectly.cs b/...alyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/ImplementIDisposableCorrectly.cs
@@ -155,3 +155,104 @@ public class DerivedWithInterface2 : NoVirtualDispose, IMyDisposable // Complian
     {
     }
 }
+
+namespace Rspec_Compliant_Samples
+{
+    // Sealed class
+    public sealed class Foo1 : IDisposable
+    {
+        public void Dispose()
+        {
+            // Cleanup
+        }
+    }
+
+    // Simple implementation
+    public class Foo2 : IDisposable
+    {
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        protected virtual void Dispose(bool disposing)
+        {
+            // Cleanup
+        }
+    }
+
+    // Implementation with a finalizer
+    public class Foo3 : IDisposable
+    {
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        protected virtual void Dispose(bool disposing)
+        {
+            // Cleanup
+        }
+
+        ~Foo3()
+        {
+            Dispose(false);
+        }
+    }
+
+    // Base disposable class
+    public class Foo4 : DisposableBase
+    {
+        protected override void Dispose(bool disposing)
+        {
+            // Cleanup
+            // Do not forget to call base
+            base.Dispose(disposing);
+        }
+    }
+}
+
+namespace VS_Generated_Implementation
+{
+    // NOTE: this is not compliant
+    public class Foo3 : IDisposable // Noncompliant
+    {
+
+        #region IDisposable Support
+        private bool disposedValue = false; // To detect redundant calls
+
+        protected virtual void Dispose(bool disposing)
+        {
+            if (!disposedValue)
+            {
+                if (disposing)
+                {
+                    // TODO: dispose managed state (managed objects).
+                }
+
+                // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
+                // TODO: set large fields to null.
+
+                disposedValue = true;
+            }
+        }
+
+        // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.
+        // ~Foo3() {
+        //   // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
+        //   Dispose(false);
+        // }
+
+        // This code added to correctly implement the disposable pattern.
+        public void Dispose() // Secondary
+        {
+            // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
+            Dispose(true);
+            // TODO: uncomment the following line if the finalizer is overridden above.
+            // GC.SuppressFinalize(this);
+        }
+        #endregion
+    }
+}