Why does FxCop not report CA2000 for this trivial case of not-disposed class instance?

Question

The following code produces a CA2000 ("Dispose objects before losing scope") violation for the first line of Main, but not the second. I would really like the CA2000 violation for the second line, because this is a (obviously simplified) pattern often found in a large code base I work on.

Does anyone know why the violation is not produced for the second line?

public static void Main()
{
    new Disposable();
    MakeDisposable();
}

private static Disposable MakeDisposable() { return new Disposable(); }

private sealed class Disposable : IDisposable
{
    public void Dispose()
    {
    }
}

Answer 1

The short answer is, CA2000 is broken and not likely to get fixed anytime soon. See this bug report which is almost exactly your question:

The warning CA2000 in particular is a rule that has known issues and that we won’t be able to fix it up in its current form.

---

The longer answer is that getting CA2000 right is hard. In past versions of Visual Studio, particularly 2010, false CA2000 warnings fired all over the place, and there was nothing you could do to make them go away. Search Stack Overflow for any of the dozens of questions about it.

Even in cases where you could eliminate the warning, the workaround was almost worse than just leaving it in place. The problem is, in cases like the one you have here, is that you don't want the object disposed before it leaves scope of the factory method. Except, you do -- if it throws an exception. In that case, the return value of the method is lost, and the caller has no way to dispose the object for itself.

Unfortunately, trying to figure that out means doing program flow analysis of the compiled IL, to determine if any code path (including exceptional ones) allow the object to leak. The end result was, almost any case where you tried to return an IDisposable from a method would produce the error.

Microsoft responded to this by doing two things:

• Cranking down on the sensitivity of CA2000, so that it only fires on the most obvious of cases. This seems reasonable, since the obvious cases like your first line are more likely to be bugs than more obscure ones, and easier to fix.
• Taking CA2000 out of their recommended code analysis rules; note that their "Extended Correctness" ruleset no longer includes CA2000.

Along with the Roslyn compiler rewrite comes the ability for tools like FxCop to do some source-level analysis, which may be easier to get correct in this case. In the mean time, the general consensus is, just turn CA2000 off.

---

In case you're curious, a bit of testing seems to confirm that the current (2013) CA rule only fires if a locally contained, locally constructed instance of IDisposable drops out of scope. IOW, the object can't leave the method in which you new it up, or CA ignores it. This leads me to believe that CA is simply not digging into method calls for it's analysis. Besides trying to silence false positives, it may also have been part of the overall attempt to speed up the CA process by cutting out some expensive checks, which I believe happened between 2010 and 2012?

If I add bit to your example, you can see the obvious pattern of which ones get the warning:

class Program
{
    public static void Main()
    {
        new Disposable(); // CA2000

        IDisposable x;
        MakeDisposable(out x);

        Test();
    }

    public static Disposable Test()
    {
        IDisposable z;
        var x = MakeDisposable(out z);
        var y = new Disposable(); // CA2000
        return x;
    }

    private static Disposable MakeDisposable(out IDisposable result)
    {
        result = new Disposable();

        new Disposable(); // CA2000
        return new Disposable(); 
    }
} 

Answer 2

I have a hunch that this happens due to a combination of factors.

---

1. MakeDisposable is fine by itself:

There is no error due to MakeDisposable because, well why would it? You could have

using ( var disposable = MakeDisposable() )
{
    // elided
}

anywhere in your code.

---

2. The references to the IDisposable in MakeDisposable is not taken into account on Main

The call to MakeDisposable() in your Main method does not cause an error because the compiler does not know that the return value of MakeDisposable is the only reference to the IDisposable.

In other words, in the eyes of the compiler, the following forms of MakeDisposable() are equivalent:

private static Disosable MakeDisposable()
{
     return new Disosable();
}

(the original), or exposing a backing field, creating it if necessary:

private static Disposable disposable;

private static Disposable MakeDisposable()
{
    if ( disposable == null )
        disposable = new Disposable();

    return disposable;
}

private static void DisposeDisposable()
{
    // elided
}

or even exposing a backing field already created:

private static Disposable disposable = new Disposable();

private static Disposable MakeDisposable()
{
    return disposable;
}

private static void DisposeDisposable()
{
    // elided
}

so the call in Main is valid.

In Main(), where you are instantiating the IDisposable, with new Disposable(); the compiler knows that you're not passing it out of that scope so correctly gives the error.