TypesThatOwnDisposableFieldsShouldBeDisposable rule (CA1001) is too permissive - by Nicole Calinoiu

Status : 

  Postponed<br /><br />
		Due to current priorities, the product team decided to postpone the resolution of this item.<br /><br />
		A more detailed explanation for the resolution of this particular item may have been provided in the comments section.


0
0
Sign in
to vote
ID 485291 Comments
Status Closed Workarounds
Type Bug Repros 0
Opened 8/26/2009 6:19:23 AM
Access Restriction Public

Description

The TypesThatOwnDisposableFieldsShouldBeDisposable rule detects disposable fields solely via examination of the values directly assigned to such fields (via a stfld opcode), and only if that value is the result of a direct initialization ("new") or a call to Activator.CreateInstance.  This has a couple of unfortunate consequences:

1.  The rule never detects a disposable field whose value is set exclusively via a property.  Amongst other problems, this means that disposable fields generated via automatic properties are never detected.

2.  The rule never detects a disposable field whose value is provided via some other means.  This a general problem when using factory or IoC approaches, or even simply when creating helper methods to assist with object creation and configuration.  However, there are also plenty of BCL types that either do not allow direct initialization (e.g.: RegistryKey) or that can also be instantiated via helper methods.

The rule would be rendered more dependable if it flagged both of the following cases when screening for disposable fields:

1.  Any field whose declared type is IDisposable or an implementation of IDisposable should be considered disposable, regardless of how its value is assigned.

2.  Any assignment of an identifiably disposable value to a field, regardless of whether the assignment is direct or via a property, should result in the field being considered disposable even if the field's declared type does not implement IDisposable.  (The source value may be the result of a direct initialization, any method or property getter call, or read directly from a local variable, method paramter, or field.)

While it is true that opening up the source will result in more false positives, it will most likely discover considerably more real problems.  Also, it is quite possible to get some problems that are difficult to resolve even with the current rule implementation.  For example, in the following scenario, which would cause the current rule to fire, class B only "owns" its A instance some of the time:

	public class A : IDisposable
	{
		private static readonly A _default = new A(null);

		public A(object data)
		{
			this.Data = data;
		}

		public void Dispose()
		{
		}

		public object Data { get; private set; }

		public static A Default
		{
			get
			{
				return A._default;
			}
		}
	}

	public class B
	{
		private readonly A _a;

		public B(bool useDefaultA)
		{
			if (useDefaultA)
			{
				this._a = A.Default;
			}
			else
			{
				this._a = new A(DateTime.Now);
			}
		}

		public A A
		{
			get
			{
				return this._a;
			}
		}
	}

(Obviously, any modifications to the identification of disposable fields should also be applied to the DisposableFieldsShouldBeDisposed rule.)
Sign in to post a comment.
Posted by Microsoft on 8/27/2009 at 1:39 PM
Hi Nicole,

Thank you for your feedback. We have reproduced the problem and it's a known limitation of the rule. We appreciate your suggestion and we will consider improving this rule in a future release of the rule.

thanks,
Brahmnes
Posted by Microsoft on 8/26/2009 at 8:22 PM
Thanks for your feedback.

We are routing this issue to the appropriate group within the Visual Studio Product Team for triage and resolution.
These specialized experts will follow-up with your issue.
Posted by David A Nelson on 8/26/2009 at 8:40 AM
The rule is called TypesThatOwnDisposableFieldsShouldBeDisposable, the key term being "Own". In the scenarios you describe, there is no way to know if the object owns the field value or not. This would lead to a huge number of false positives, greatly reducing the usefulness of the rule, and probably resulting in most people turning it off.