Roslyn analyzer/code fixer for use of Marshal.* functions

[stevenbrix]

When using C#/WinRT, how developers do COM interop has changed a bit.

Before:

Guid guid = new Guid(ISurfaceImageSourceNative);
var imageSource = new MyImageSource();
IntPtr pointer = Marshal.GetIUnknownForObject(imageSource);
IntPtr pOut;
Marshal.QueryInterface(pointer, ref guid, out pOut);
var nativeInterface = (ISurfaceImageSourceNative)Marshal.GetObjectForIUnknown(pOut);
nativeInterface = null;
Marshal.Release(pOut);

Now:

var imageSource = new MyImageSource();
var nativeInterface = imageSource.As<ISurfaceImageSourceNative>();

The new code is so much prettier! But it will be a stumbling block for developers who are used to the old pattern. I think it should be possible to detect usage of Marshal.* APIs with a Roslyn analyzer, and hopefully a code-fixer as well. This could be added to https://github.com/dotnet/try-convert/issues/308

Thoughts?

Edit: Here's the definition of ISurfaceImageSourceNative

[ComImport, Guid(MainWindow.ISurfaceImageSourceNative), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
public interface ISurfaceImageSourceNative {
}

[Scottj1s]

This recommendation is limited to interop interfaces with the ComImport attribute. The As<>() function is basically equivalent to the marshal code above. CsWinRT could add an explicit AsComInterface<> that ignores the ComImport attribute, to hide use of Marshal.GetObjectForIUnknown() so that an analyzer could flag all other calls.

[stevenbrix]

Oh sorry, let me update the issue with the definitition of ISurfaceImageSourceNative

[ComImport, Guid(MainWindow.ISurfaceImageSourceNative), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
public interface ISurfaceImageSourceNative {
}

[Scottj1s]

Thx - there's nothing that requires the ComImport attribute. I used it for consistency with old CLR behavior. We could add AsComInterface<> if users just wanted to define a COM interface explicitly. That already has to be done halfway for IInspectable COM imports, due to a CLR bug (see the unit test case).

[stevenbrix]

Gotcha, so with the way it currently works, it requires ComImport? I think that sounds ok to me for now, unless someone comes asking for it, I wouldn't change it