We should have every API backed up by an interface, in a non-static way

[ffMathy]

Hi there.

Often when working with PInvoke code, you have a function like this:

public void DoSomething() { MyApi.SomePInvokeCall(); SomeOtherCodeThatIsRelevant(); }

But then you can't unit test DoSomething because it uses a PInvoke API. In other words, it would be an integration test, and if the API in question does something nasty, you would never want it to run.

What if we (from every static class of APIs) backed them up by an interface, which was then injectable through IOC?

Yes, it would require an instance of the class (maybe we could add a Singleton pattern if some people wouldn't ever want to instantiate each API). But it would make huge amounts of code easier to test, and take PInvoke a step further.

So instead of:

public static class SomeExampleApi
    {
        [StructLayout(LayoutKind.Sequential)]
        public struct BITMAP
        {
            public int Type;
            public int Width;
            public int Height;
            public int WidthBytes;
            public ushort Planes;
            public ushort BitsPixel;
            public IntPtr Bits;
        }

        [DllImport("gdi32", CharSet = CharSet.Auto)]
        public static extern int GetObject(
            IntPtr hgdiobj,
            int cbBuffer,
            out BITMAP lpvObject
            );
}

We would have:

private class SomeExampleApi : ISomeExampleApi
    {
        [StructLayout(LayoutKind.Sequential)]
        public struct BITMAP
        {
            public int Type;
            public int Width;
            public int Height;
            public int WidthBytes;
            public ushort Planes;
            public ushort BitsPixel;
            public IntPtr Bits;
        }

        public int GetObject(
            IntPtr hgdiobj,
            int cbBuffer,
            out BITMAP lpvObject
            ) {
               return GetObjectApi(hgdiobj, cbBuffer, out lpvObject);
        }

        [DllImport("gdi32", EntryPoint = "GetObject", CharSet = CharSet.Auto)]
        private static extern int GetObjectApi(
            IntPtr hgdiobj,
            int cbBuffer,
            out BITMAP lpvObject
            );
}

And then the ISomeExampleApi would look like:

public interface ISomeExampleApi {
     public int GetObject(
            IntPtr hgdiobj,
            int cbBuffer,
            out BITMAP lpvObject
            );
}

That way, I could inject an ISomeExampleApi whenever I wanted to use it, and then when testing, fake it out with my favorite mocking framework, and still test other functionality.

[AArnott]

While mockability has some value, I have several concerns with this proposal:

1. I fear that by the time we have a substantial amount of API coverage in the library, that DLL size will be a deterrent to folks actually consuming it via NuGet. For example, if the average consumer only wants 2-3 P/Invoke method signatures, and the Kernel32.dll alone is 1MB in size, they'll see that as a reason against using it, and instead simply copy the code out. While copying the code out is fine, it's not the easiest workflow for them and I'd like to optimize for the easiest case. Since most of this library's size is simply due to metadata (many methods have no implementations at all), with your proposal the metadata size will triple in size (P/Invoke method, interface method and interface implementation).
2. The cost of adding API will go up. Now instead of copying a p/invoke signature from p/invoke and touching it up as necessary, at least three places have to be updated. The touching up of the API (replacing IntPtr with safe handles, deliberating between uint/int, etc.) will also go up sharply because every change has to be replicated in at least 3 places.
3. Xml doc comments will either have to be added to and maintained in 3 places, or they won't be in some places that can be useful at least for internal library development.
4. The using static syntax makes calling these static functions extremely natural feeling. Making these methods callable only via an interface would adversely impact the syntax of the callers. Unless we exposed both APIs (static methods as well as instance methods), and that has a cost in usability as well.

I also wonder just how valuable mocking at the P/Invoke level would really be. How likely is outside caller code into p/invoke methods to be testable just by mocking up these interfaces, and is that really what is going to be the easiest way to test it? Personally, I'd lean toward wrapping P/Invoke method calls in a higher level API which is what I would mock out, if I mocked at all.
Also, I believe there are test libraries out there that can replace static method calls with alternative implementations, which may be worth considering for folks who want to do that.

I think I've mentioned in the guidance doc that this project shouldn't become a high level API for any individual library. It's strictly for P/Invokes, and a few helper methods when the P/Invoke method is so difficult to call that it requires special memory handling, marshaling, etc. would where a strong case can be made that virtually every caller would greatly benefit from a higher level API for the method. In other words, if a library or collection of functions could benefit from just a more managed friendly API with specific use cases in mind, that's a great idea for another library -- not PInvoke. And that other library may certainly bring in PInvoke as a dependency as it adds a higher level API to it. Everyone who needs to P/Invoke will need the P/Invoke signatures. As we add additional features on top of it, we actually filter out potential customers as they look at it and decide this library is too specialized for them and they'll look elsewhere.

I feel that the rise in cost for development, the increase in assembly size, and the (IMO) limited audience that would want to leverage the mockability of the library suggests the cost is higher than the benefit. So my inclination is to Won't Fix this. But I'm open to discussion for a short time to see if you or anyone else wants to chime in.

[ffMathy]

If we used #56 for this, it would be very easy to make this extra level. Furthermore, even for a megabyte NuGet, it isn't bad. 1 megabyte of memory is nothing, and neither is 1 megabyte of diskspace.

To see an example where this really shines, I'll refer to some of the code I wrote recently (I'll find it and get back with a concrete example).

