A proper implementation of IDisposable.Dispose should allow for it to be called multiple times on the same object, however this is not
-guaranteed and could result in an exception being thrown.
It is best not to rely on this behaviour and therefore make sure an object is disposed only once on all execution paths. This is particularly true
-when dealing with nested using statements.
Disposing an object twice, either with the using keyword or by calling Dispose directly, in the same method is at best
+confusing and at worst error-prone. The next developer might see only one of the Dispose/using and try to use an
+already-disposed object.
In addition, even if the documentation of
+Disposable explicitly states that calling the Dispose method multiple times should not throw an exception, some
+implementation still do it. Thus it is safer to not dispose an object twice when possible.
This rule raises an issue when, in the same method, the Dispose method is explicitly called twice on the same object, or when
+using is used with a direct call to Dispose().
-using (Stream stream = new FileStream("file.txt", FileMode.OpenOrCreate))
+using (var d = new Disposable()) // Noncompliant
{
- using (StreamWriter writer = new StreamWriter(stream)) // Noncompliant: 'stream' will be disposed twice
- {
- // Use the writer object...
- }
+ d.Dispose();
}
+
+using var d = new Disposable();
+d.Dispose(); // Noncompliant {{Refactor this code to make sure 'd' is disposed only once.}}
+
-Stream stream = null;
-try
-{
- stream = new FileStream("file.txt", FileMode.OpenOrCreate);
- using (StreamWriter writer = new StreamWriter(stream))
- {
- stream = null;
- // Use the writer object...
- }
-}
-finally
-{
- if(stream != null)
- stream.Dispose();
-}
+using var d = new Disposable();
diff --git a/sonaranalyzer-dotnet/rspec/cs/S3966_c#.json b/sonaranalyzer-dotnet/rspec/cs/S3966_c#.json
index 9456348a8fe..6f93f74b81d 100644
--- a/sonaranalyzer-dotnet/rspec/cs/S3966_c#.json
+++ b/sonaranalyzer-dotnet/rspec/cs/S3966_c#.json
@@ -7,6 +7,7 @@
"constantCost": "10min"
},
"tags": [
+ "confusing",
"pitfall"
],
"defaultSeverity": "Major",
diff --git a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
index 4d4afe1e51d..78719307dee 100644
--- a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
+++ b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
@@ -196,6 +196,7 @@
"S3928",
"S3949",
"S3963",
+ "S3966",
"S3971",
"S3972",
"S3973",
diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
index debe7f0f32c..82aedf421d6 100644
--- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
+++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
@@ -8534,10 +8534,10 @@
A proper implementation of IDisposable.Dispose should allow for it to be called multiple times on the same object, however this is not
-guaranteed and could result in an exception being thrown.
It is best not to rely on this behaviour and therefore make sure an object is disposed only once on all execution paths. This is particularly true
-when dealing with nested using statements.
Disposing an object twice, either with the using keyword or by calling Dispose directly, in the same method is at best
+confusing and at worst error-prone. The next developer might see only one of the Dispose/using and try to use an
+already-disposed object.
In addition, even if the documentation of
+Disposable explicitly states that calling the Dispose method multiple times should not throw an exception, some
+implementation still do it. Thus it is safer to not dispose an object twice when possible.
This rule raises an issue when, in the same method, the Dispose method is explicitly called twice on the same object, or when
+using is used with a direct call to Dispose().
-using (Stream stream = new FileStream("file.txt", FileMode.OpenOrCreate))
+using (var d = new Disposable()) // Noncompliant
{
- using (StreamWriter writer = new StreamWriter(stream)) // Noncompliant: 'stream' will be disposed twice
- {
- // Use the writer object...
- }
+ d.Dispose();
}
+
+using var d = new Disposable();
+d.Dispose(); // Noncompliant {{Refactor this code to make sure 'd' is disposed only once.}}
+
-Stream stream = null;
-try
-{
- stream = new FileStream("file.txt", FileMode.OpenOrCreate);
- using (StreamWriter writer = new StreamWriter(stream))
- {
- stream = null;
- // Use the writer object...
- }
-}
-finally
-{
- if(stream != null)
- stream.Dispose();
-}
+using var d = new Disposable();
diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ObjectsShouldNotBeDisposedMoreThanOnce.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ObjectsShouldNotBeDisposedMoreThanOnce.cs
index b172979e58b..9b5fa499b28 100644
--- a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ObjectsShouldNotBeDisposedMoreThanOnce.cs
+++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ObjectsShouldNotBeDisposedMoreThanOnce.cs
@@ -7,14 +7,14 @@ public interface IInterface1 : IDisposable { }
class Program
{
- public void DisposedTwise()
+ public void DisposedTwice()
{
var d = new Disposable();
d.Dispose();
d.Dispose(); // Noncompliant
}
- public void DisposedTwise_Conditional()
+ public void DisposedTwice_Conditional()
{
IDisposable d = null;
d = new Disposable();
@@ -26,7 +26,7 @@ public void DisposedTwise_Conditional()
// ^
}
- public void DisposedTwise_Try()
+ public void DisposedTwice_Try()
{
IDisposable d = null;
try
@@ -41,74 +41,43 @@ public void DisposedTwise_Try()
}
}
- public void DisposedTwise_Array()
+ public void DisposedTwice_Array()
{
var a = new[] { new Disposable() };
a[0].Dispose();
a[0].Dispose(); // Compliant, we don't handle arrays
}
- public void Disposed_Using_WithDeclaration()
+ public void Dispose_Stream_LeaveOpenFalse()
{
- using (var d = new Disposable()) // Noncompliant
+ using (MemoryStream memoryStream = new MemoryStream()) // Compliant
+ using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, leaveOpen: false))
{
- d.Dispose();
}
}
- public void Disposed_Using_WithExpressions()
+ public void Dispose_Stream_LeaveOpenTrue()
{
- var d = new Disposable();
- using (d) // Noncompliant
+ using (MemoryStream memoryStream = new MemoryStream()) // Compliant
+ using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, leaveOpen: true))
{
- d.Dispose();
}
}
- public void Disposed_Using3(Stream str)
+ public void Disposed_Using_WithDeclaration()
{
- using (var s = new FileStream("path", FileMode.Open)) // Noncompliant
-// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- {
- using (var sr = new StreamReader(s))
- {
- }
- }
-
- Stream stream;
- using (stream = new FileStream("path", FileMode.Open)) // Noncompliant
-// ^^^^^^
- {
- using (var sr = new StreamReader(stream))
- {
- }
- }
-
- using (str)
+ using (var d = new Disposable()) // Noncompliant
{
- var sr = new StreamReader(str);
- using (sr) // Compliant, we cannot detect if 'str' was argument of the 'sr' constructor or not
- {
- }
+ d.Dispose();
}
}
- public void Disposed_Using4()
+ public void Disposed_Using_WithExpressions()
{
- Stream s = new FileStream("path", FileMode.Open);
- try
- {
- using (var sr = new StreamReader(s))
- {
- s = null;
- }
- }
- finally
+ var d = new Disposable();
+ using (d) // Noncompliant
{
- if (s != null)
- {
- s.Dispose();
- }
+ d.Dispose();
}
}
@@ -118,14 +87,14 @@ public void Disposed_Using_Parameters(IDisposable param1)
param1.Dispose(); // Noncompliant
}
- public void Close_ParametersOfDifferenceTypes(IInterface1 interface1, IDisposable interface2)
+ public void Close_ParametersOfDifferentTypes(IInterface1 interface1, IDisposable interface2)
{
// Regression test for https://github.com/SonarSource/sonar-csharp/issues/1038
interface1.Dispose(); // ok, only called once on each parameter
interface2.Dispose();
}
- public void Close_ParametersOfSameTypes(IInterface1 instance1, IInterface1 instance2)
+ public void Close_ParametersOfSameType(IInterface1 instance1, IInterface1 instance2)
{
// Regression test for https://github.com/SonarSource/sonar-csharp/issues/1038
instance1.Dispose();
@@ -140,7 +109,6 @@ public void Close_OneParameterDisposedTwice(IInterface1 instance1, IInterface1 i
instance2.Dispose(); // ok - only disposed once
}
-
}
public class Disposable : IDisposable
@@ -165,137 +133,6 @@ public void DoSomething()
}
}
- public class UsingWithLeaveOpenArgument
- {
- public void LeaveOpenTrue1()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, true))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue2()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, leaveOpen: true))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue3()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), leaveOpen: true, bufferSize: 1024))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue4()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), false, 1024, true))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue5()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), bufferSize: 2014, leaveOpen: true, detectEncodingFromByteOrderMarks: false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue6()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), leaveOpen: true, bufferSize: 1024))
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), bufferSize: 2014, leaveOpen: true, detectEncodingFromByteOrderMarks: false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue7()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamWriter writer = new StreamWriter(memoryStream))
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), bufferSize: 2014, leaveOpen: true, detectEncodingFromByteOrderMarks: false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenTrue8()
- {
- using (MemoryStream memoryStream = new MemoryStream())
- using (StreamWriter writer = new StreamWriter(leaveOpen: true, stream: memoryStream, encoding: new System.Text.UTF8Encoding(false), bufferSize: 1024))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse1()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse2()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), 1024, leaveOpen: false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse3()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamWriter writer = new StreamWriter(memoryStream, new System.Text.UTF8Encoding(false), leaveOpen: false, bufferSize: 1024))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse4()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), true, 1024, false))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse5()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamReader reader = new StreamReader(memoryStream, new System.Text.UTF8Encoding(false), bufferSize: 2014, leaveOpen: false, detectEncodingFromByteOrderMarks: true))
- {
- string str = null;
- }
- }
-
- public void LeaveOpenFalse6()
- {
- using (MemoryStream memoryStream = new MemoryStream()) // Noncompliant
- using (StreamWriter writer = new StreamWriter(leaveOpen: false, stream: memoryStream, encoding: new System.Text.UTF8Encoding(false), bufferSize: 1024))
- {
- string str = null;
- }
- }
- }
-
class TestLoopWithBreak
{
public static void LoopWithBreak(System.Collections.Generic.IEnumerable