Roslyn-analyzers: CA1063 when implementing the dispose pattern for a derived class

[gpetrou]

Analyzer package

https://www.nuget.org/packages/Microsoft.CodeQuality.Analyzers

Package Version

v2.6.2

Diagnostic ID

CA1063

Repro steps

1. Follow the Implementing the dispose pattern for a derived class guide and override the Object.Finalize method.

   using System;
   

   class BaseClass : IDisposable
   {
   // Flag: Has Dispose already been called?
   bool disposed = false;

   // Public implementation of Dispose pattern callable by consumers.
   public void Dispose()
   { 
      Dispose(true);
      GC.SuppressFinalize(this);           
    }
   
   // Protected implementation of Dispose pattern.
   protected virtual void Dispose(bool disposing)
   {
      if (disposed)
         return; 
   
      if (disposing) {
         // Free any other managed objects here.
        //
      }
   
      // Free any unmanaged objects here.
      //
      disposed = true;
   }
   
      ~BaseClass()
     {
         Dispose(false);
      }
   }
   
   
         class DerivedClass : BaseClass
      {
       // Flag: Has Dispose already been called?
       bool disposed = false;
   
     // Protected implementation of Dispose pattern.
     protected override void Dispose(bool disposing)
     {
        if (disposed)
           return; 
   
        if (disposing) {
           // Free any other managed objects here.
           //
        }
   
      // Free any unmanaged objects here.
      //
      disposed = true;
   
          // Call the base class implementation.
          base.Dispose(disposing);
       }
   
        ~DerivedClass()
        {
          Dispose(false);
         }
       }
   

Expected behavior

No warnings show up.

Actual behavior

A CA1063 warning shows up. Either the analyzer is buggy or the documentation.

[mavasani]

Analyzer incorrectly generates warning CA1063: Remove the finalizer from type DerivedClass, override Dispose(bool disposing), and put the finalization logic in the code path where 'disposing' is false. We should fix the analyzer here...

[mavasani]

Tagging @duracellko, who seemed to have originally implemented the finalize override portion of the rule in d6a65038 and also added unit tests in that commit to verify the current behavior - which is different from the documented/recommended pattern in bullet number 2 here.

[mavasani]

Also tagging @333fred, who has good background on this rule and @sharwell for consensus.

I feel we should relax the implementation of this rule to allow for derived types to implement the destructor, but still check that the destructor implementation just invokes Dispose(false) and returns, i.e. basically remove this CheckFinalizeOverrideRule invocation and associated rule Remove the finalizer from type {0}, override Dispose(bool disposing), and put the finalization logic in the code path where 'disposing' is false. and move this CheckFinalizeImplementationRule outside of the ImplementsDisposableDirectly condition check.

[mavasani]

I have a PR out to implement the proposed change: https://github.com/dotnet/roslyn-analyzers/pull/1959

[duracellko]

What is reason for recommendation of implementing finalizer in derived class? I understand that destructor/finalize method does not change virtual method table. So overriden Dispose(false) is called from base class finalizer. When there is also finalizer in derived class, then Dispose(false) is called twice.

Assuming there is BaseClass:

class BaseClass : IDisposable
{
  protected virtual void Dispose(bool disposing)
  {
    // recommended Dispose(bool) implementation
  }

  ~BaseClass()
  {
    Dispose(false);
  }
}

Destructor in derived class

If derived class implements destructor

class DerivedClass
{
  protected override void Dispose(bool disposing)
  {
    // recommended Dispose(bool) implementation
    base.Dispose(disposing);
  }

  ~DerivedClass()
  {
    Dispose(false);
  }
}

then method calls look like this:

1. DerivedClass.~DerivedClass() - called from .NET GC.
2. DerivedClass.Dispose(false) - called from destructor.
3. BaseClass.Dispose(false) - called from DerivedClass.Dispose(false)
4. BaseClass.~BaseClass() - called from DerivedClass.Finalize()
5. DerivedClass.Dispose(false) - called from destructor.
6. BaseClass.Dispose(false) - called from DerivedClass.Dispose(false)

Derived class without destructor

If derived class implements destructor

class DerivedClass
{
  protected override void Dispose(bool disposing)
  {
    // recommended Dispose(bool) implementation
    base.Dispose(disposing);
  }
}

