Merge pull request #2348 from mavasani/DisposeBugs

Fix false positives for Dispose rules

Author: mavasani

src/Microsoft.NetCore.Analyzers/Core/Runtime/DisposableFieldsShouldBeDisposed.cs

@@ -39,7 +39,7 @@ public sealed class DisposableFieldsShouldBeDisposed : DiagnosticAnalyzer
         public override void Initialize(AnalysisContext context)
         {
             context.EnableConcurrentExecution();
-            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);
 
             context.RegisterCompilationStartAction(compilationContext =>
             {

src/Microsoft.NetCore.Analyzers/UnitTests/Runtime/DisposableFieldsShouldBeDisposedTests.cs

@@ -2745,5 +2745,58 @@ public void Dispose()
 }
 ");
         }
+
+        [Fact, WorkItem(2306, "https://github.com/dotnet/roslyn-analyzers/issues/2306")]
+        public void DisposableAllocationInConstructor_DisposedInGeneratedCodeFile_NoDiagnostic()
+        {
+            VerifyCSharp(@"
+using System;
+
+class A : IDisposable
+{
+    public void Dispose()
+    {
+    }
+}
+
+class B : IDisposable
+{
+    private readonly A a;
+    public B()
+    {
+        a = new A();
+    }
+
+    [System.CodeDom.Compiler.GeneratedCodeAttribute("""", """")]
+    public void Dispose()
+    {
+        a.Dispose();
+    }
+}
+");
+
+            VerifyBasic(@"
+Imports System
+
+Class A
+    Implements IDisposable
+    Public Sub Dispose() Implements IDisposable.Dispose
+    End Sub
+End Class
+
+Class B
+    Implements IDisposable
+
+    Private ReadOnly a As A
+    Sub New()
+        a = New A()
+    End Sub
+
+    <System.CodeDom.Compiler.GeneratedCodeAttribute("""", """")> _
+    Public Sub Dispose() Implements IDisposable.Dispose
+        a.Dispose()
+    End Sub
+End Class");
+        }
     }
 }

src/Microsoft.NetCore.Analyzers/UnitTests/Runtime/DisposeObjectsBeforeLosingScopeTests.cs

@@ -486,6 +486,97 @@ public bool ValueExists(int i)
 ");
         }
 
+        [Fact, WorkItem(2245, "https://github.com/dotnet/roslyn-analyzers/issues/2245")]
+        public void OutDisposableArgument_StoredIntoField_NoDiagnostic()
+        {
+            VerifyCSharp(@"
+using System;
+
+class A : IDisposable
+{
+    public void Dispose()
+    {
+    }
+}
+
+class Test
+{
+    private A _a;
+    void M(out A param)
+    {
+        param = new A();
+    }
+
+    void Method()
+    {
+        M(out _a);  // This is considered as an escape of interprocedural disposable creation.
+    }
+}
+");
+        }
+
+        [Fact, WorkItem(2245, "https://github.com/dotnet/roslyn-analyzers/issues/2245")]
+        public void OutDisposableArgument_WithinTryXXXInvocation_DisposedOnSuccessPath_NoDiagnostic()
+        {
+            VerifyCSharp(@"
+using System;
+using System.Collections.Concurrent;
+
+public class C
+{
+    private readonly ConcurrentDictionary<object, IDisposable> _dictionary;
+    public C(ConcurrentDictionary<object, IDisposable> dictionary)
+    {
+        _dictionary = dictionary;
+    }
+
+    public void Remove1(object key)
+    {
+        if (_dictionary.TryRemove(key, out IDisposable value))
+        {
+            value.Dispose();
+        }
+    }
+
+    public void Remove2(object key)
+    {
+        if (!_dictionary.TryRemove(key, out IDisposable value))
+        {
+            return;
+        }
+
+        value.Dispose();
+    }
+}");
+        }
+
+        [Fact, WorkItem(2245, "https://github.com/dotnet/roslyn-analyzers/issues/2245")]
+        public void OutDisposableArgument_WithinTryXXXInvocation_NotDisposed_Diagnostic()
+        {
+            VerifyCSharp(@"
+using System;
+using System.Collections.Concurrent;
+
+public class C
+{
+    private readonly ConcurrentDictionary<object, IDisposable> _dictionary;
+    public C(ConcurrentDictionary<object, IDisposable> dictionary)
+    {
+        _dictionary = dictionary;
+    }
+
+    public void Remove(object key)
+    {
+        if (_dictionary.TryRemove(key, out IDisposable value))
+        {
+            // value is not disposed.
+        }
+    }
+}",
+            // Test0.cs(15,40): warning CA2000: Call System.IDisposable.Dispose on object created by 'out IDisposable value' before all references to it are out of scope.
+            GetCSharpResultAt(15, 40, "out IDisposable value"));
+        }
+
         [Fact]
         public void LocalWithMultipleDisposableAssignment_DisposeCallOnSome_Diagnostic()
         {

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/DisposeAnalysis/DisposeAnalysis.DisposeDataFlowOperationVisitor.cs

@@ -335,14 +335,6 @@ protected override void PostProcessArgument(IArgumentOperation operation, bool i
             {
                 base.PostProcessArgument(operation, isEscaped);
                 if (isEscaped)
-                {
-                    PostProcessEscapedArgument();
-                }
-
-                return;
-
-                // Local functions.
-                void PostProcessEscapedArgument()
                 {
                     // Discover if a disposable object is being passed into the creation method for this new disposable object
                     // and if the new disposable object assumes ownership of that passed in disposable object.
@@ -351,8 +343,39 @@ void PostProcessEscapedArgument()
                         var pointsToValue = GetPointsToAbstractValue(operation.Value);
                         HandlePossibleEscapingOperation(operation, pointsToValue.Locations);
                     }
+                    else if (FlowBranchConditionKind == ControlFlowConditionKind.WhenFalse &&
+                        operation.Parameter.RefKind == RefKind.Out &&
+                        operation.Parent is IInvocationOperation invocation &&
+                        invocation.TargetMethod.ReturnType.SpecialType == SpecialType.System_Boolean)
+                    {
+                        // Case 1:
+                        //      // Assume 'obj' is not a valid object on the 'else' path.
+                        //      if (TryXXX(out IDisposable obj))
+                        //      {
+                        //          obj.Dispose();
+                        //      }
+                        //
+                        //      return;
+
+                        // Case 2:
+                        //      if (!TryXXX(out IDisposable obj))
+                        //      {
+                        //          return; // Assume 'obj' is not a valid object on this path.
+                        //      }
+                        //
+                        //      obj.Dispose();
+
+                        HandlePossibleInvalidatingOperation(operation);
+                    }
+                }
+                else if (operation.Parameter.RefKind == RefKind.Out || operation.Parameter.RefKind == RefKind.Ref)
+                {
+                    HandlePossibleEscapingOperation(operation, GetEscapedLocations(operation));
                 }
 
+                return;
+
+                // Local functions.
                 bool IsDisposeOwnershipTransfer()
                 {
                     if (!operation.Parameter.Type.IsDisposable(WellKnownTypeProvider.IDisposable))

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs

@@ -319,6 +319,12 @@ protected override void PostProcessArgument(IArgumentOperation operation, bool i
                         AnalysisEntityFactory.TryCreate(operation, out var analysisEntity))
                     {
                         CacheAbstractValue(operation, GetAbstractValue(analysisEntity));
+
+                        if (analysisEntity.SymbolOpt?.Kind == SymbolKind.Field)
+                        {
+                            // Ref/Out field argument is considered escaped.
+                            HandleEscapingOperation(operation, operation.Value);
+                        }
                     }
                 }
                 else if (operation.Parameter.RefKind == RefKind.Ref)
@@ -568,8 +574,16 @@ private void HandleEscapingOperation(IOperation escapingOperation, IOperation es
                 Debug.Assert(escapingOperation != null);
                 Debug.Assert(escapedInstance != null);
 
-                var escapedInstancePointsToValue = GetPointsToAbstractValue(escapedInstance);
-                AnalysisEntityFactory.TryCreate(escapedInstance, out var escapedEntityOpt);
+                PointsToAbstractValue escapedInstancePointsToValue;
+                if (AnalysisEntityFactory.TryCreate(escapedInstance, out var escapedEntityOpt))
+                {
+                    escapedInstancePointsToValue = GetAbstractValue(escapedEntityOpt);
+                }
+                else
+                {
+                    escapedInstancePointsToValue = GetPointsToAbstractValue(escapedInstance);
+                }
+
                 HandleEscapingLocations(escapingOperation, builder, escapedEntityOpt, escapedInstancePointsToValue);
             }