CA2000 passing object reference to base constructor in C#

Question

I receive a warning when I run some code through Visual Studio's Code Analysis utility which I'm not sure how to resolve. Perhaps someone here has come across a similar issue, resolved it, and is willing to share their insight.

I'm programming a custom-painted cell used in a DataGridView control. The code resembles:

public class DataGridViewMyCustomColumn : DataGridViewColumn
{
    public DataGridViewMyCustomColumn() : base(new DataGridViewMyCustomCell())
    {
    }

It generates the following warning:

CA2000 : Microsoft.Reliability : In method 'DataGridViewMyCustomColumn.DataGridViewMyCustomColumn()' call System.IDisposable.Dispose on object 'new DataGridViewMyCustomCell()' before all references to it are out of scope.

I understand it is warning me DataGridViewMyCustomCell (or a class that it inherits from) implements the IDisposable interface and the Dispose() method should be called to clean up any resources claimed by DataGridViewMyCustomCell when it is no longer.

The examples I've seen on the internet suggest a using block to scope the lifetime of the object and have the system automatically dispose it, but base isn't recognized when moved into the body of the constructor so I can't write a using block around it... which I'm not sure I'd want to do anyway, since wouldn't that instruct the run time to free the object which could still be used later inside the base class?

My question then, is the code okay as is? Or, how could it be refactored to resolve the warning? I don't want to suppress the warning unless it is truly appropriate to do so.

score 18 · Accepted Answer · answered Apr 22 '10 at 00:26

If you're using Visual Studio 2010 then CA2000 is completely broken. It may also be broken in other versions of FxCop (a.k.a. Code Analysis), but VS2010 is the only one I can vouch for. Our codebase is giving CA2000 warnings for code like this...

internal static class ConnectionManager 
{
    public static SqlConnection CreateConnection()
    {
         return new SqlConnection("our connection string");
    }
}

...indicating that the connection is not being disposed before it goes out of scope in the method. Well, yeah, that's true, but it isn't out of scope for the application as it's returned to a caller - that's the whole point of the method! In the same way, your constructor argument isn't going out of scope but is being passed to the base class, so it's a false positive from the rule rather than an actual problem.

This used to be a useful rule, but now all you can really do is turn it off until they fix it. Which is unfortunate, because the (very few) actual positives are things that should be fixed.

+1 for 'completely broken in VS10'. It's very hard to persuade people to join me on a code-quality-improvement ride when the tools make me look so stupid... — Will Dean, Nov 03 '10 at 13:08

score 2 · Answer 2 · answered Feb 19 '14 at 19:15

2

There is no safe and elegant way to have a chained constructor pass a new IDisposable object to the base constructor, since as you note it's not possible to wrap the chained constructor call in any sort of try finally block. There is an approach which is safe, but it's hardly elegant: define a utility method something like:

internal static TV storeAndReturn<TR,TV>(ref TR dest, TV value) where TV:TR
{ 
  dest = value; return value;
}

Have the constructor look something like:

protected DataGridViewMyCustomColumn(ref IDisposable cleaner) : 
   base(storeAndReturn(ref cleaner, new DataGridViewMyCustomCell()))
{
}

Code which needs a new object would then have to call a public static factory method which would call the appropriate constructor within a try/finally block whose main line would null out cleaner just before it finished, and whose finally block would call Dispose on cleaner if it's not null. Provided that every subclass defines a similar factory method, this approach will ensure that the new IDisposable object will get disposed even if an exception occurs between time it's created and the time the encapsulating object is exposed to client code. The pattern is ugly, but I'm not sure any nicer other pattern would assure correctness.

answered Feb 19 '14 at 19:15

supercat

77,689
9
166
211

It's frustrating that parent/self constructor calls are syntactically-limited for no good reason - I wish C# would allow the call to `base()` or `this()` to be within the body of the constructor, that would eliminate whole classes of problems (and safety can be guaranteed by prohibiting the dereferencing of `this` prior to the `base()`/`this()` call). – Dai Jul 20 '21 at 14:43
I think the design intention was to prevent code from manipulating a base object that hasn't yet been constructed. IMHO, that should have been accomplished by having a syntactic requirement that there be no code path that references `this` or returns successfully without calling the base constructor, nor any code path which calls the base constructor more than once. The syntax is needlessly restrictive for that purpose, though. I also think there should have been separate syntax to accommodate early and late initialization of class fields, since both constructs have legit uses. – supercat Jul 20 '21 at 16:43
and yet despite that, C# *does* allow for `virtual` calls from within a superclass constructor which defeats the point entirely :/ – Dai Jul 20 '21 at 17:08
@Dai: I don't think it provides any mechanism for such calls to be performed before control reaches the base class constructor, does it? If the base class contract specifies that subclasses must tolerate having certain virtual methods or properties called before their constructor runs (e.g. a property to indicate what base-class features the derived class is going to require) the fact that such properties get called before the derived class constructor should hardly be "astonishing". The author of a base class could not, however, read the documentation of a not-yet-written derived class. – supercat Jul 20 '21 at 17:12
Correct, virtual calls in ctors are performed in the body and not before, but the problem is that the actual virtual method override that’s invoked _can_ access the derived class’ state before it’s constructed - this is officially discouraged [everywhere](https://stackoverflow.com/questions/962132/calling-virtual-functions-inside-constructors) [including in C#](https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2214). You mention this depends on the “contract” - but C# is not expressive enough to have the compiler check it and no-one reads documentation :) – Dai Jul 20 '21 at 17:34
@Dai: Someone who inherits a class with virtual methods should read the parent's documentation about what the methods do and the circumstances in which they may be called. Deriving from classes without reading their documentation is not a recipe for reliable programs. – supercat Jul 20 '21 at 17:52

CA2000 passing object reference to base constructor in C#

2 Answers2

Linked