CA2213:Disposable fields should be disposed and support for DisposeCoreAsync

[MartyIX]

Attempts to fix #4950 but I don't have any clue if the approach of the PR is correct or not.

However, this seems to be in line with https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync#implement-the-async-dispose-pattern except for naming DisposeCoreAsync vs DisposeAsyncCore:

public class ExampleAsyncDisposable : IAsyncDisposable
{
    private IAsyncDisposable? _example;

    public ExampleAsyncDisposable() =>
        _example = new NoopAsyncDisposable();

    public async ValueTask DisposeAsync()
    {
        await DisposeAsyncCore().ConfigureAwait(false);

        GC.SuppressFinalize(this);
    }

    protected virtual async ValueTask DisposeAsyncCore()
    {
        if (_example is not null)
        {
            await _example.DisposeAsync().ConfigureAwait(false);
        }

        _example = null;
    }
}

Somewhat unrelated: I created a PR in vs-threading repo where I claimed that DisposeAsyncCore seemed to be the correct name for the method but this repo uses DisposeCoreAsync (AFAIK). It's confusing.

[codecov]

Codecov Report

Merging #6976 (a907d86) into main (8b1675a) will increase coverage by 0.00%.
Report is 59 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head a907d86 differs from pull request most recent head 905cc3f. Consider uploading reports for the commit 905cc3f to get more accurate results

@@           Coverage Diff            @@
##             main    #6976    +/-   ##
========================================
  Coverage   96.42%   96.43%            
========================================
  Files        1410     1412     +2     
  Lines      335956   336528   +572     
  Branches    11095    11119    +24     
========================================
+ Hits       323956   324527   +571     
+ Misses       9207     9200     -7     
- Partials     2793     2801     +8     

[MartyIX]

@Youssef1313 @mavasani Any feedback please?

[mavasani]

Implementation of the fix looks fine to me, but I am not familiar with the semantics and naming requirements of the API for recommended dispose patterns. I'll let someone from the runtime team comment on it - @buyaa-n @carlossanlop @stephentoub @jeffhandley

[MartyIX]

Any feedback @buyaa-n @carlossanlop @stephentoub @jeffhandley please?

[MartyIX]

Friendly ping. Any feedback @buyaa-n @carlossanlop @stephentoub @jeffhandley please?

[buyaa-n]

Sorry for late reply @MartyIX, the latest Framework Design Guidelines suggests the name DisposeAsyncCore, so we should use DisposeAsyncCore.

I have never seen DisposeCoreAsync usage, though got 2 hits from source.net search, seems it is rare but used, probably only in old code. Not sure if we should cover both names.

[MartyIX]

@buyaa-n I can see in this repo:

• https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DisposeMethodsShouldCallBaseClassDispose.cs

• roslyn-analyzers/src/Utilities/Compiler/DisposeMethodKind.cs

  Lines 31 to 35 in cf0cf55

  	/// <summary>
  	/// An overridden method named DisposeCoreAsync that takes a single Boolean parameter and returns Task, as
  	/// is used when implementing the standard DisposeAsync pattern.
  	/// </summary>
  	DisposeCoreAsync,

So IMHO this repo supports DisposeCoreAsync and not DisposeAsyncCore. So I think you are right that DisposeAsyncCore is the proper name but that name might not play well with other analyzers.

probably only in old code. Not sure if we should cover both names.

Not necessarily, I found

• https://stackoverflow.com/search?q=disposeCoreAsync
• https://github.com/dotnet/razor/blob/fcae177dd5bf7a41c83ffee3da6afe03851e486b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/ProjectSystem/WindowsRazorProjectHostBase.cs#L210 (last modified a week ago)
• https://github.com/dotnet/wcf/blob/0ded969a6571d582ee493c7e82119607bcff6f97/src/System.ServiceModel.Primitives/src/Internals/System/Runtime/AsyncLock.cs#L102
• https://github.com/dotnet/aspnetcore/blob/ffa0a028464e13d46aaec0c5ad8de0725a4d5aa5/src/SignalR/clients/csharp/Client/test/UnitTests/TestConnection.cs#L204
• Plus this very repo

So I think that Github Search just does not return all results as one might expect and it might be more used (not by majority but by some non-trivial minority of users).

@buyaa-n Any next step idea?

[buyaa-n]

Thanks @MartyIX, definitely we need to support both names. Looks we could just adjust the name check in HasOverriddenDisposeCoreAsyncMethodSignature and HasVirtualDisposeCoreAsyncMethodSignature, plus update the doc/comments that it covers both versions. Doesn't not necessarily need to change the method name and Enum member name DisposeCoreAsync, what you think? I'll leave that up to you and @mavasani.

[mavasani]

Thanks @MartyIX, definitely we need to support both names. Looks we could just adjust the name check in HasOverriddenDisposeCoreAsyncMethodSignature and HasVirtualDisposeCoreAsyncMethodSignature, plus update the doc/comments that it covers both versions. Doesn't not necessarily need to change the method name and Enum member name DisposeCoreAsync, what you think? I'll leave that up to you and @mavasani.

Agree with all of @buyaa-n's assessment.

[MartyIX]

I have updated the code based on the @buyaa-n's suggestion.

I'm struggling with the VB test though. Does anyone know how to fix https://github.com/dotnet/roslyn-analyzers/pull/6976/files#diff-5a72cda0b6e4fe072ec3ad611132009049afb0a1d445e15b2997688a9f82ac4eR3741 please?

[MartyIX]

@mavasani @buyaa-n Could you please check the latest version of the PR?

I'm having difficulty with the VB test but then I'm not sure if it is required at all.

[paulomorgado]

@mavasani @buyaa-n Could you please check the latest version of the PR?

I'm having difficulty with the VB test but then I'm not sure if it is required at all.

What VB code are you having issues with?

[MartyIX]

@mavasani @buyaa-n Could you please check the latest version of the PR?
I'm having difficulty with the VB test but then I'm not sure if it is required at all.

What VB code are you having issues with?

Please see #6976 (comment). It's commented out. I can't figure out how to rewrite C# code above the commented out VB code.

[paulomorgado]

@mavasani @buyaa-n Could you please check the latest version of the PR?
I'm having difficulty with the VB test but then I'm not sure if it is required at all.

What VB code are you having issues with?

Please see #6976 (comment). It's commented out. I can't figure out how to rewrite C# code above the commented out VB code.

It would help if you said what particular line of code is getting that error.

Looks like Visual Basic's Asynchronous programming with Async and Await does not support ValueTask.

[MartyIX]

This line is problematic: https://github.com/dotnet/roslyn-analyzers/pull/6976/files#diff-5a72cda0b6e4fe072ec3ad611132009049afb0a1d445e15b2997688a9f82ac4eR3784

[MartyIX]

Looks like Visual Basic's Asynchronous programming with Async and Await does not support ValueTask.

Yes, something like this. Btw, not even MSVS did give me a useful suggestion how to fix my VB code.

[MartyIX]

I removed the VB test.

[mavasani]

@buyaa-n Please review the PR and merge if it looks good to you.

[buyaa-n]

LGTM, thank you @MartyIX