Skip to content

Fix false positives for Dispose rules #2348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of anything of the top of my head, but this seems like a heuristic to keep an eye on in the future. Otherwise LGTM.

// 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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}

Expand Down