I only think it makes sense if automatic generation is possible.

[AArnott]

I only think it makes sense if automatic generation is possible.

Most of my concerns would be resolved if that were the case. I look forward to what you get back to us with.

[vbfox]

I agree with @AArnott on that one, it doesn't feel like the correct level to mock and losing the using static would be sad.

I'm not concerned so much about library size and having a second "interface + non-static class" version of the API could be a way to have such a "mockable" version (Kinda like Rothko does for IO in the framework)
Preferably it would be in another nuget along the normal one providing a wrapper. As long as it's auto-generated and not hand-crafted it wouldn't be so hard to maintain.

[ffMathy]

Here's an example from a project I am working on.

This file: https://github.com/ffMathy/Shapeshifter/blob/master/src/Shapeshifter.UserInterface.WindowsDesktop/Factories/ClipboardDataControlPackageFactory.cs

Is entirely untestable because of one single call to ClipboardApi.GetClipboardFormats (which is a PInvoke call).

This repeats itself in a lot of scenarios, and I believe we live in a world where testing is a must, and isolation is important. This library has the potential to transform the way we work with PInvoke, not just making it more accessible. And yes, automatic generation is a must.

Here's a proposal in regards to the lack of being able to use using static. What if (in the example above), SomeExampleApi and SomeExampleApi.GetObjectApi were public? That way, you could decide if you wanted fakeable isolation or just accessing the public static API. Heck, we could even allow the "isolation" one to be a separate NuGet. For instance, PInvoke.BCrypt.Isolated or something.

[AArnott]

Yes, automatic generation and still exposing the static API would make me feel better.
Whether separate NuGet package or built-in may be more negotiable. I might call it PInvoke.LIBNAME.Mockable instead though to make it more clear what scenario folks would use it fore.

[ffMathy]

True. The naming is details though.

[ffMathy]

The first example of this generator is here: #70

[AArnott]

Another consideration with interfaces is that they are not immutable, by nature, in this project. Because every new API we add coverage for will add a member to this interface, it is technically a breaking change.
Even without that consideration, these interfaces may easily reach thousands of members for some libraries.
So the only way anyone would sanely use these interfaces for mocking (IMO) would be to use a mocking library that generates code dynamically so I only have to implement a few methods that I know are used in the code path I'm about to test. I think one of these can "fix" static methods already. But I think most rely on implementing interfaces, so while I see value here, I think we need to be sure we communicate that these interfaces are for automated mocking only and not for manual implementation to avoid breaking with upgrades.

[ffMathy]

