P/Invoke Properly, Please!

A managed runtime is a wonderful thing -- exception handling is a nice luxury, although some die-hard system developers out there believe it hides as much complexity as it removes.  (I tend to agree, although I think exception handling is still a useful tool, if applied properly.)

Exception handling also tends to lull one into a false sense of complacency.  If it was easy to neglect Win32 functions' return values in the old days, it's downright effortless to forget about them, today.  "Hey, if something bad happens, an exception will be thrown, right?"  No, not from p/invoke!

The interop layer in .NET knows how to marshal a very impressive set of intrinsic types, arrays, and whatnot, to and from unmanaged code.  But, beyond basic marshalling, it doesn't give any special treatment to Win32 return values -- it can't, because (unfortunately) the semantics of Win32 return values vary greatly from function to function.  (Sometimes NULL is good, sometimes NULL is bad.)  So, the burden remains with us...

And it looks a lot like we're failing to carry that burden!  Folks apparently seem to think that a managed runtime is a license to completely ignore Win32 errors.  I'm talking about the DllImport attribute's SetLastError field -- and it's utter lack of usage, out here in the trenches.  [Update: a quick browse through the PInvoke.net web site -- a newly founded (but long awaited!) wiki library of p/invoke signatures -- shows how scarce the usage of SetLastError really is.]

According to SpellWeb, the ratio of hits for "DllImport" vs "DllImport SetLastError true" is about 14 to 1.  And yet.  Surely more than 1 in 14 Win32 API functions have error return-paths. (In my experience, the true ratio should probably be closer to 14 to 12, not 14 to 1.)  IMHO, SetLastError should be 'true', by default.  Alas, it is not.

Here's the good news:  if you do SetLastError=true, and you do remember to actually check the function's return value, the default constructor for Win32Exception makes it very easy to throw a managed exception in response to a Win32 error... you typically need only write one conditional statement, to perform the mapping.  It could hardly be any easier!

Here's the canonical example of good Win32 P/Invoke form that I give to my dev team, to study and emulate.  I'll gladly offer it up here, for public review...

using System;
using System.ComponentModel;
using System.Runtime.InteropServices;

internal sealed class Win32
{
  private Win32() { }

  //
  // Error-safe wrappers

  public static void SetSystemTime(DateTime dt)
  {
    // Marshal the DateTime arg into a SystemTime structure.
    SystemTime st;
    st.wYear = (ushort)dt.Year;
    st.wMonth = (ushort)dt.Month;
    st.wDayOfWeek = (ushort)dt.DayOfWeek;
    st.wDay = (ushort)dt.Day;
    st.wHour = (ushort)dt.Hour;
    st.wMinute = (ushort)dt.Minute;
    st.wSecond = (ushort)dt.Second;
    st.wMilliseconds = (ushort)dt.Millisecond;

    // Make the call; throw on failure.
    if (!Kernel32.SetSystemTime(ref st))
      throw new Win32Exception();
  }

  //
  // Raw (unchecked) Win32 API declarations
  
  //[System.Security.SuppressUnmanagedCodeSecurity]
  private sealed class Kernel32
  {
    private Kernel32() { }

    [ DllImport("Kernel32.dll", SetLastError=true) ]
    public static extern bool SetSystemTime(ref SystemTime systemTime);
  }

  //
  // Supporting types and constants

  [StructLayout(LayoutKind.Sequential)]
    private struct SystemTime // from WinBase.h(~279)
  {
    public ushort wYear;
    public ushort wMonth;
    public ushort wDayOfWeek;
    public ushort wDay;
    public ushort wHour;
    public ushort wMinute;
    public ushort wSecond;
    public ushort wMilliseconds;
  }
}

In short, my P/Invoke guidelines say this: declare all raw Win32 methods as private methods -- on nested classes if necessary, to avoid namespace collisions -- encapsulation is your friend.  Use SetLastError=true to track the Win32 error code, if the function's documentation suggests that it updates the thread's Win32 error code upon failure; use SetLastError=false, otherwise, to explicitly indicate functions that don't fail.

Then, wrap those private Win32 calls with a few lines of public, managed code to [a] perform any special translation from managed types to their unmanaged counterparts (if no default marshalling is available, or appropriate -- as in the case of DateTime to SystemTime, in the above example) and [b] look out for error-returns and throw exceptions (or retry, or assert, or whatever's appropriate) in response.

I'd really like to see an FxCop rule for this: perhaps a lookup table of Win32 functions which have failure modes, and a rule to verify that they're invoked w/ SetLastError=true.  Comments?