dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

CA2213 should be raised for collection fields with IDisposable items #7112

Open JorisCleVR opened 11 months ago

JorisCleVR commented 11 months ago

Analyzer

Diagnostic ID: CA2213

Describe the improvement

CA2213 should be raised when the disposable items are stored in a collection type.

Describe suggestions on how to achieve the rule

When a collection type field is available that stores IDisposable items the rule should be raised when the items in the list are not disposed. I am not sure how easy it is to detect whether items in a list are disposed. Unfortunately a DisposeAll method is not available natively by C# itself, otherwise it could just check if that method was called on the list field.

Additional context

The following part is an adjustment of https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2213

The following snippet shows a type TypeA that implements IDisposable.

    public class TypeA : IDisposable
    {
        protected virtual void Dispose(bool disposing)
        {
            if (disposing)
            {
                // Dispose managed resources
            }

            // Free native resources
        }

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

        // Disposable types implement a finalizer.
        ~TypeA()
        {
            Dispose(false);
        }
    }

The following snippet shows a type TypeB that violates rule CA2223 by declaring a collection type field aListFieldOfADisposableType containing items of disposable type (TypeA) and not calling Dispose on the items in the list.

    public class TypeB : IDisposable
    {
        // Assume this type has some unmanaged resources.
        List<TypeA> aListFieldOfADisposableType = new List<TypeA>() { new TypeA(), new TypeA() };
        private bool disposed = false;

        protected virtual void Dispose(bool disposing)
        {
            if (!disposed)
            {
                // Dispose of resources held by this instance.

                // Violates rule: DisposableFieldsShouldBeDisposed.
                // Should call Dispose on all items of the aListFieldOfADisposableType field;

                disposed = true;
                // Suppress finalization of this disposed instance.
                if (disposing)
                {
                    GC.SuppressFinalize(this);
                }
            }
        }

        public void Dispose()
        {
            if (!disposed)
            {
                // Dispose of resources held by this instance.
                Dispose(true);
            }
        }

        // Disposable types implement a finalizer.
        ~TypeB()
        {
            Dispose(false);
        }
    }

To fix the violation, call Dispose() on the disposable items of the collection field:

protected virtual void Dispose(bool disposing)
{
   if (!disposed)
   {
      // Dispose of resources held by this instance.
      aListFieldOfADisposableType.ForEach(x => x?.Dispose());

      disposed = true;

      // Suppress finalization of this disposed instance.
      if (disposing)
      {
          GC.SuppressFinalize(this);
      }
   }
}
mavasani commented 11 months ago

Seems like a reasonable enhancement, but might be tricky to implement as you mentioned. We need to handle all different type of collections, and also ensure we only flag those collections which we know have elements instantiated in the containing type (see the Note in https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/CA2213#rule-description). We also need to bail out if the collection is passed outside the type.

steveberdy commented 3 months ago

@mavasani I'd like to look into this if you could assign this to me.