Automatic generation of mockable classes and interfaces

[ffMathy]

@AArnott what do you think? Code isn't pretty right now, but it gets the job done. Can you try it out and let me know your thoughts?

[ffMathy]

And @vbfox what do you think?

[vbfox]

I think that you should generate elastic trivia and run the Formatter on the generated doc

[ffMathy]

What do you mean? This has nothing to do with the documentation. Actually, the documentation is a trivia of the [DllImport] attribute of each function.

[vbfox]

I was speaking of whitespace trivia, not the documentation one.

The current code generate specific whitespace trivia (NewLike, TabCharacter) but if you use elastic trivia and run the Formatter it will generate the correct indentation code.

In essense when you generate code, it's better to avoid doing indentation & most whitespace handling by hand, the Formatter class is there for that.

It's only problem is that it depends on a configuration that isn't layered (As R# ones is) so it isn't saved in a commitable file and it's not extendable so the generated files might be fixed in a way stylecop doesn't like.
The first can be solved by changing the configuration of the workspace and the second one can be solved by running the solution-wide code fixe of any stylecop analyzer that disagree after the generation.

[ffMathy]

Ah nice! ReSharper also offers the free InspectCode.exe now. Not sure if it can actually bulk fix files, but that could be relevant as a build step.

[vbfox]

The big avantage regarding generation & the Formatter is that it run only on nodes tagged via WithAdditionalAnnotation(Formatter.Annotation) so you can be selective and even if you disagree on some generic things (like how to indent case break) you can tag what you want him to touch.

I don't think that R# got a way to run bulk fixes without being in visual studio with the full R# loaded. Roslyn is more re-usable in that sense.

[vbfox]

Generating code like that seem very nice, 👍

Except from the fact of not having so much generated code in the repo that @AArnott already raised I have a few general remarks :

• I don't like the Invoke prefix so much and would prefer for the methods to have their original name.
• The interface shouldn't contain Mockable, so IKernel32 for example
• I think that we are missing a static instance of the Mockable class on the main one to be able to have Kernel32.Instance that provide us with an IKernel32 instance.
• No need for the Mockable class to be public, it's not something that we wan in our API surface it's internal plumbing to provide an IKernel32 instance.

The result will be an usage that look like this :

SafeObjectHandle Foo(IKernel32 kernel32)
{
    return kernel32.CreateProcess(...);
}

...

var process = Foo(Kernel32.Instance);
...

---

One thing that is completly forgoten here is the helpers we provide. They shouldn't be second class citizens and should be available on the interfaces.

One solution to do that is to always write the helpers in term of interfaces (Extension method on the current project interface taking the other interfaces as parameters and have the generation code generate the static version for us)

        public static PROCESSENTRY32 Process32First(this IKernel32 kernel32, SafeObjectHandle hSnapshot)
        {
            if (hSnapshot == null)
            {
                throw new ArgumentNullException(nameof(hSnapshot));
            }

            var entry = new PROCESSENTRY32();
            if (kernel32.Process32First(hSnapshot, entry))
            {
                return entry;
            }

            var lastError = kernel32.GetLastError();
            if (lastError != Win32ErrorCode.ERROR_NO_MORE_FILES)
            {
                throw new Win32Exception(lastError);
            }

            return null;
        }

That will produce a generated code like :

public static PROCESSENTRY32 Process32First(SafeObjectHandle hSnapshot)
    => Process32First(Kernel32.Instance, hSnapshot);

---

One of the problem of all that generation is that the generation of the cs files will be needed before compiling & doing it during the build would be slow (Roslyn is fast when already running but startup time in a new process is costly).

But contrary to the "exports" generation it will be needed to be run by contributors, idealy before even opening the solution.

So I think that we start to see the need of some build script to have some build command at the root doing generation/build/test that can be parametrized like build generatedFiles. It can be either done by delegating to msbuild (batch / powershell calling msbuild) or via FAKE, Cake or PSake (I use FAKE everyday but it's the only one I know, still I head good things about Cake)

[ffMathy]

I made it so that all classes, interfaces and methods in the files now have the CompilerGenerated attribute on them. We could then go in and simply ignore ReSharper inspections for those files, and leave them be.

[ffMathy]

I removed the Invoke prefix on methods. They are remnants from when I had one class that was non-static, with public static extern PInvoke calls and the Mockables side by side.

[ffMathy]

@vbfox if I generate the Singletons in Roslyn, should we fix the code manually to use these Singletons, or did you expect that to be fixed in the Roslyn code as well?

[ffMathy]

Also, while Singletons are nice, would it make sense to also make Mockables out of the helpers? That would solve the issues easily.

[vbfox]

We could then go in and simply ignore ReSharper inspections for those files, and leave them be.

For R# it respect the <auto-generated> If I remember well, it also ignore the .Generated.cs file extension by default, it might not be a bad idea to have both.

Also, while Singletons are nice, would it make sense to also make Mockables out of the helpers? That would solve the issues easily.

It's possible but a lot more complex, think of helpers calling other helpers, it's far from a trivial transformation and make it so that all helpers code is in 2 places in the generated binary i'm not super-fan.

But I agree that it's a change that will affect how we all code helpers so @AArnott might want to chime in and might have a different point of view.

I generate the Singletons in Roslyn, should we fix the code manually to use these Singletons

The Helpers should be fixed by hand if we go that way, for the test code no need to change let it call the generated static versions we don't need to test auto-generated code (But might need to test the generator)

[ffMathy]

We should maybe also take a look at #71 here.

[ffMathy]

But @vbfox, it is pretty simple. Remember that all we want are the methods to be fakeable. The helpers don't have to call other helpers. They can just call the static methods. It'll be fine, because the original call can still be faked out.

[ffMathy]

Also, how do I fix the conflicts in this branch? I am not a Git ninja (I typically use GUI driven tools for it).

[ffMathy]

Also, we could have this all as part of our pre-build step, right? And I think the .generated.cs idea is brilliant. Should I work on renaming files in that way instead?

[vbfox]

But @vbfox, it is pretty simple. Remember that all we want are the methods to be fakeable. The helpers don't have to call other helpers. They can just call the static methods. It'll be fine, because the original call can still be faked out.

Problem is that helpers are calling other helpers and have to do for complex ones.

I don't think that we want the helpers to be fakeable, They should just take the interface as parameter.

A method like :

void MyMethod(IKernel32 kernel32)
{
    var array = // Array allocation
    fixed(/* the array*/)
    {
        kernel32.Foo(...);
    }
    return array;
}

That is testable should be rewritable to :

void MyMethod(IKernel32 kernel32)
{
    return Kernel32Helpers.Foo(kernel32);
}

Without any behavior change (same methods called on the mocked interface)

We should maybe also take a look at #71 here.

I'll chime on the issue I think that it should be a separate class but all in the same nuget.

Also, we could have this all as part of our pre-build step, right?

Possible but slow if always done (Like the export generation was at the start), don't really want to have all builds in VS take 3x time longer due to generation.

[ffMathy]

But what value does it give injecting IKernel32 for instance? I don't see the apparent advantage.

I do however see the advantage of faking an entire method out, but that is still possible, without injecting other APIs in through the constructor.

Can you elaborate again why you believe it adds value to do it like this?

[ffMathy]

Should we get this merged in, and then developed further? I won't have much time the next couple of weeks.

[AArnott]

As soon as we have general agreement that it's a good direction, I'm OK to merge it even if we have more work to do on it (provided it doesn't regress overall quality of the project).
It's been long enough that I personally need to re-review this to freshen my memory. We should probably solicit approval/disapproval votes from @vbfox and @jmelosegui as well.

[AArnott]

Also to consider, once we add this, it's a breaking change to remove. I wouldn't mind shipping the first stable version just as P/Invokes and then added this when more people request it.
Shipping v1 as "replace all your p/invoke signatures" is a simple story that people can immediately grasp.
Shipping v2 with "new: mockability" is clear to your existing customers.

But shipping both features at once in v1 not only increases our initial burden of support, but can make it a little confusing just what the library is. It could help and hurt adoption, and as I don't know which will dominate the other, it's something of a risk for a v1.

[AArnott]

But what we can do for now if you'd like to get this ready to merge is get the generated code out of source -- make it a build artifact. That's a double-bonus because then we won't have merge conflicts while it waits to be merged since not much (if any) source code will actually be changed by the PR.
Thoughts?

[vbfox]

I agree that both #76 and #55 might be too much for a v1, the "manually created static dllimport" story add enough value to be released independently without waiting for the codegen to be there.

Also it's important to release early / release often. There might be biases in the current API that we don't see as we are only a few to hack on it and that we will see when it start to be used externally.

But it still is an interesting thing and merging the generation code could be a start. We need to find a workflow where we are happy to use it and where it both add value and isn't too much in our way.

Maybe we should start labelling what we want for a "v1.0" (Removing prerelase in NuGet) and what would be for a "v2.0"

[AArnott]

I like that idea.
I'll take some brainstorming on milestones to gitter.im

[AArnott]

I've created a "2.0" milestone and put #55 into it.
Just so we don't have a PR lingering open when we're not merging it soon, I'll close this for now. I do still think the feature is interesting and we should definitely look into this after we get the rest of the API stable and a stable release published and some more active users providing feedback.
Thanks so much for the research and effort put into this already.