S3966: Remove FP on streams special handling

sonaranalyzer-dotnet/rspec/cs/S3966_c#.html

@@ -1,33 +1,24 @@
-<p>A proper implementation of <code>IDisposable.Dispose</code> 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.</p>
-<p>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 <code>using</code> statements.</p>
-<h2>Noncompliant Code Example</h2>
+<p>Disposing an object twice, either with the <code>using</code> keyword or by calling <code>Dispose</code> directly, in the same method is at best
+confusing and at worst error-prone. The next developer might see only one of the <code>Dispose</code>/<code>using</code> and try to use an
+already-disposed object.</p>
+<p>In addition, even if <a href="https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?view=netcore-3.1">the documentation of
+<code>Disposable</code></a> explicitly states that calling the <code>Dispose</code> 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.</p>
+<p>This rule raises an issue when, in the same method, the <code>Dispose</code> method is explicitly called twice on the same object, or when
+<code>using</code> is used with a direct call to <code>Dispose()</code>.</p>
+<h2>Noncompliant Code Examples</h2>
 <pre>
-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();
 }
 </pre>
+<pre>
+using var d = new Disposable();
+d.Dispose(); // Noncompliant {{Refactor this code to make sure 'd' is disposed only once.}}
+</pre>
 <h2>Compliant Solution</h2>
 <pre>
-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();
 </pre>
 

sonaranalyzer-dotnet/rspec/cs/S3966_c#.json

@@ -7,6 +7,7 @@
     "constantCost": "10min"
   },
   "tags": [
+    "confusing",
     "pitfall"
   ],
   "defaultSeverity": "Major",

sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json

@@ -196,6 +196,7 @@
     "S3928",
     "S3949",
     "S3963",
+    "S3966",
     "S3971",
     "S3972",
     "S3973",

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx

@@ -8534,10 +8534,10 @@
     <value>Major Code Smell</value>
   </data>
   <data name="S3966_Description" xml:space="preserve">
-    <value>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.</value>
+    <value>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.</value>
   </data>
   <data name="S3966_IsActivatedByDefault" xml:space="preserve">
-    <value>False</value>
+    <value>True</value>
   </data>
   <data name="S3966_Remediation" xml:space="preserve">
     <value>Constant/Issue</value>
@@ -8552,7 +8552,7 @@
     <value>Major</value>
   </data>
   <data name="S3966_Tags" xml:space="preserve">
-    <value>pitfall</value>
+    <value>confusing,pitfall</value>
   </data>
   <data name="S3966_Title" xml:space="preserve">
     <value>Objects should not be disposed more than once</value>

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/SymbolicExecution/ObjectsShouldNotBeDisposedMoreThanOnce.cs

@@ -27,7 +27,6 @@
 using Microsoft.CodeAnalysis.Diagnostics;
 using SonarAnalyzer.ControlFlowGraph.CSharp;
 using SonarAnalyzer.Helpers;
-using SonarAnalyzer.Helpers.CSharp;
 using SonarAnalyzer.Rules.SymbolicExecution;
 using SonarAnalyzer.ShimLayer.CSharp;
 using SonarAnalyzer.SymbolicExecution;
@@ -39,13 +38,6 @@ internal sealed class ObjectsShouldNotBeDisposedMoreThanOnce : ISymbolicExecutio
     {
         internal const string DiagnosticId = "S3966";
         private const string MessageFormat = "Refactor this code to make sure '{0}' is disposed only once.";
-        private const string LeaveOpenParameterName = "leaveOpen";
-        private const string StreamParameterName = "stream";
-
-        private static readonly ImmutableDictionary<KnownType, int> typesDisposingUnderlyingStreamWithLeaveOpenArgumentIndex =
-            ImmutableDictionary<KnownType, int>.Empty
-                .Add(KnownType.System_IO_StreamReader, 4)
-                .Add(KnownType.System_IO_StreamWriter, 3);
 
         private static readonly DiagnosticDescriptor rule =
             DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
@@ -105,7 +97,7 @@ public override ProgramState PreProcessUsingStatement(ProgramPoint programPoint,
                     newProgramState = ProcessDisposableSymbol(newProgramState, disposable.SyntaxNode, disposable.Symbol);
                 }
 
-                return ProcessStreamDisposingTypes(newProgramState, (SyntaxNode)usingFinalizer.UsingStatement.Expression ?? usingFinalizer.UsingStatement.Declaration);
+                return newProgramState;
             }
 
             public override ProgramState PreProcessInstruction(ProgramPoint programPoint, ProgramState programState)
