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: Handle overriden DisposeCoreAsync / DisposeAsyncCore #7347

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Jul 7, 2024

Follow-up to #6976

This PR fixes up CA2213 implementation to handle the following use case:

using System;
using System.Threading.Tasks;

class A : IAsyncDisposable
{
    private readonly object disposedValueLock = new object();
    private bool disposedValue;
    private readonly Inner innerA;

    public A() 
    {
        innerA = new Inner();
    }

    protected virtual async ValueTask DisposeCoreAsync()
    {
        lock (disposedValueLock)
        {
            if (disposedValue)
            {
                return;
            }

            disposedValue = true;
        }

        await innerA.DisposeAsync().ConfigureAwait(false);
    }

    public ValueTask DisposeAsync()
    {
        return default(ValueTask);
    }
}

class B : A
{
    private readonly object disposedValueLock = new object();
    private bool disposedValue;

    // Newly, we want to test that no diagnostic is reported on this line.
    private readonly Inner innerB;  /* <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< */

    public B() : base()
    {
        innerB = new Inner();
    }

    protected override async ValueTask DisposeCoreAsync()
    {
        lock (disposedValueLock)
        {
            if (disposedValue)
            {
                return;
            }

            disposedValue = true;
        }

        await innerB.DisposeAsync().ConfigureAwait(false);

        await base.DisposeCoreAsync().ConfigureAwait(false);
    }
}

// Declares an IAsyncDisposable type.
class Inner : IAsyncDisposable
{
    public Inner() 
    {
    }

    public ValueTask DisposeAsync()
    {
        return default(ValueTask);
    }
}

That is innerB is now marked as not disposed because its DisposeCoreAsync method has in its method signature override instead of virtual.

@MartyIX MartyIX requested a review from a team as a code owner July 7, 2024 19:34
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.49%. Comparing base (43709af) to head (3c81f1d).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7347    +/-   ##
========================================
  Coverage   96.49%   96.49%            
========================================
  Files        1443     1443            
  Lines      345799   345885    +86     
  Branches    11374    11374            
========================================
+ Hits       333665   333771   +106     
+ Misses       9251     9233    -18     
+ Partials     2883     2881     -2     

@MartyIX
Copy link
Contributor Author

MartyIX commented Jul 16, 2024

@buyaa-n Could you please take a look if you have a moment?

@MartyIX
Copy link
Contributor Author

MartyIX commented Jul 25, 2024

@drewnoakes Could you please take a look?

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 2, 2024

@Evangelink Could you take a look please?

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

LGTM

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 6, 2024

@buyaa-n @Evangelink Could the PR be merged?

@Evangelink
Copy link
Member

@buyaa-n this seems low risk for me to merge.

@Evangelink Evangelink merged commit 3211f48 into dotnet:main Aug 6, 2024
14 checks passed
@MartyIX MartyIX deleted the feature/2024-07-07-CA2213-override branch August 6, 2024 06:48
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.

None yet

2 participants