6

... if I use an IDisposable in a local variable, but do not call Dispose() or use the using() pattern.

public void BadMethod()
{
    var fs = new FileStream("file.txt", FileMode.Create);
    fs.WriteByte(0x55);
    // no dispose, no using()
}

Just like the "Types that own disposable fields should be disposable" rule for fields.


EDIT: Replaced MemoryStream by FileStream, because MemoryStream just allocates memory and doesn't use (unmanaged) resources, so someone could discuss about a mandatory Dispose() call.

ulrichb
  • 19,610
  • 8
  • 73
  • 87
  • 1
    Why would you want that? It is pointless, memory isn't disposable. Hopefully we won't get a tool that says it is, it will destroy programmer minds irreparably. – Hans Passant Feb 06 '10 at 22:06
  • 8
    @nobugz: It's still proper here to actually call Dispose on MemoryStream. The fact that MemoryStream doesn't use any unmanaged resources is an *implementation* detail. The **contract** for MemoryStream says that it implements IDisposable, and as such, it should always have Dispose called on it. It is *always* better to code against the contract than against the specific implementation details. – casperOne Feb 06 '10 at 22:33
  • @capser: yes, some programmers like the Machine to tell them what to do. It is a religion I don't subscribe to, I prefer to break the rulez knowingly. Go ahead and do it your way, you'll never be proven wrong. Merely sluggish. – Hans Passant Feb 06 '10 at 22:46
  • 3
    I'll have to agree with @casperOne here. Static analysis is extremely valuable for uncovering defects early on. Tools like FxCop help especially in finding design issues, which are often costly to fix if they are discovered late in a product cycle. Breaking the rules is perfectly fine when the you know what you are doing; that's why annotations like `[SuppressMessage]` exist -- it indicates a conscious "rule-breaking" choice was made rather than simple ignorance of the rule. – bobbymcr Feb 07 '10 at 00:35

1 Answers1

15

Is there an FxCop rule for this? Yes and no.

In FxCop 1.35, which is what Visual Studio 2005 Code Analysis is based on, there was a rule DisposeObjectsBeforeLosingScope which did exactly this.

In FxCop 1.36 (Visual Studio 2008 Code Analysis), they removed their data flow analysis engine, which meant that this rule also had to be removed.

However, in the next FxCop (Visual Studio 2010 Code Analysis), it seems that DisposeObjectsBeforeLosingScope has returned!

bobbymcr
  • 23,769
  • 3
  • 56
  • 67
  • +1. Didn't know it's added to 2010. While I don't necessarily agree with the rule, this is a direct answer to the question. – Mehrdad Afshari Feb 06 '10 at 20:52
  • BTW: On your second link: What does "Returning a disposable object requires that the object is constructed in a try/finally block outside of a using block" mean?? – ulrichb Feb 07 '10 at 13:28
  • 1
    They actually cover this in the sample in that article. Look at the `OpenPort2` method. Basically, if the object is constructed but fails to initialize, it should be `Dispose()`'d before throwing the exception to the user (otherwise it goes out of scope). A `using` would not be appropriate here because the *caller* is expected to call `Dispose()` at a later time. – bobbymcr Feb 07 '10 at 18:58
  • OK. Agree. But couldn't I do the same by using `try-catch` and re-throwing the exception after disposing the serial port? This way I would not need the `tempPort` variable. – ulrichb Feb 07 '10 at 19:59
  • 1
    Yeah, that would probably be about the same. But I think the reason they do it this way is more about convention and consistency. Most people expect to see resource cleanup in a finally block, and it also avoids duplication in case there are multiple catch handlers. There are also subtle differences in how a debugger behaves for catching/rethrowing vs. not catching (see http://blogs.msdn.com/jmstall/archive/2007/02/07/catch-rethrow.aspx ). – bobbymcr Feb 08 '10 at 09:17