The danger of over simplification: Enum.IsDefined()

I just
stumbled across this guideline in the Design Guidelines document and I thought
it needed more explanation.. "urn:schemas-microsoft-com:office:office" />

Do
argument validation. Do not assume
enum
arguments will be in the defined range. It is legal to cast any integer value
into an enum even if the value is not defined in the enum.

public void
PickColor(Color color) {

   switch
(color) {

         
case
Red:

                
...

                
break;

         
case
Blue:

                
...

                
break;

         
case
Green:

                
...

                
break;

         
//repeat
for all known values of Color

         
default:

                
throw
new ArgumentOutOfRangeException();

                
break;

   
}

    //do
work
}

Note: do not use Enum.IsDefined() for these checks as it
is based on the runtime type of the enum which can change out of sync with this
code.

There are really two problems with
Enum.IsDefined(). First it loads
reflection and a bunch of cold type metadata making it a deceptively expensive
call.

Secondly, as the note alludes to
there is a versioning issue here.
Consider an alternate way to write the method defined
above:

public void PickColor(Color color)
{

   if (!Enum.IsDefined
(typeof(Color), color) {
throw new
InvalidEnumArgumentException("color", (int) color,
typeof(Color));

   }

   //Security issue: never pass a
negative color value

   NativeMethods.SetImageColor
(color, byte[] image);

}

Callsite:

Foo.PickColor
((Color) -1); //throws
InvalidEnumArgumentException()

This
looks pretty good, even if you know this (mythical) native API has a buffer
overrun if you pass a color value that is negative. You know this because you know the
enum only defined postive values and you are sure that any value passed was one
of the ones defined in the enum right?
Well, only ½ right. You
don’t know what values are defined in the enum. Checking at the moment you write this
code is not good enough because IsDefined takes the value of the enum at
runtime. So if later someone added
a new value (say Ultraviolate = -1) to the enum IsDefined will start allowing
the value -1 one through. This is
true whether the enum is defined in the same assembly as the method or another
assembly.

public
Color {
Red = 1,
Green = 2,
Blue = 3,
Ultraviolate = -1, //new value
added this version
}

Now, that same callsite no longer
throws.

Callsite:

Foo.PickColor
((Color) -1); //causes a buffer overrun in
NativeMethods.SetImageColor()

The moral of the story is also two
fold. First be very careful when
you use Enum.IsDefined in your code.
The Second is when you design an API to simplify a situation, but sure
the fix isn’t worse than the current problem.

Comments

• Anonymous
  November 29, 2003
  I wasn't too happy to discover, by using a profiler, that Enum.IsDefined is taking a significant fraction of CPU in our app that uses System.Drawing. There are some checks in there, and there is nothing we can do to get rid of them.
• Anonymous
  December 01, 2003
  The comment has been removed
• Anonymous
  December 01, 2003
  Therefore would it be safe to say one should simply not be using Enum.IsDefined for anything at all? If there is no good reason for it, why does it exist? WIll it be deprecated in a future release?
• Anonymous
  December 16, 2003
  Why not make Enum.IsDefined inlined at compiletime like a #define -- that's essentially what you're asking us to do in our verification code. If there's a standard thing that we should be doing, there should be a compiler/tool way to make it automated.I guess there's some value to making people look at it, but I think it's less of a benefit than making IsDefined (or some IsDefinedAtLastCompile equivalent) easier to do (and therefore more likely to be done). IMHO, there would probably be more errors by people's manual maintenance of this validation code than by not having to handedit the values and missing the assumptions broken by the introduction of new values.
• Anonymous
  February 20, 2004
  For my own reference, I thought I'd compile a quick list of design guidelines added by Brad Abrams, et al.
• Anonymous
  May 25, 2005
  Geoff wrote a whole bunch of code for dealing with Enums. Enums are annoying because you can cast any...
• Anonymous
  March 13, 2006
  re: C# Tips: Enumなプロパティのsetterの冒頭は
• Anonymous
  December 10, 2007
  PingBack from http://xidey.wordpress.com/2007/12/10/c-enumparse-bug/