how does CA2000 work, and my attempt to work around it

Question

I am trying to deal with this warning without suppressing it. Call it a personal challenge.

CA2000 In method 'CreateAndUse', call System.IDisposable.Dispose on object 'new Project()' before all references to it are out of scope.

assuming an IDisposable interface on an arbitrary Project class.

public void CreateAndUse()
{
    var instances = new Project().Using(_ => _.LoadItems());
}

Given the extension method:

public static TR Using<T, TR>(this T instance, Func<T, TR> expression)
    where T : IDisposable
{
    using (instance)
        return expression(instance);
}

Surely a smart compiler should be able to analyse this correctly?
I think I am missing something...

What value do your extension method give you over using the `using` statement? Is it only a few characters less code? Do you think that any other developer would appreciate your layer of indirection? Is the code easier to read and understand? This is nonsense. — jgauffin, May 13 '16 at 07:21
`CA2000` is not a compiler error, it's a Code Analysis error. Code Analysis wasn't designed to perform exhaustive flow analysis to prove you code is definitely doing the right thing, it's designed to perform fairly simple analysis to detect problematic code patterns, which it's doing here. "Disposable objects should be disposed before they go out of scope" is a little easier to implement than "verify that all possible execution paths in this particular program provably lead to `.Dispose()` being called", which is even an unsolvable problem in general. — Jeroen Mostert, May 13 '16 at 07:35
THe last comment actually has the answer. The QUESTION asked is "Surely a smart compiler should be able to analyse this correctly?" - and CA2000 is totally unrelated to the compiler at all. — TomTom, May 13 '16 at 07:40
Static code analyzers have to solve the Halting Problem. Don't have too much faith in that turning out well consistently. — Hans Passant, May 13 '16 at 08:13
@hans: I don't think you have to solve the halting problem to achieve a better analysis of the using statement - a perfect analysis maybe. — Jim, May 13 '16 at 11:44
@jgaufin: I am passing the result, a simple string in this case to a method call so in my case it seemed to read better. Some additional restructuring options do remain - I digressed and was interested in the answer nevertheless. — Jim, May 13 '16 at 11:48

score 2 · Accepted Answer · edited May 23 '17 at 11:59

2

The one question you actually ask is:

Surely a smart compiler should be able to analyse this correctly?

This shows a fundamental misunderstanding. CA2000 is not a compiler issue. It is a code analysis issue, which is totally separate from the compiler. And no, it is not "quite" smart. So, no - the compiler can do nothing here because the compiler is not involved in this.

The proper way to handle this CA is to look at it, decide whether it is relevant and then possibly suppress it on the method.

Reason?

While it often is an error to nor dispose a created object, sometimes it is the intended function. Factory methods (GetDatabaseConnection()) would be totally utterly useless if the returned object already would be disposed.

But those are "edge" cases - which the programmer can decide once he sees the warning. This is what a method level suppression is for.

See How to get rid of CA2000 warning when ownership is transferred? for a similar (though not identical) example for this case.

edited May 23 '17 at 11:59

Community

1
1

answered May 13 '16 at 07:43

TomTom

61,059
10
88
148

in my opinion, with all the analysis available in the roslyn compiler it should be able to detect this better. `/analyze` is actually a [compiler option](https://msdn.microsoft.com/en-us/library/ms173498.aspx)... – Jim May 13 '16 at 08:46
It is a compiler OPTION but it is not in the compiler. Have you ever bothered reading up on analysis? They are external. FIrst, do you actually use the new ones? (https://www.nuget.org/packages/Microsoft.CodeAnalysis)? Yes, Roslyn COULD make it better, but until you install their analysis,... and even then: Sit down and fix it ;) – TomTom May 13 '16 at 08:48
jeepers, since you asked, sure: I have written roslyn code anlaysis plugins, contributed to the stylecop analyzers and written some compilers over the years, so I do know a little about the topic. Thanks for correcting me on the code analysis as pertains to this question. Much appreciated. – Jim May 13 '16 at 08:51
I think iI rad something that Roslyn does not do anything new re code analysis for compatibility reasons until you install the nuget analyzers. They wanted to avoid users installing the new visual studio and suddenly having thousands of warnings. On top, this allows one to slowly update analyzers on a project by project basis. So, without nuget - same old game as in older VS versions.With nuget - at leasst this is worth reporting as an issue. – TomTom May 13 '16 at 08:55

BoltClock · Answer 2 · 2016-05-13T07:44:12.620

The reason CA2000 occurs is plain and simple: you have a reference to an IDisposable that hasn't been disposed in your CreateAndUse() method.

The fact that your extension method disposes of your object is irrelevant. The code analyzer can't assume that the instance is guaranteed to be disposed by the time the extension method is done. What if you end up doing some other stuff with the object within the extension method outside of its using block? What if that other stuff causes an exception (rendering the using block unreachable)? Then all of a sudden you have an undisposed object lying around. And that's what CA2000 is trying to guard you against. If nothing else, the code analyzer is smart enough to anticipate all the other ways your code could possibly fail for you.

You can choose to suppress this warning if you can guarantee that your extension method will do nothing with the IDisposable outside of the using block, one of the few times the documentation says it is OK to suppress such a warning. On the other hand, the best way to deal with the CA2000 without suppressing it is to prevent it from ever happening in the first place. And the best way to do that, is to just use using directly the way it was meant to be used:

public void CreateAndUse()
{
    List<Item> instances;

    using (var p = new Project())
        instances = p.LoadItems();
}

As you can see, the scope of the Project is narrowed down to just one using block. Pretty clear-cut. No need for an extra layer of indirection, as jgauffin has said.

But in truth, the analysis engine *could* be made smart enough to handle this kind of case, they just didn't make it that smart. It would be very narrow, however, an if-statement inside would throw a spanner in the whole thing, which means that it is probably an edge case of an edge case of usability. — Lasse V. Karlsen, May 13 '16 at 07:43
@Lasse V. Karlsen: Heh, I suppose so. I ended up thinking about this another way (see my edit). — BoltClock, May 13 '16 at 07:45

how does CA2000 work, and my attempt to work around it

2 Answers2