Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tracks disposed objects through implicit interface implementations #6147

Merged
merged 3 commits into from
Nov 9, 2022
Merged

Tracks disposed objects through implicit interface implementations #6147

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47c393f
Tracks disposed objects through implicit interface implementations
myblindy Sep 3, 2022
f6fc3b0
Merge remote-tracking branch 'origin/main' into patch-01
myblindy Sep 7, 2022
790bef2
alternative implementation, as well as an additional test for VB
myblindy Nov 8, 2022
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
141 changes: 141 additions & 0 deletions ...rs/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DisposableFieldsShouldBeDisposedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,147 @@ End Function
}.RunAsync();
}

[Fact, WorkItem(6075, "https://github.com/dotnet/roslyn-analyzers/issues/6075")]
public async Task AsyncDisposableDisposedInExplicitAsyncDisposable_Disposed_NoDiagnosticAsync()
{
await new VerifyCS.Test
{
ReferenceAssemblies = AdditionalMetadataReferences.DefaultWithAsyncInterfaces,
TestCode = @"
using System;
using System.IO;
using System.Net.Http;
using System.Threading.Tasks;

class FileStream2 : IAsyncDisposable
{
public ValueTask DisposeAsync() => default;
}

public sealed class Test : IAsyncDisposable, IDisposable
{
private readonly HttpClient client;
private readonly FileStream2 stream;

public Test()
{
client = new HttpClient();
stream = new FileStream2();
}

public void Dispose()
{
client.Dispose();
}

async ValueTask IAsyncDisposable.DisposeAsync()
{
await stream.DisposeAsync();
}
}
"
}.RunAsync();

await new VerifyVB.Test
{
ReferenceAssemblies = AdditionalMetadataReferences.DefaultWithAsyncInterfaces,
TestCode = @"
Imports System
Imports System.IO
Imports System.Net.Http
Imports System.Threading.Tasks

class FileStream2
implements IAsyncDisposable
public function DisposeAsync() as ValueTask implements IAsyncDisposable.DisposeAsync
return nothing
end function
end class

public class Test
implements IAsyncDisposable, IDisposable

private readonly client as HttpClient
private readonly stream as FileStream2

public sub new()
client = new HttpClient
stream = new FileStream2
end sub

public sub Dispose() implements IDisposable.Dispose
client.Dispose()
end sub

function DisposeAsync() as ValueTask implements IAsyncDisposable.DisposeAsync
Copy link
Contributor

Choose a reason for hiding this comment

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

For VB, the implementing method name can be an arbitrary one, say

function DisposeSomethingAsync() as ValueTask implements IAsyncDisposable.DisposeAsync

Will your logic work for the above?

Copy link
Member

Choose a reason for hiding this comment

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

Probably better to add the test case for future reference.

return stream.DisposeAsync()
end function
end class
"
}.RunAsync();
}

