Runtime: Consider current (.NET 4.0-era) guidance for unmanaged resources

[jnm2]

Analyzer package: Microsoft.CodeAnalysis.FxCopAnalyzers
Package Version: 2.6.2-beta2
Diagnostic ID: CA1063

Since .NET 4.0 was released in 2010, the guidance has been to use SafeHandle instead of writing finalizers and Dispose(bool) methods. The unmanagedness is fully encapsulated within the SafeHandle class, so the class that uses the SafeHandle no longer needs a finalizer or a Dispose(bool) method.

Unfortunately, the MSDN documentation has continued encouraging disposal of unmanaged resources to be put in Dispose(bool) methods, perpetuating a pattern which is verbose and has fewer safety guarantees.

For extremely rare cases where the unmanaged resource is not represented by an IntPtr handle, it's still safer to represent the unmanaged resource as a private class deriving from CriticalFinalizerObject than to store the fields directly inside the class that uses them.

(@sharwell, @davkean, may I use you as references since I've seen you speak out about this?)

Therefore, I believe this warning pushes folks in the wrong direction:

CA1063 Provide an overridable implementation of Dispose(bool) on ServicesIntegrationTests or mark the type as sealed. A call to Dispose(false) should only clean up native resources. A call to Dispose(true) should clean up both managed and native resources.

Even if the class is virtual, we can assume that any derived class will also not need a finalizer or Dispose(bool) method because best practice is for the derived class to encapsulate the unmanagedness in a SafeHandle class rather than to store unmanaged references as direct fields.

[mavasani]

The current CA1063 rule implementation mostly matches the old FxCop implementation. The only major known different in implementation is that we also flag finalizers in derived types if there is at least one base type in its inheritance chain that also overrides the finalizer.

https://github.com/dotnet/docs/issues/8463 discusses complete overhaul of dispose patterns/guidelines. When the guidelines are updated, we should potentially implement a separate rule ID to enforce those guidelines, with CA1063 still being reserved for legacy guidelines for legacy code bases which already have existing code following the Dispose(bool) pattern and would prefer consistency in newly added disposable types (though we might turn off CA1063 by default to avoid conflicting guidelines).

Let us use this issue to track implementing a new Dispose pattern rule based on the resolution of https://github.com/dotnet/docs/issues/8463.

[carlossanlop]

@mavasani is the Microsoft.NetCore.Analyzers area the most appropriate for this particular proposal? If yes, then I can move it to the runtime repo so it goes through API review.

[mavasani]

@carlossanlop Yes, this needs a clear design and approval from the runtime team before it can be implemented.

[sharwell]

The last time this came up, the conclusion is CA1063 still contains the official framework design guidance for public APIs. As with the other framework design guidelines, this rule is primarily intended for improving the quality of shared APIs. While it is often convenient for projects to adopt the same rules for non-public code (consistency/simplicity/maintainability), it is allowed for internal code to deviate from the guidelines while still adhering to their primary intent.

Other IDisposable patterns are not part of the official guidance, so it is _at best_ a non-goal of the framework design analyzers to support them. However, if a proposal came to somehow tweak the rules of the analyzers and/or split the rules such that they were more convenient for authors, it might be possible to accommodate the request without undermining the primary functionality.

With that said, my _personal_ view certainly deviates from the official design guidelines. I've found the simplest solution to be disabling CA1063 and other IDisposable rules in my own libraries.