[Mono-dev] New Gendarme Rules

[Russell Morris]

Hello all,

I'm beginning to undertake some work on a few new Gendarme rules.  The
attached document outlines ideas for 5 new Gendarme rules I plan on
writing in the next week or two.  I'd appreciate it if some folks could
look over them and provide any suggestions or criticisms.  

Particularly, rules 4 and 5 concern safe usage of IDisposable objects.
I want to be able to catch non-disposal (4) and also potentially
premature disposal (5) scenarios.  However, neither rule should trip
code that either aggregates IDisposable objects as a part of its
lifetime, or participates in a chain of IDisposables (like an
implementation of Stream).

Thanks,

Russ
-------------- next part --------------
Gendarme Rules
==============

1. DontDestroyExceptionStackTrace
  
  In .NET, an Exception object's .StackTrace is a snapshot of the stack 
  information that is available at the time the exception is thrown.
  This can be somewhat of a pain, as rethrowing an existing exception from
  within a catch block can potentially reset its .StackTrace, thus
  obfuscating the original source of the Exception itself.  The following
  code will reset a thrown Exception's stack trace:
  
    try {
      // do some stuff
    } catch(Exception ex) {
      // do some cleanup specific to this exception type, but
      // rethrow the exception because we can't fully recover
      throw ex;
    }
  
    
  When you need to rethrow an exception, you really have two proper means by
  which you can do this without losing information about where the exception
  occurred.
  
  The first way is to use an 'implicit' throw from within the catch block:
  
    try {
      // ...
    } catch(Exception ex) {
      // ... 
      throw;
    }
    
  The second way is to construct a new Exception-derived instance to throw, and
  include the exception you caught as its .InnerException:
  
    try {
      // ...
    } catch(Exception ex) {
      // ...
      throw new MyException("[Error message]", ex);
    }
    
  I think ensuring that Exceptions are properly rethrown (when they need to be) 
  is pretty important to maintainability.  Anyone who's ever had to dive into
  a call graph to find the source of a NullReferenceException that has had its
  stack trace destroyed will agree, I think :)
  
  
2. DontSwallowNonExceptionDerivedExceptions
  
  In the CLR, anything can be thrown as an exception - not just instances of 
  classes that derive from System.Exception.  It's an incredibly unfriendly 
  thing to do, but it can occur when integrating with managed or unmanaged 
  C++ code, or if the underlying system is capable (Win32 can barf up SEH 
  exceptions, for instance).  There may also be other .NET languages that
  allow the throwing of non Exception-derived objects, but I'm not aware of
  them.  You can catch these types of exceptions in C# code in blocks such as 
  the following:
  
  try {
    //...
  } catch {
    // ... will catch anything thrown, whether it's derived from
    // System.Exception or not.
  }
  
  There's no exception-specific work you could do in a catch block like this, 
  because you've got no way of knowing what was thrown, or what it means other
  than 'bad'.  I believe that the majority of these 'naked' catch blocks are
  a result of programmers wanting a catch block for all exceptions, but not
  wanting to do any exception-specific cleanup.
  
  Catching and swallowing non System.Exception-derived exceptions is a bad 
  thing, I believe, because exceptions that nasty should really bubble up to
  the framework (they're not thrown for fun - they indicate something 
  extraordinarily bad has happenned).
  
  Of course, folks writing .NET code in languages like C++ that can happily 
  catch non Exception-derived exceptions should turn this type of rule off.  
  But I would suggest that, in the majority of cases, flagging this 'naked'
  catch block as an error is the appropriate response.
  
  
3. DontBeLazyLogIt
  
  Typically, any time you catch an Exception, it's worth logging as a DEBUG
  mode message.  This rule would inspect the instructions available in the
  catch{} block to ensure that the caught exception was being passed to some
  function call.  I think this would be a relatively unobtrusive and 
  effective way of ensuring that the exception wasn't silently swallowed.
  The following code would trip the rule:
    
    try {
      // ...
    } catch(Exception ex) {
      // silently swallow the exception
    }
    
    try {
      // ...
    } catch(Exception ex) {
      // emit a braindead message that doesn't help anyone
      System.Console.WriteLine("Error occurred...");
    }
    
  The following code blocks would not trip the rule, as the caught exception
  'ex' is either passed as an argument to a function, or is used to initialize
  the arguments passed to a function:
    
    try {
      // ...
    } catch(Exception ex) {
      // Still pretty lazy, but better
      System.Console.WriteLine("Error: " + ex.Message);
    }
    
    try {
      // ...
    } catch(Exception ex) {
      // Much better
      My.Logging.Package.Debug(
        "Error while trying to [blah...]",
        ex);
    }
    
4.  DisposeOfLocalIDisposables

  In general, it's a good idea to .Dispose() ( or .Close() ) on an object
  that implements IDisposable when you're done with it.  However, it's bad
  to be too enthusiastic, as such objects will sit around unusable until the
  GC decides to get ride of them.
  
  This rule will ensure that IDisposable instances that are provably local in
  scope are created in a try{} block, and Dispose()'d or Close()'d in the 
  adjoining finally{} block.

  The following code would trip the rule because it doesn't .Dispose()
  a provably-local IDisposable. (A FileStream is used in this example.  
  System.IO.Stream, unfortunately, explicitly implements 
  IDisposable.Dispose(), meaning that you'd have to explicitly cast to
  IDisposable before calling Dispose().  Stream.Close() is implemented as
  a synonym for .Dispose(), so the rule treats a call to .Close() as
  equivalent to a call to .Dispose()).
    
    FileStream fs = null;
    try {
      fs = new FileStream("/foo/bar.baz");
      //... do stuff
    } finally {
      if(fs != null) {
        fs.Close();
      }
    }

  Note that this rule would NOT be tripped if the newly created IDisposable
  was stored in a static or member variable, or if it was passed to a 
  function or a property setter.

5.  DontDisposeOfNonLocalDisposables
  
  On the flipside, it's usually a bad idea to Dispose() IDisposables if you
  aren't really sure everyone that could have a reference to it is done
  with it.
    
  The following code would trip this rule, because it disposes of
  an IDisposable instance that could potentially be referenced outside of
  this function's scope.
    
    FileStream fs = null;
    try {
      fs = new FileStream("/foo/bar.baz");
      //... do stuff
      this.SomeOtherFunction(fs);
    } finally {
      if(fs != null) {
        fs.Close();
      }
    }
    
  Note that this rule would NOT be tripped if the IDisposable instance 
  whose .Dispose() was called was not created in this function.