[Fact, WorkItem(6075, "https://github.com/dotnet/roslyn-analyzers/issues/6075")]
public async Task DisposableDisposedInExplicitDisposable_Disposed_NoDiagnosticAsync()
{
await new VerifyCS.Test
{
ReferenceAssemblies = AdditionalMetadataReferences.DefaultWithAsyncInterfaces,
TestCode = @"
using System;
using System.IO;
using System.Net.Http;
using System.Threading.Tasks;

public sealed class Test : IDisposable
{
private readonly HttpClient client;
private readonly FileStream stream;

public Test()
{
client = new HttpClient();
stream = new FileStream(""C://some-path"", FileMode.CreateNew);
}

void IDisposable.Dispose()
{
client.Dispose();
stream.Dispose();
}
}
"
}.RunAsync();

await new VerifyVB.Test
{
ReferenceAssemblies = AdditionalMetadataReferences.DefaultWithAsyncInterfaces,
TestCode = @"
Imports System
Imports System.IO
Imports System.Net.Http
Imports System.Threading.Tasks

public class Test
implements IDisposable

private readonly client as HttpClient
private readonly stream as FileStream

public sub new()
client = new HttpClient
stream = new FileStream(""C://some-path"", FileMode.CreateNew)
end sub

public sub Dispose() implements IDisposable.Dispose
client.Dispose()
stream.Dispose()
end sub
end class
"
}.RunAsync();
}

[Fact]
public async Task DisposableAllocation_AssignedThroughLocal_Disposed_NoDiagnosticAsync()
{
Expand Down
19 changes: 13 additions & 6 deletions src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,19 @@ public static bool IsAsyncDisposeImplementation([NotNullWhen(returnValue: true)]
method.IsImplementationOfInterfaceMethod(null, iAsyncDisposable, "DisposeAsync");
}

/// <summary>
/// Checks the name of the method of an implicit interface implementation (so, as is) or an explicit interface implementation (which also contains the interface as a prefix).
/// </summary>
private static bool IsImplicitOrExplicitInterfaceImplementationName(this IMethodSymbol method, string name, string interfacePrefix)
=> (method.Name == name && method.MethodKind is MethodKind.Ordinary) ||
(method.Name == $"{interfacePrefix}.{name}" && method.MethodKind is MethodKind.ExplicitInterfaceImplementation);
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this will handle the VB explicit implementation @mavasani was mentioning earlier.

Copy link
Contributor

@mavasani mavasani Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the correct way to check explicit interface implementation is to check the name for the explicit implementation method, not the method itself. IsImplicitOrExplicitInterfaceImplementationName method should take an INamedTypeSymbol disposableType parameter (the argument could be the type symbol for System.IDisposable or type symbol for System.IAsyncDisposable) instead of string interfacePrefix and the check should be as below:

private static bool IsImplicitOrExplicitInterfaceImplementationName(this IMethodSymbol method, string name, INamedTypeSymbol disposableType)
            => (method.Name == name && method.MethodKind is MethodKind.Ordinary) ||
            (method.MethodKind is MethodKind.ExplicitInterfaceImplementation && method.ExplicitInterfaceImplementations.Any(i => i.Name == name && SymbolEqualityComparer.Default.Equals(i.ContainingType, disposableType))

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also convert the above into a block body instead of expression body for clarity:

private static bool IsImplicitOrExplicitInterfaceImplementationName(this IMethodSymbol method, string name, INamedTypeSymbol disposableType)
{
    switch (method.Kind)
    {
        case MethodKind.Ordinary:
            return method.Name == name;

        case MethodKind.ExplicitInterfaceImplementation:
            return method.ExplicitInterfaceImplementations.Any(i => i.Name == name && SymbolEqualityComparer.Default.Equals(i.ContainingType, disposableType);

        default:
            return false;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, scratch all the above comments. I looked at primary entrypoint method to get Dispose method kind and it seems it already handles the explicit implementation case:

roslyn-analyzers/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

Lines 313 to 359 in c2bc164

/// <summary>
/// Gets the <see cref="DisposeMethodKind"/> for the given method.
/// </summary>
public static DisposeMethodKind GetDisposeMethodKind(
this IMethodSymbol method,
INamedTypeSymbol? iDisposable,
INamedTypeSymbol? iAsyncDisposable,
INamedTypeSymbol? task,
INamedTypeSymbol? valueTask)
{
if (method.ContainingType.IsDisposable(iDisposable, iAsyncDisposable))
{
if (IsDisposeImplementation(method, iDisposable) ||
(SymbolEqualityComparer.Default.Equals(method.ContainingType, iDisposable) &&
method.HasDisposeMethodSignature())
#if CODEANALYSIS_V3_OR_BETTER
|| (method.ContainingType.IsRefLikeType &&
method.HasDisposeSignatureByConvention())
#endif
)
{
return DisposeMethodKind.Dispose;
}
else if (method.HasDisposeBoolMethodSignature())
{
return DisposeMethodKind.DisposeBool;
}
else if (method.HasDisposeAsyncMethodSignature(task, valueTask))
{
return DisposeMethodKind.DisposeAsync;
}
else if (method.HasOverriddenDisposeCoreAsyncMethodSignature(task))
{
return DisposeMethodKind.DisposeCoreAsync;
}
else if (method.HasDisposeCloseMethodSignature())
{
return DisposeMethodKind.Close;
}
else if (method.HasDisposeCloseAsyncMethodSignature(task))
{
return DisposeMethodKind.CloseAsync;
}
}
return DisposeMethodKind.None;
}

So, if the tests you added fail, then something needs to be fixed in the above method itself?


/// <summary>
/// Checks if the given method has the signature "void Dispose()".
/// </summary>
private static bool HasDisposeMethodSignature(this IMethodSymbol method)
{
return method.Name == "Dispose" && method.MethodKind == MethodKind.Ordinary &&
return method.IsImplicitOrExplicitInterfaceImplementationName("Dispose", "System.IDisposable") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the below constant string instead of hardcoding it here:

roslyn-analyzers/src/Utilities/Compiler/WellKnownTypeNames.cs

Line 228 in c2bc164

public const string SystemIDisposable = "System.IDisposable";

method.ReturnsVoid && method.Parameters.IsEmpty;
}

Expand All @@ -243,7 +250,7 @@ public static bool HasDisposeSignatureByConvention(this IMethodSymbol method)
/// </summary>
public static bool HasDisposeBoolMethodSignature(this IMethodSymbol method)
{
if (method.Name == "Dispose" && method.MethodKind == MethodKind.Ordinary &&
if (method.IsImplicitOrExplicitInterfaceImplementationName("Dispose", "System.IDisposable") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test for Dispose(bool) explicit implementation case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion here and below, use WellKnownTypeNames.SystemIDisposable instead of hard coded string.

method.ReturnsVoid && method.Parameters.Length == 1)
{
IParameterSymbol parameter = method.Parameters[0];
Expand All @@ -260,7 +267,8 @@ public static bool HasDisposeBoolMethodSignature(this IMethodSymbol method)
/// </summary>
private static bool HasDisposeCloseMethodSignature(this IMethodSymbol method)
{
return method.Name == "Close" && method.MethodKind == MethodKind.Ordinary &&
return method.Name == "Close" &&
method.MethodKind is MethodKind.Ordinary &&
method.ReturnsVoid && method.Parameters.IsEmpty;
}

Expand All @@ -278,8 +286,7 @@ private static bool HasDisposeAsyncMethodSignature(this IMethodSymbol method,
INamedTypeSymbol? task,
INamedTypeSymbol? valueTask)
{
return method.Name == "DisposeAsync" &&
method.MethodKind == MethodKind.Ordinary &&
return method.IsImplicitOrExplicitInterfaceImplementationName("DisposeAsync", "System.IAsyncDisposable") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Use below constant:

roslyn-analyzers/src/Utilities/Compiler/WellKnownTypeNames.cs

Line 225 in c2bc164

public const string SystemIAsyncDisposable = "System.IAsyncDisposable";

method.Parameters.IsEmpty &&
(SymbolEqualityComparer.Default.Equals(method.ReturnType, task) ||
SymbolEqualityComparer.Default.Equals(method.ReturnType, valueTask));
Expand All @@ -291,7 +298,7 @@ private static bool HasDisposeAsyncMethodSignature(this IMethodSymbol method,
private static bool HasOverriddenDisposeCoreAsyncMethodSignature(this IMethodSymbol method, [NotNullWhen(returnValue: true)] INamedTypeSymbol? task)
{
return method.Name == "DisposeCoreAsync" &&
method.MethodKind == MethodKind.Ordinary &&
method.MethodKind is MethodKind.Ordinary &&
method.IsOverride &&
SymbolEqualityComparer.Default.Equals(method.ReturnType, task) &&
method.Parameters.Length == 1 &&
Expand Down