then method calls look like this:

1. BaseClass.~BaseClass() - called from .NET GC.
2. DerivedClass.Dispose(false) - called from destructor.
3. BaseClass.Dispose(false) - called from DerivedClass.Dispose(false)

[mavasani]

@duracellko If the base class has no unmanaged resources, then it will not provide a destructor and hence there will be no Dispose(false) invocation if derived type doesn’t define a destructor. If we can verify base class does provide a finalizer with correct implementation that invokes Dispose(false) and returns, your stance is valid. However, the base type might be coming from a referenced assembly for which we cannot validate the implementation of the finalizer. Given these two situations, I feel we should _relax_ the rule so we allow both patterns:

1. No finalizer in derived type due to either no unmanaged resources or knowledge about base type having a destructor with Dispose(false) invocation.
2. Finalizer in derived type with correct implementation that just invokes Dispose(false).

[mavasani]

Also note that base type with unmanaged resources that does provide a destructor at time of implementing the derived type is liable to change in future if it no longer needs unmanaged resources and hence the destructor. Guarding against such a situation needs derived type to provide a destructor with Dispose(false) invocation.

[mavasani]

I understand the concern about dual or multiple Dispose (false) invocations for base types, but these should be guarded by recommended Dispose(bool) implementation that checks upfront if disposed field is true and bails out.

[duracellko]

I understand.. Whole IDisposable pattern/recommendation is about making base class implementation working with derived classes with or without unmanaged resources. However, it's not always possible to assume correct implementation of base class. The question is, if it should be just exception to the rule. As implementation of finalizer does not cause issues (when class can handle calling Dispose(bool) twice), than it can be part of the rule.

[mavasani]

The question is, if it should be just exception to the rule. As implementation of finalizer does not cause issues (when class can handle calling Dispose(bool) twice), than it can be part of the rule.

@duracellko Sorry, I did not understand your suggestion.

[mavasani]

I feel that given there is no API contract or requirement for non-sealed IDisposable types without unmanaged resources to implement a destructor, the only way a derived type with unmanaged resources can guarantee correctness, in a future proof way, is by implementing a destructor with Dispose(false) invocation. I believe that is the likely motivation for the documentation suggesting the same.

[duracellko]

I meant, there are 2 options:

1. Assume that base class has correct implementation by default. So rule can generate warning, when there is a destructor in derived class, because it is already in base class. And if developer cannot assume that, because of external library or something, then he/she can suppress the rule in this special case.
2. Do not assume that base class has always correct implementation. and thus it is recommended to implement destructor in derived class.

I am fine with option 2, because there is only minimal impact, when destructor is in derived class.

[mavasani]

Assume that base class has correct implementation by default. So rule can generate warning, when there is a destructor in derived class, because it is already in base class.

My primary concern here is that there is no public API contract that prevents a base type from removing it's destructor if it ends up with no unmanaged resources. A correct implementation of such a base type _will_ not have a destructor, even if a prior shipped implementation had one. So our only option is to _allow_ both patterns in derived types (destructor or no destructor), without _recommending_ one over the other.

[mavasani]

By the way, given https://github.com/dotnet/roslyn-analyzers/pull/1952, if we all think stricter version is still useful if end user configurable, I think it might be worthwhile to have the relaxed implementation as the default and have an .editorconfig option to enable end users to enforce the stricter version, with the understanding that it might generate false positives for them. However, I would prefer to hear more customer feedback before going that route, so we don't add an option that no one uses :-)

[sharwell]