@@ -138,8 +130,7 @@ private ProgramState PreProcessVariableDeclarator(ProgramState programState, Var
                 var disposableSymbol = semanticModel.GetDeclaredSymbol(declarator);
                 var newProgramState = programState.StoreSymbolicValue(disposableSymbol, programState.PeekValue());
 
-                newProgramState = ProcessDisposableSymbol(newProgramState, declarator, disposableSymbol);
-                return ProcessStreamDisposingTypes(newProgramState, declarator);
+                return ProcessDisposableSymbol(newProgramState, declarator, disposableSymbol);
             }
 
             private ProgramState VisitInvocationExpression(InvocationExpressionSyntax instruction, ProgramState programState)
@@ -171,46 +162,6 @@ instruction.Expression as IdentifierNameSyntax
                 return newProgramState;
             }
 
-            private ProgramState ProcessStreamDisposingTypes(ProgramState programState, SyntaxNode usingExpression)
-            {
-                var newProgramState = programState;
-                var objectCreationExpressions = usingExpression.DescendantNodes().OfType<ObjectCreationExpressionSyntax>();
-
-                foreach (var objectCreationExpression in objectCreationExpressions)
-                {
-                    if (semanticModel.GetSymbolInfo(objectCreationExpression.Type).Symbol is ISymbol symbol
-                        && symbol.GetSymbolType() is ITypeSymbol objectType
-                        && objectType.DerivesOrImplementsAny(typesDisposingUnderlyingStreamWithLeaveOpenArgumentIndex.Keys.ToImmutableArray())
-                        && objectCreationExpression.ArgumentList?.Arguments is IEnumerable<ArgumentSyntax> argumentList
-                        && argumentList.Any())
-                    {
-                        // Checks for the 'leaveOpen' argument. It is at position 4 if present for StreamReader constructor, while it is at position 3 for StreamWriter.
-                        // See #2491 for more details
-                        var leaveOpenArgumentIndex = typesDisposingUnderlyingStreamWithLeaveOpenArgumentIndex
-                            .Where(entry => objectType.DerivesOrImplements(entry.Key))
-                            .Select(entry => entry.Value)
-                            .First();
-                        if (!HasLeaveOpenTrueArgument(argumentList, leaveOpenArgumentIndex)
-                            && GetArgument(argumentList, StreamParameterName, 0) is ArgumentSyntax argument)
-                        {
-                            var streamSymbol = semanticModel.GetSymbolInfo(argument.Expression).Symbol;
-                            newProgramState = ProcessDisposableSymbol(newProgramState, argument.Expression, streamSymbol);
-                        }
-                    }
-                }
-
-                return newProgramState;
-            }
-
-            private static bool HasLeaveOpenTrueArgument(IEnumerable<ArgumentSyntax> argumentList, int leaveOpenArgumentIndex) =>
-                GetArgument(argumentList, LeaveOpenParameterName, leaveOpenArgumentIndex) is ArgumentSyntax leaveOpenArgument
-                && CSharpEquivalenceChecker.AreEquivalent(leaveOpenArgument.Expression, CSharpSyntaxHelper.TrueLiteralExpression);
-
-            private static ArgumentSyntax GetArgument(IEnumerable<ArgumentSyntax> argumentList, string argumentName, int defaultArgumentIndex) =>
-                argumentList.FirstOrDefault(arg => argumentName.Equals(arg.NameColon?.Name.Identifier.ValueText)) is { } argument
-                    ? argument
-                    : argumentList.ElementAtOrDefault(defaultArgumentIndex);
-
             private ProgramState ProcessDisposableSymbol(ProgramState programState, SyntaxNode disposeInstruction, ISymbol disposableSymbol)
             {
                 if (disposableSymbol == null) // DisposableSymbol is null when we invoke an array element

sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3966.html

@@ -1,33 +1,24 @@
-<p>A proper implementation of <code>IDisposable.Dispose</code> 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.</p>
-<p>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 <code>using</code> statements.</p>
-<h2>Noncompliant Code Example</h2>
+<p>Disposing an object twice, either with the <code>using</code> keyword or by calling <code>Dispose</code> directly, in the same method is at best
+confusing and at worst error-prone. The next developer might see only one of the <code>Dispose</code>/<code>using</code> and try to use an
+already-disposed object.</p>
+<p>In addition, even if <a href="https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?view=netcore-3.1">the documentation of
+<code>Disposable</code></a> explicitly states that calling the <code>Dispose</code> 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.</p>
+<p>This rule raises an issue when, in the same method, the <code>Dispose</code> method is explicitly called twice on the same object, or when
+<code>using</code> is used with a direct call to <code>Dispose()</code>.</p>
+<h2>Noncompliant Code Examples</h2>
 <pre>
-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();
 }
 </pre>
+<pre>
+using var d = new Disposable();
+d.Dispose(); // Noncompliant {{Refactor this code to make sure 'd' is disposed only once.}}
+</pre>
 <h2>Compliant Solution</h2>
 <pre>
-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();
 </pre>