You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We currently have following related dispose rules that ensure that disposable objects are disposed by the creating method and/or containing type:
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.
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:
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.
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.
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 -
perform less expensive and precise analysis that generates known false reports that can be explicitly suppressed in source OR
perform more powerful but computationally expensive inter-procedural analysis OR
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.
We currently have following related dispose rules that ensure that disposable objects are disposed by the creating method and/or containing 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
ModuleMetadataconstructor have dispose ownership of the passed in dispsoablePEReaderargument, and hence are all flagged. In practice,ModuleMetadatais wrapper that creates aPEModuleobject that wraps thePEReaderobject and disposes it when the PE module is disposed. Dispose ownership is transferred from creators of ModuleMetadata, to theModuleMetadataobject to thePEModuleobject. We need a complete inter-procedural dataflow analysis to figure out this dispose ownership transfer and ensure that eitherPEModule.Disposedisposes 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:
I believe this issue captures one of the core design issues around dataflow 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.