The danger of over simplification: Enum.IsDefined()


style="MARGIN: 3pt 0in; TEXT-INDENT: 0in; mso-list: none; tab-stops: .25in"> face=Arial size=2>I just
stumbled across this guideline in the Design Guidelines document and I thought
it needed more explanation.. "urn:schemas-microsoft-com:office:office" />


style="MARGIN: 3pt 0in; TEXT-INDENT: 0in; mso-list: none; tab-stops: .25in"> face=Arial size=2> style="FONT-SIZE: 10pt; FONT-FAMILY: Arial"> 


style="MARGIN: 3pt 0in 3pt 0.25in; TEXT-INDENT: 0in; mso-list: none; tab-stops: .25in"> face=Verdana>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.


name=OLE_LINK64> style="FONT-SIZE: 9pt">public void
PickColor(Color color) {


style="MARGIN: 0in 0in 0pt 0.25in; mso-add-space: auto; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 1">   style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">switch
(color) {


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 2">         
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">case
Red:


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">…


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">break;


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 2">         
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">case
Blue:


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">…


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">break;


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 2">         
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">case
Green:


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">…


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">break;


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 2">         
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">//repeat
for all known values of Color


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 2">         
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">default:


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">throw
new ArgumentOutOfRangeException();


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'"> style="mso-tab-count: 3">                
face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">break;


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">   
}


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">    //do
work
}


style="MARGIN: auto 0in auto 0.25in; mso-add-space: auto"> style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 7.5pt; FONT-FAMILY: 'Courier New'"> 


style="mso-bookmark: OLE_LINK64"> style="FONT-SIZE: 10pt">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. 


size=2> 


style="FONT-SIZE: 10pt; FONT-FAMILY: Arial">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. 


style="FONT-SIZE: 10pt; FONT-FAMILY: Arial">Secondly, as the note alludes to
there is a versioning issue here. 
Consider an alternate way to write the method defined
above:


face="Courier New" size=1> style="mso-bidi-font-size: 10.0pt">public void PickColor(Color color)
{


style="MARGIN: 0in 0in 0pt 0.25in; mso-add-space: auto; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">   if (!Enum.IsDefined
(typeof(Color), color) {
style="mso-spacerun: yes">       throw new
InvalidEnumArgumentException(“color”, (int) color,
typeof(Color));


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">   }


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">   //Security issue: never pass a
negative color value


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt"> style="mso-spacerun: yes">   NativeMethods.SetImageColor
(color, byte[] image);


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">}


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 7.5pt; FONT-FAMILY: 'Courier New'"> 


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">Callsite:


style="MARGIN: auto auto auto 0.25in; mso-add-space: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">Foo.PickColor
((Color) -1); //throws
InvalidEnumArgumentException()


style="MARGIN: 0in 0in 0pt; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face="Courier New" size=1> style="FONT-SIZE: 7.5pt; FONT-FAMILY: 'Courier New'"> 


style="MARGIN: 0in 0in 0pt; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face=Arial size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: Arial; mso-bidi-font-size: 10.0pt">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. style="mso-spacerun: yes">   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. style="mso-spacerun: yes">  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. 


style="MARGIN: 0in 0in 0pt; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face="Courier New" size=1> style="FONT-SIZE: 7.5pt; FONT-FAMILY: 'Courier New'"> 


style="MARGIN: 0in 0in 0pt; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">public
Color {
   Red = 1,
style="mso-spacerun: yes">   Green = 2,
style="mso-spacerun: yes">   Blue = 3,
style="mso-spacerun: yes">   Ultraviolate = -1, //new value
added this version
}


style="FONT-SIZE: 10pt; FONT-FAMILY: Arial">Now, that same callsite no longer
throws.


face="Courier New" size=1> style="mso-bidi-font-size: 10.0pt">Callsite:


style="MARGIN: 0in 0in 0pt 0.25in; mso-add-space: auto; mso-margin-top-alt: auto; mso-margin-bottom-alt: auto"> face="Courier New" size=1> style="FONT-SIZE: 9pt; FONT-FAMILY: 'Courier New'; mso-bidi-font-size: 10.0pt">Foo.PickColor
((Color) -1); //causes a buffer overrun in
NativeMethods.SetImageColor()


size=2> 


style="FONT-SIZE: 10pt; FONT-FAMILY: Arial">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 (15)

  1. Frank Hileman says:

    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.

  2. Ian Griffiths says:

    Even worse, Enum.IsDefined ignores the [Flags] attribute. Given the following:

    [Flags]
    enum Foo
    {
    X = 1,
    Y = 2
    }

    the value Foo.X | Foo.Y is allowed. But Enum.IsDefined indicates that it’s not a valid value!

  3. sharpie says:

    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?

  4. James says:

    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.

  5. Ken Brubaker says:

    For my own reference, I thought I’d compile a quick list of design guidelines added by Brad Abrams, et al.

  6. Geoff wrote a whole bunch of code for dealing with Enums. Enums are annoying because you can cast any…

  7. 菊池 Blog says:

    re: C# Tips: Enumなプロパティのsetterの冒頭は