I disagree with the claim that the original code above is correct. The case is explicitly covered in DG Update: Dispose, Finalization, and Resource Management (search in page for BUG: Shouldn't override finalizer.). CA1063 is reporting an occurrence of this bug in user code.

[sharwell]

@gpetrou Also note even though there is not an analyzer for it currently, any use of a finalizer in user code should be reviewed as a probable bug with near certainty. The syntax has been effectively obsolete for at least 4 years now based on the date of this tweet.

[mavasani]

@sharwell The documentation does recommend SafeHandle approach over the finalizer approach:
Either a class derived from SafeHandle that wraps your unmanaged resource (recommended), or an override to the Object.Finalize method.
However, lot of existing user code does have finalizers, and an analyzer firing against all destructors is likely going to be very noisy if enabled by default. Additionally, we cannot consider a pattern deprecated, unless our documentation states so explicitly.
Regardless, for this analyzer, I feel the safest approach is to just verify that any finalizer, if present, has the recommended implementation of invoking Dispose(false).

[sharwell]

Regardless, for this analyzer, I feel the safest approach is to just verify that any finalizer, if present, has the recommended implementation of invoking Dispose(false).

In addition to verifying the form of the finalizer, the code should verify that the base type does not already provide a non-default finalizer.

• Removing a finalizer from an exposed non-sealed type is a breaking API change, so we can safely consider it as continuing to exist.
• If a finalizer is later added to the base type without updating recompiling the derived type, Dispose(false) will still only be called once by the derived type because it explicitly invokes System.Object.Finalize, bypassing the added intermediate finalizer.
• Failure to add this check will allow Dispose(false) to be called multiple times in sequence.

[mavasani]

If a finalizer is later added to the base type without updating the derived type, Dispose(false) will still only be called once by the derived type because it explicitly invokes System.Object.Finalize, bypassing the added intermediate finalizer.

That does not seem to be the case though. Following code in a simple console app leads to Dispose(false) being invoked twice.

    class Program
    {
        static void Main(string[] args)
        {
            var x = new Derived();
        }
    }

    class Base : IDisposable
    {
        public virtual void Dispose(bool disposing)
        {
        }

        public void Dispose()
        {
            Dispose(true);
        }

        ~Base()
        {
            Dispose(false);
        }
    }

    class Derived : Base
    {
        public override void Dispose(bool disposing)
        {
            base.Dispose(disposing);
        }

        ~Derived()
        {
            Dispose(false);
        }
    }

I believe that if preventing duplicate Dispose calls is our intention, then the only reliable approach is @duracellko's original implementation that flags all derived type finalizers. We can potentially re-word his diagnostic to state that this pattern can lead to duplicate Dispose calls and recommended approach is to use SafeHandles instead. This will be somewhat of a compat break from legacy FxCop and likely to cause noise, but given it moves people away from finalizers in favor of SafeHandles, I feel it is a reasonable break. I will also make this behavior configurable with an .editorconfig option so for legacy code bases or for folks that still prefer destructors, we skip this diagnostic.

[mavasani]

Actually, I think @sharwell meant we can tweak the original check that flags all Derived type destructors to do so only if at least one base type has a destructor. If base type does add a destructor in future, re-compiling the project with derived type will fire the diagnostic and we should be good. This will also prevent noise from cases where derived type is the only one in the inheritance chain to have a destructor, which is correct. For such cases, we will verify destructor is implemented correctly. Let me tweak my PR according to @sharwell's suggestion

[mavasani]

@sharwell Do we need a design meeting to decide between 3 possible approaches?

1. Outright ban destructors in derived types (@duracellko's current approach in this analyzer) , with a reworded message about preferred approach being using SafeHandles.
2. Flag destructors in derived types, only if at least one base type also has a destructor, with a reworded message about possibility of duplicate Dispose invocations. If this diagnostic does not fire, verify destructor implementation is correct (invokes Dispose(false) and returns).
3. Do not flag destructors in derived types, but only verify destructor implementation is correct (invokes Dispose(false) and returns).

[sharwell]

@mavasani Option 2 is the behavior most consistent with the current guidance relating to code using finalizers.

[sharwell]

That does not seem to be the case though. Following code in a simple console app leads to Dispose(false) being invoked twice.

My reply in https://github.com/dotnet/roslyn-analyzers/issues/1950#issuecomment-450895726 was not clear enough, and has now been edited. I was specifically referring to cases where Base and Derived are in different assemblies, with a finalizer added to Base without recompiling Derived.

[mavasani]

Great thanks @sharwell - let me update my PR accordingly.

[mavasani]

@sharwell @duracellko - https://github.com/dotnet/roslyn-analyzers/pull/1959/commits/dcd47483f1d967048f7ff7ba5eea85c1b98b312d makes the changes as discussed here, please review.

[mavasani]

@gpetrou As discussed in this thread, the approach of using SafeHandles is preferred over overriding a finalizer. I am going to file a documentation issue on the cited docs page above to make it explicit that we will flag the latter in presence of base type with a finalizer.