CA2213: False positive if base class Dispose is virtual

[manfred-brands]

Analyzer

Diagnostic ID: CA2213: Disposable fields should be disposed

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 8.0.0 (Latest)

Describe the bug

We are using a Disposer class to collect all disposables. This way the creation and disposal is next to each other instead of having to ensure all creations are added to a dispose method:

    this.field = Disposer.Add(new ClassThatNeedsDisposing())

The base class has a protected property Disposer and disposes all items added in its Dispose method in reverse order. This takes advantage of the IsCollectionAddMethod which is called by PointsToDataFlowOperationVisitor where the created object is escaped to prevent it from reporting.

This works when the base-class Dispose method is non-virtual. It does not work if it isvirtual, not even if it is a sealed overload of a higher up the chain Dispose method. There used to be a rule that Dispose in a non-sealed class should be virtual so a derived class can Dispose its own fields. It should not matter if the Dispose method is virtual or not as there is a rule CA2215 which enforces that such a derived Dispose methods calls base.Dispose.

Steps To Reproduce

The attached CA2213.zip contains the Disposer and the DisposableObject base class with a non-virtual Dispose that don't report CA2213. It also contains the DerivedDisposableObject with a sealed override of Dispose which does report CA2213.

Expected behavior

No errors in both ClassUsingDisposerFromDisposable and ClassUsingDisposerFromDerivedDisposable

Actual behavior

ClassUsingDisposerFromDerivedDisposable.cs(8,50): error CA2213: 'ClassUsingDisposerFromDerivedDisposable' contains field 'm_Instance' that is of IDisposable type 'ClassThatNeedsDisposing', but it is never disposed. Change the Dispose method on 'ClassUsingDisposerFromDerivedDisposable' to call Close or Dispose on this field. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2213)

There is no Dispose method on that class. Also the field m_Instance was escaped when it was added to the Disposer collection.

Additional context

When compiling with the 6.0.400 SDK and the Microsoft.CodeAnalysis.NetAnalyzers version 6.0.0 no errors were reported. Switching to the 8.0.100 SDK, even with the same Analyzers results in the above error. Using the 6.0.400 SDK with version 8.0.0 of the Analyzers also result in CA2213 being raised. Using the 8.0.100 SDK with version 8.0.0 of the Analyzers results in CA22213 being reported twice.

We have a large suite that uses this pattern we want to upgrade from NET6 to NET8, but the reported CA2213 requires us to suppress CA2213 for many fields. We don't want to disable the rule as we need it to detect if people had forgotten to Dispose a new field.

[manfred-brands]

Looking at the source code, the ownership transfer as mentioned above doesn't seem to work at all. The reason nothing is reported in the first case is because there is no possibility of a Dispose method:

From: DisposableFieldsShouldBeDisposed

 // Flag non-disposed fields only if the containing type has a Dispose method implementation.
 if (!disposed &&
     HasDisposeMethod(field.ContainingType))

With HasDisposeMethod

Looking in the base class for a virtual Dispose

foreach (var method in type.GetMembers().WhereAsArray(x => x.IsVirtual && x.Kind == SymbolKind.Method))

Which doesn't take into account sealed override which should be an easy fix:

foreach (var method in type.GetMembers().WhereAsArray(x => x.IsVirtual && !x.IsSealed && x.Kind == SymbolKind.Method))

Although I do see code going through the 'escape' code, those escaped locations are not ignored.

if (!visitedArguments.IsEmpty &&
    method.IsCollectionAddMethod(CollectionNamedTypes))
{
    // FxCop compat: The object added to a collection is considered escaped.
    foreach (var argument in visitedArguments)
    {
        HandleEscapingOperation(argument, argument.Value);
    }
}

The code in DisposableFieldsShouldBeDisposed does not call GetEscapedAbstractLocations to exclude fields that should not be considered.

I'm not sure how it should work. The pattern does work for CA2000 Dispose objects before losing scope.

[mavasani]

Dispose ownership and implementation of Dispose analyzers needs a complete overhaul. See https://github.com/dotnet/runtime/issues/29631. Until then, we can try to fine tune the existing implementation as much as possible from community contributions.

[manfred-brands]

Thanks @mavasani, I will see if I can make the changes I need here. Yes that overhaul is welcome, but nothing seems to be happening after your last comment in Aug 2020.