Sonar-dotnet: Fix S3881: "Implement IDisposable correctly" should allow calling GC.SuppressFinalize(this) even when there is no destructor

[mchaloupka]

Description

I am following the basic dispose pattern (https://docs.microsoft.com/en-GB/dotnet/standard/design-guidelines/dispose-pattern) but I am getting violation of the rule S3881. Even in the SonarLint documentation it seems that it is implemented correctly.

Repro steps

The following code triggers the issue:

public class Server
    : IDisposable
{
    private readonly IListener _listener;

    public Server(IListener listener)
    {
        _listener = listener;
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            _listener.Dispose();
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

Expected behavior

I would expect no error reported.

Actual behavior

Error about rule S3881.

Known workarounds

When I change the class to sealed then it is fine. It happened in .NET Core project with target framework: .NET Standard 1.4.

Related information

• SonarC# Version: 3.1.1.1583
• Visual Studio Version: 15.2 (26430.14)

[valhristov]

It is complaining because you have GC.SuppressFinalize in your Dispose method. This call is not needed if you don't have a finalizer (e.g. destructor). Yes, the call also does not have any effect if you don't have a finalizer, but why keeping a line in your code that does nothing?

[mchaloupka]

You are right, that removing GC.SuppressFinalize removes the warning. However, that would be correct solution if and only if the class is sealed.

When the class is not sealed, it has to call GC.SupressFinalize otherwise it will prevent derived types that introduce a finalizer from needing to reimplement IDisposable to call it.

Actually, when the class is not sealed you should raise the warning when the GC.SuppressFinalize is not present. See related Microsoft Code Analysis rules:
CA1063: Implement IDisposable correctly
CA1816: Call GC.SuppressFinalize correctly

[valhristov]

You are right, we are too strict here. Even though I personally don't like deep class hierarchies, adding a finalizer in a subclass is a valid scenario, and S3881 is exactly about extensibility of IDisposable implementations. We will discuss and fix that. Thanks a lot for the feedback!

[ericbl]

The fixed implementation, following Microsoft CA1816 and CA1063, is however wrong as I wrote here:
https://github.com/dotnet/docs/issues/21734
As stated in the IDispose snippet in VS, by Stephen Cleary in his blog here:
https://blog.stephencleary.com/2009/08/second-rule-of-implementing-idisposable.html
a class implementing IDisposable with only managed objects should NOT have any finalizer, and do NOT need a call to CG.SuppressFinalize

Adding CG.SuppressFinalize will not harm since
https://docs.microsoft.com/en-us/dotnet/api/system.gc.suppressfinalize?redirectedfrom=MSDN&view=net-5.0#System_GC_SuppressFinalize_System_Object_

If obj does not have a finalizer or the GC has already signaled the finalizer thread to run the finalizer, the call to the SuppressFinalize method has no effect.

but this is just useless: every .NET developer dealing mostly with managed code is adding a line of code for nothing in his code.

IMHO, it would be much better to detect if the class has a finalizer, and if no finalizer, do not ask for SuppressFinalize !
Even better would be to detect for unmanaged ressources and only then ask for a finalizer and possibly SuppressFinalize as in the 3rd rule of implementing IDisposable:
https://blog.stephencleary.com/2009/08/third-rule-of-implementing-idisposable.html