Add an analyzer to catch inadvertent ephemeral delegates

[asklar]

There are several Win32 APIs that take callbacks, e.g. WNDPROCs or DLGPROCs. These are currently projected as delegates, and if you do the "natural thing" and assign static methods to fields of these *PROC types, the runtime will create a temporary delegate, which gets garbage collected from under you, and you will start getting ExecutionEngineExceptions at random. These are hard to debug (they don't hint at what went wrong), so they constitute a dangerous sharp edge.

cswin32 should either have a build-time warning, or an analyzer, that validates that you are not falling into the trap of doing the natural-yet-unsafe way of assigning these callbacks.

An alternative is to change the projection of structs taking callbacks to always use function pointers instead of delegates.

See https://github.com/microsoft/CsWin32/issues/623#issuecomment-2230824850

[AArnott]

Thanks for filing this. This has come up from two others in the last week besides you, yet before that I hardly heard about it at all. Funny how perfect storms come up like that.

I am curious about your failure. In studying code with others, it seems that modern C# compilers actually store a delegate over a static method in a static field just as an optimization, but with the nice side effect of avoiding the very problem you're seeing. Can you confirm by looking at the compiled IL whether a field for the delegate isn't implicitly created?

Here is a sample of the delegate being cached.

[asklar]

looking through the IL dump, I see that it just creates an instance of WNDPROC which is a delegate, no field is created for the delegate.

WNDCLASSEXW param = new WNDCLASSEXW
{
    cbSize = (uint)Marshal.SizeOf<WNDCLASSEXW>(),
    hInstance = PInvoke.GetModuleHandle((PCWSTR)null),
    lpszClassName = ptr,
    lpfnWndProc = WndProc
};

IL_004c: ldftn valuetype Windows.Win32.Foundation.LRESULT [redacted]::WndProc(valuetype Windows.Win32.Foundation.HWND, uint32, valuetype Windows.Win32.Foundation.WPARAM, valuetype Windows.Win32.Foundation.LPARAM)
IL_0052: newobj instance void Windows.Win32.UI.WindowsAndMessaging.WNDPROC::.ctor(object, native int)
IL_0057: stfld class Windows.Win32.UI.WindowsAndMessaging.WNDPROC Windows.Win32.UI.WindowsAndMessaging.WNDCLASSEXW::lpfnWndProc

[AArnott]

I see the same. Switching from method group to lambda syntax triggers the C# compiler optimization that I expected to see here.

Implicit delegate creation is still not a problem so long as the p/invoke doesn't store the function pointer beyond its own lifetime. In your particular case of storing it in a field, that is the problematic storage, by definition because the struct is used for interop rather than permanent storage. So ironically the analyzer would be noticing you store a new delegate into a field, but that the field is on an interop struct, and I guess the message would advise that you also store the delegate elsewhere and then assign from there. It wouldn't catch every use of an implicit delegate (since they can appear as direct arguments to methods), and some would be false positives. So it's not going to have a high hit rate, which makes me hesitant to write it in the first place.

But we can leave the issue active to track the design thoughts on this, in case I or other have a chance to write the analyzer.

[AArnott]

We could potentially trigger the analyzer when the ephemeral delegate is used to initialize an argument or struct field that is annotated with the [Retained] attribute (which is described at https://github.com/microsoft/win32metadata/issues/1726). We'd need to work with the win32metadata repo to get the appropriate places attributed.