IDisposable created within a method and returned

Question

I happy coded quite a project that works fine and do not manifest any oddities at runtime. So I've decided to run static code analysis tool (I'm using Visual Studio 2010). It came out that rule CA2000 is being violated, message as follows:

Warning - CA2000 : Microsoft.Reliability : In method 'Bar.getDefaultFoo()', call System.IDisposable.Dispose on object 'new Foo()' before all references to it are out of scope.

The code refered goes like this:

private static IFoo getDefaultFoo()
{
    return (Baz.canIDoIt()) ? new Foo() : null;
}

I thought myself: maybe conditional expression spoils the logic (mine or validator's). Changed to this:

private static IFoo getDefaultFoo()
{
    IFoo ret = null;
    if (Baz.canIDoIt())
    {
        retFoo = new Foo();
    }
    return ret;
}

Same thing happened again, but now the object was referred to as retFoo. I've googled, I've msdn'ed, I've stackoverflow'ed. Found this article. There are no operations I need done after creating the object. I only need to return the reference to it. However, I have tried to apply the pattern suggested in OpenPort2 example. Now code looks like this:

private static IFoo getDefaultFoo()
{
    Foo tempFoo = null;
    Foo retFoo = null;
    try
    {
        if (Baz.canIDoIt())
        {
            tempFoo = new Foo();
        }
        retFoo= tempFoo;
        tempFoo = null;
    }
    finally
    {
        if (tempFoo != null)
        {
            tempFoo.Dispose();
        }
    }
    return retFoo;
}

Same message again, but tempFoo variable is rule violator this time. So basically, code went twisted, longer, little irrational, unnecesarrily complex and does the very same, but slower.

I've also found this question, where same rule seems to attack a valid code in similar manner. And there questioner is adviced to ignore the warning. I've also read this thread and a mass of similar questions.

Is there anything I missed? Is the rule bugged / irrelevant? What should I do? Ignore? Handle in some magickal way? Maybe apply some design pattern?

Edit:

After Nicole's request, I'm submiting the whole related code in form I also tried of using.

public class DisposableFooTest
{
    public interface IFoo
    {
        void bar();
    }

    public class Foo : IFoo, IDisposable
    {
        public void bar()
        {
            Console.Out.WriteLine("Foo baring now");
        }

        public void Dispose()
        {
            // actual Dispose implementation is irrelevant, or maybe it is?
            // anyway I followed microsoft dispose pattern
            // with Dispose(bool disposing)
        }
    }

    public static class Baz
    {
        private static bool toggle = false;
        public static bool canIDoIt()
        {
            toggle ^= true;
            return toggle;
        }
    }

    private static IFoo getDefaultFoo()
    {
        IFoo result = null;
        try
        {
            if (Baz.canIDoIt())
            {
                result = new Foo();
            }

            return result;
        }
        catch
        {
            if (result != null)
            {
                (result as IDisposable).Dispose();
                // IFoo does not inherit from IDisposable, hence the cast
            }

            throw;
        }
    }

    public static void Main()
    {
        IFoo bar = getDefaultFoo();
    }
}

Analysis report contain the folowing:
`CA2000 : Microsoft.Reliability : In method 'DisposableFooTest.getDefaultFoo()', call System.IDisposable.Dispose on object 'result' before all references to it are out of scope. %%projectpath%%\DisposableFooTest.cs 44 Test

Edit2:

The following approach resolved CA2000 problem:

private static IFoo getDefaultFoo()
{
    Foo result = null;
    try
    {
        if (Baz.canIDoIt())
        {
            result = new Foo();
        }
        return result;
    }
    finally
    {
        if (result != null)
        {
            result.Dispose();
        }
    }
}

Unfortunately, I can't go that way. What is more, I'd rather expect following object-oriented principles, good practices and guidelines to simplify the code, make it readable, maintanable and extendible. I doubt anyone read it as intended: give the Foo if possible, or null otherwise.

Answer 1

This is a false positive warning. There is no way to return an appropriate instance of IFoo, if IFoo implements IDisposable, without the code analysis tool warning you that you're not disposing of it properly.

The code analysis doesn't analyze your intent or logic, it just tries to warn about common errors. In this case, it "looks like" you're using an IDisposable object and not calling Dispose(). Here, you're doing it by design, as you want your method to return a new instance and its acting as a form of factory method.

Answer 2

The problem here isn't something that your C# code is explicitly doing, but rather that the generated IL is using multiple instructions to accomplish what looks like one "step" in the C# code.

For example, in the third version (with the try/finally), the return retFoo line actually involves assignment of an additional variable that is generated by the compiler and that you cannot see in your original code. Since this assignment takes place outside the try/finally block, it really does introduce potential for an unhandled exception to leave your disposable Foo instance "orphaned".

Here's a version that will avoid the potential problem (and pass CA2000 screening):

private static IFoo getDefaultFoo()
{
    IFoo result = null;
    try
    {
        if (Baz.canIDoIt())
        {
            result = new Foo();
        }

        return result;
    }
    catch
    {
        if (result != null)
        {
            result.Dispose();
        }

        throw;
    }
}

Obviously, that hampers readability and maintainability quite a bit as compared to the first version, so you'll have to decide whether the likelihood of orphaning and the consequences of not disposing an orphan actually merit the code change, or whether suppression of the CA2000 violation might be preferable in this particular case.

Answer 3

Static analysis is basically complaining for the following reason:

1. The IFoo interface does not inherit from IDisposable,
2. But you are returning a concrete implementation which must be disposed, and callers won't have a way of knowing this.

In other words, the caller of your GetDefaultFoo method expects an IFoo implementation, but doesn't have a clue that your implementation needs to be explicitly disposed, and will therefore probably not dispose it. If this was a common practice in other libraries, you would have to manually check if an item implements IDisposable for, well, every possible implementation of any possible interface.

Obviously, there is something wrong with that.

IMHO, the cleanest way of fixing this would be to make IFoo inherit from IDisposable:

public interface IFoo : IDisposable
{
    void Bar();
}

This way, callers know that they any possible implementation they receive must be disposed. And this will also allow the static analyzer to check all places where you are using an IFoo object, and warn you if you are not disposing them properly.

Answer 4

When returning an IDisposable object, in the event of a null value, dispose of the object and return null in the method - this should clear up the 'warning/recommendation' message associated with your 'private static IFoo getDefaultFoo()' method. Off course you still need to deal with the object as an IDisposable (using or dispose) in your calling routine.

private static IFoo getDefaultFoo()
{
    IFoo ret = null;
    if (Baz.canIDoIt())
    {
        retFoo = new Foo();
    }
    return ret;
}

Change to

private static IFoo getDefaultFoo()
{
    IFoo ret = null;
    if (Baz.canIDoIt())
    {
        retFoo = new Foo();
    }
    if (ret == null)
    {
        ret.dispose();
        return null;
    }
    else
        return ret;
}