That is why we auto generate the members. Also, are you really concerned that somebody will implement the interface themselves? Why would anyone do that?

And no, static methods can't be faked.

[AArnott]

Also, are you really concerned that somebody will implement the interface themselves? Why would anyone do that?

My point was that they wouldn't and shouldn't. If we define these interfaces, it's under the goal that folks will only find them useful when mocking using a tool that generates code. But nevertheless it is a public API that incurs breaking changes with most releases and thereby breaks semver rules to simply add an interface with a minor version update -- so I feel better if we make it very clearly documented that these interfaces are not meant to be implemented except by such tools.

And no, static methods can't be faked.

Yes they can. I know of at least two test frameworks that can do this, including shims as supported by Microsoft Fakes.

[ffMathy]

Ah yes. I forgot about shims. But they alter the assembly they fake and that's how they get away with it. Not sure how other frameworks would do it.

Most people I know use Moq or NSubstitute.

[AArnott]

I've never used Fakes, but from what I've seen partner teams' experience is, I'd rather avoid it, myself. The best library that supports mocking up statics is a MS internal one (done by a lone dev). Most folks I know are like yours: Moq.

[ffMathy]

Hmmm actually I just thought of something. You want to use static classes because then you can use the "using static" operation.

Why, actually? Isn't IOC more common than using static methods? We are evolving into an IOC era.

[AArnott]

Yes, I've noticed the trend. But I don't think that P/Invoke methods are the best place to 'inject'. They're so low level, it seems more helpful in the scenarios I'm thinking of that components that P/Invoke are themselves the injected and mocked components.
Consider GDI32 for example: I think folks would rather mock up their simpler windowing abstraction than hundreds of individual p/invoke methods. Therefore static methods for p/invoke themselves is adequate.
If this library didn't exist, folks would have static p/invoke methods (because that's what's required). This project's first responsibility is to remove the need for folks to define those static p/invoke methods out of convenience. If those same folks (before they used the library) wanted to use IOC they already would have done it by wrapping code that calls those static methods in something injectable. This library comes in and replaces those static methods, but folks who have already invested in such code may want to keep their existing architecture in place.
Now, folks who haven't yet done any IOC conversions in their code and see adopting this project as a way to not only remove p/invoke methods but also get IoC support "for free" may see value in this proposal, I admit. But those same people may soon decide that they'd rather mock up at a higher level. Personally, that's what I would probably do.

[AArnott]

We haven't had anyone else request this. So closing.

[AArnott]

Incidentally, a means of doing p/invoke via an interface came to my attention this morning, so I thought I'd share. Unfortunately its license is more restrictive than our own so it's not even really an option, but we could leverage our build time code generation to do the same thing that they're doing with RefEmit at runtime and thus claim the "modern" benefits and (questionable) perf boosts but without the startup time perf hit.

http://sharkman.asuscomm.com/blog/an-introduction-to-adl/

[ffMathy]

Now that's worth considering! Nice find. Re open the issue then?

[AArnott]

I'm considering it. If we do, I think it would only be to modify our code gen to produce interfaces and implement them (or maybe generate classes with virtual methods?). I don't think we could pick up ADL itself though, for these reasons:

1. It's LGPL licensed (ours is MIT)
2. calli doesn't work in 32-bit processes (apparently)
3. Its use of delegates is far slower by their own metrics, particularly on mono.
4. It's not actually much faster than DllImport anyway (significant improvement seen only on .NET Core)
5. It has a startup time perf hit for RefEmit.

[vatsan-madhavan]

Since partial trust is deprecated but supported on the desktop .NET framework, would the plan be to continue supporting a mode that keeps working in partial trust scenarios on .NET 3.5 and .NET 4.x?

[AArnott]

Partial trust is not a concern at all here. We're P/Invoking after all, which requires full trust anyway.

[AArnott]

(The ADL intro blog I linked to above mentions that calli isn't verifiable and therefore doesn't work in partial trust, but fails to consider that partial trust already is out with [DllImport])