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 does not recognize an IAsyncDisposable implementaton as sufficient. #4950

Closed
MartyIX opened this issue Mar 15, 2021 · 3 comments · Fixed by #6976
Closed

CA2213 does not recognize an IAsyncDisposable implementaton as sufficient. #4950

MartyIX opened this issue Mar 15, 2021 · 3 comments · Fixed by #6976
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@MartyIX
Copy link
Contributor

MartyIX commented Mar 15, 2021

Analyzer

Diagnostic ID: CA2013: Do not use ReferenceEquals with value types

Analyzer source

SDK: Built-in CA analyzers in .NET 5 SDK or later

Describe the bug

CA2213 seemingly does not consider DisposeAsyncCore method.

/// <summary>State object for <see cref="RestClient"/>.</summary>
private readonly RestClientContext restContext; // <<<< CA2213: WARNING EMMITED HERE.

// ...

/// <summary>
/// Frees managed resources used by the object.
/// </summary>
/// <returns>A <see cref="ValueTask">task</see> that represents the asynchronous dispose operation.</returns>
protected virtual async ValueTask DisposeAsyncCore()
{
    this.log.Debug("*");

    lock (this.disposedValueLock)
    {
        if (this.disposedValue)
        {
            this.log.Debug("$<ALREADY_DISPOSED>");
            return;
        }

        this.disposedValue = true;
    }

    this.log.Debug("Dispose REST context.");
    this.restContext.Dispose(); // // This does not make the warning go away!

    this.log.Debug("$");
}

/// <inheritdoc/>
public async ValueTask DisposeAsync()
{
    // this.restContext.Dispose(); // THIS MAKES THE WARNING GO AWAY.
    await this.DisposeAsyncCore().ConfigureAwait(false);
    GC.SuppressFinalize(this);
}

Steps To Reproduce

Please see the source code above.

Expected behavior

No warning is emitted when this.restContext.Dispose(); is called in DisposeAsyncCore.

Actual behavior

A warning is emitted when this.restContext.Dispose(); is called in DisposeAsyncCore.

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 15, 2021

@Youssef1313 Would this be something worthwhile for you? 🙏

@sharwell
Copy link
Member

@mavasani this appears to be a case where a field is not being disposed directly within Dispose(), but instead within a non-patterned helper method. Do you know the degree to which this interprocedural analysis is supported under the default configuration?

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 15, 2021

@sharwell But the name DisposeAsyncCore is standard-ish according to https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync#disposeasync-and-disposeasynccore, isn't it?

@mavasani mavasani added Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting labels Apr 15, 2021
@mavasani mavasani added this to the Unknown milestone Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
3 participants