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.

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

CA2213:Disposable fields should be disposed and support for DisposeCoreAsync #6976

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Oct 9, 2023

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
Copy link

codecov bot commented Oct 9, 2023

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
Copy link
Contributor Author

MartyIX commented Oct 10, 2023

@Youssef1313 @mavasani Any feedback please?

@mavasani
Copy link
Member

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
Copy link
Contributor Author

MartyIX commented Oct 13, 2023

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

@MartyIX
Copy link
Contributor Author

MartyIX commented Oct 28, 2023

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

@buyaa-n
Copy link
Member

buyaa-n commented Oct 30, 2023

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
Copy link
Contributor Author

MartyIX commented Oct 30, 2023

@buyaa-n I can see in this repo:

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

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
Copy link
Member

buyaa-n commented Oct 30, 2023

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
Copy link
Member

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
Copy link
Contributor Author

MartyIX commented Nov 3, 2023

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 MartyIX marked this pull request as ready for review November 7, 2023 11:11
@MartyIX MartyIX requested a review from a team as a code owner November 7, 2023 11:11
@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 7, 2023

@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
Copy link
Contributor

@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
Copy link
Contributor Author

MartyIX commented Nov 7, 2023

@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
Copy link
Contributor

@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
Copy link
Contributor Author

MartyIX commented Nov 7, 2023

@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 7, 2023

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
Copy link
Contributor Author

MartyIX commented Nov 7, 2023

I removed the VB test.

@MartyIX MartyIX requested a review from mavasani November 7, 2023 18:52
@mavasani
Copy link
Member

mavasani commented Nov 8, 2023

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

Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @MartyIX

@buyaa-n buyaa-n enabled auto-merge (squash) November 8, 2023 19:10
@buyaa-n buyaa-n merged commit b924542 into dotnet:main Nov 8, 2023
10 of 11 checks passed
@MartyIX MartyIX deleted the feature/DisposeCoreAsync branch November 8, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CA2213 does not recognize an IAsyncDisposable implementaton as sufficient.
4 participants