Skip to content

Design for "Dispose ownership transfer" in dispose rules #1617

@mavasani

Description

@mavasani

We currently have following related dispose rules that ensure that disposable objects are disposed by the creating method and/or containing type:

  1. CA2000 (Dispose objects before losing scope): A local object of a IDisposable type is created but the object is not disposed before all references to the object are out of scope.
  2. CA2213 (Disposable fields should be disposed): A type that implements System.IDisposable declares fields that are of types that also implement IDisposable. The Dispose method of the field is not called by the Dispose method of the declaring type.

So, a disposable object created in a method body, which is not saved into an instance field and/or escaped via either calling into an external library or returned by the method, must be disposed in a method. If assigned to a field of a disposable object, then the Dispose method (or call graph rooted at Dispose) of the containing type must dispose the field. If escaped via returning the disposable object, then the caller takes over the ownership to dispose the object.

These rules seem appropriate, except for the cases where a dispose ownership transfer takes place from method that creates a disposable object to a wrapper object that holds onto this disposable object into a field and disposes the field when the wrapper is disposed. For example, see ModuleMetadata. According to above dispose design, the callers of the ModuleMetadata constructor have dispose ownership of the passed in dispsoable PEReader argument, and hence are all flagged. In practice, ModuleMetadata is wrapper that creates a PEModule object that wraps the PEReader object and disposes it when the PE module is disposed. Dispose ownership is transferred from creators of ModuleMetadata, to the ModuleMetadata object to the PEModule object. We need a complete inter-procedural dataflow analysis to figure out this dispose ownership transfer and ensure that either PEModule.Dispose disposes the PE reader (CA2213) or the methods creating PE reader disposes it (CA2000). Legacy FxCop as well as our current implementation do not perform inter-procedural analysis, and generates false negative CA2000 instances for these cases.

Design choices:

  1. Keep reporting CA2000 false positives when dispose ownership transfer takes place. End users must explicitly suppress violations in source when this takes place. However, note that CA2213 will not flag such cases even if the actual owner of the disposable object after this ownership transfer does not dispose the wrapped disposable object - CA2213 fires only if a disposable field was assigned to a locally created disposable object in one of the containing type's methods. This causes us to miss reporting cases where there is an actual dispose violation.
  2. Add support for end users to explicit specify dispose ownership transfer method/parameter pairs in an additional file that the dispose rules can use. Essentially implement Allow end-users to configure dispose ownership transfer #1587. Dispose rules can correctly detect such ownership transfers and we avoid CA2000 false positives and don't miss CA2213 violations in eventual owner.
  3. Stay with option 1 for now and fix it when and if we eventually add inter-procedural analysis. Note that the analyses will still be more computationally expensive than approach 2, but doesn't need user annotations/configuration as in approach 2.

I believe this issue captures one of the core design issues around dataflow analysis -

  1. perform less expensive and precise analysis that generates known false reports that can be explicitly suppressed in source OR
  2. perform more powerful but computationally expensive inter-procedural analysis OR
  3. take a hybrid approach of user annotations to aid analyses to produce more precise results with not so expensive analysis

I prefer 3 as it also provides an explicit (pseudo) documentation/contracts and allows user configuration for cases that analyses just cannot detect. Primary downside would be if such annotations or user configuration just grow too much and get out of sync with actual user code.

Metadata

Metadata

Assignees

Labels

DataFlowDiscussionNeeds-ProposalA proposal defining the desired behavior is needed before review/approval process can begin

Type

No type
No fields configured for issues without a type.

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions