Tracks disposed objects through implicit interface implementations #6147

myblindy · 2022-09-03T01:03:48Z

codecov · 2022-09-07T03:03:54Z

Codecov Report

Merging #6147 (790bef2) into main (a26d1a2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6147      +/-   ##
==========================================
+ Coverage   96.03%   96.04%   +0.01%     
==========================================
  Files        1348     1360      +12     
  Lines      309832   312225    +2393     
  Branches     9967    10047      +80     
==========================================
+ Hits       297534   299886    +2352     
- Misses       9900     9932      +32     
- Partials     2398     2407       +9

MartyIX · 2022-11-04T07:02:38Z

@Evangelink We hit #6075 too. It would be great if somebody could review the PR 🙏

mavasani · 2022-11-04T08:47:51Z

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

@@ -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") &&


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

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

mavasani · 2022-11-04T08:49:37Z

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

        /// <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") &&


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";

mavasani · 2022-11-04T08:51:18Z

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

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


Use below constant:

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

Line 225 in c2bc164

public const string SystemIAsyncDisposable = "System.IAsyncDisposable";

mavasani · 2022-11-04T08:55:38Z

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

+        client.Dispose()
+	end sub
+
+	function DisposeAsync() as ValueTask implements IAsyncDisposable.DisposeAsync


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?

Probably better to add the test case for future reference.

Evangelink · 2022-11-04T09:00:12Z

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

+        client.Dispose()
+	end sub
+
+	function DisposeAsync() as ValueTask implements IAsyncDisposable.DisposeAsync


Probably better to add the test case for future reference.

Evangelink · 2022-11-04T09:01:22Z

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

+        /// </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);


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

mavasani · 2022-11-04T09:04:18Z

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

+        /// </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);


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))

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; } }

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?

mavasani

As mentioned in the review, GetDisposeMethodKind is designed to handle the explicit implementation cases upfront before moving onto checking for implicit implementation cases. The fix for the issue needs to go into the prior checks, not the later ones which are only meant to handle implicit implementations

mavasani · 2022-11-04T09:24:18Z

I believe #6075 needs a single line fix in the below code:

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

Lines 340 to 343 in c2bc164

    
           else if (method.HasDisposeAsyncMethodSignature(task, valueTask)) 
        
           { 
        
               return DisposeMethodKind.DisposeAsync; 
        
           }

It should instead be:

                else if (method.IsAsyncDisposeImplementation(iAsyncDisposable, valueTask) || method.HasDisposeAsyncMethodSignature(task, valueTask))
                {
                    return DisposeMethodKind.DisposeAsync;
                }

myblindy · 2022-11-08T19:11:09Z

You're right, that was sufficient to pass my tests. I also added the test you suggested for VB with an arbitrary name for the explicitly implemented method, and that passed as well.

mavasani · 2022-11-09T06:05:21Z

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

+        client.Dispose()
+	end sub
+
+    rem arbitrary implementation name


Oh its been ages since I saw use of rem instead of the ' for VB comments :-)!! Good old days...

I had to google how to do a struct in VB, I haven't used it since like VB6 lol

mavasani · 2022-11-09T06:07:07Z

@Evangelink - can you also do a quick review/approval? Feel free to merge the PR once you approve.

@myblindy Thank you for your contribution!

Tracks disposed objects through implicit interface implementations

47c393f

myblindy requested a review from a team as a code owner September 3, 2022 01:03

github-actions bot added the Community label Sep 3, 2022

Merge remote-tracking branch 'origin/main' into patch-01

f6fc3b0

mavasani reviewed Nov 4, 2022

View reviewed changes

Evangelink reviewed Nov 4, 2022

View reviewed changes

mavasani reviewed Nov 4, 2022

View reviewed changes

mavasani suggested changes Nov 4, 2022

View reviewed changes

alternative implementation, as well as an additional test for VB

790bef2

mavasani reviewed Nov 9, 2022

View reviewed changes

mavasani approved these changes Nov 9, 2022

View reviewed changes

Evangelink approved these changes Nov 9, 2022

View reviewed changes

Evangelink merged commit 31373ce into dotnet:main Nov 9, 2022

github-actions bot added this to the vNext milestone Nov 9, 2022

jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracks disposed objects through implicit interface implementations #6147

Tracks disposed objects through implicit interface implementations #6147

myblindy commented Sep 3, 2022

codecov bot commented Sep 7, 2022 •

edited

Loading

MartyIX commented Nov 4, 2022 •

edited

Loading

mavasani Nov 4, 2022

mavasani Nov 4, 2022

mavasani Nov 4, 2022

mavasani Nov 4, 2022

mavasani Nov 4, 2022

Evangelink Nov 4, 2022

Evangelink Nov 4, 2022

Evangelink Nov 4, 2022

mavasani Nov 4, 2022 •

edited

Loading

mavasani Nov 4, 2022

mavasani Nov 4, 2022

mavasani left a comment

mavasani commented Nov 4, 2022

myblindy commented Nov 8, 2022

mavasani Nov 9, 2022

myblindy Nov 9, 2022

mavasani commented Nov 9, 2022

	/// <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;
	}

Tracks disposed objects through implicit interface implementations #6147

Tracks disposed objects through implicit interface implementations #6147

Conversation

myblindy commented Sep 3, 2022

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

MartyIX commented Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mavasani commented Nov 4, 2022

myblindy commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Nov 9, 2022

codecov bot commented Sep 7, 2022 •

edited

Loading

MartyIX commented Nov 4, 2022 •

edited

Loading

mavasani Nov 4, 2022 •

edited

Loading