Support for default interface implementations

[stakx]

@kzu you asked me in #109 (comment) to open an issue regarding default interface implementations. I'm taking your word for them not currently being supported by this library.

At the IL level, there's nothing too surprising about them: they simply aren't marked abstract as interface methods usually are; they can have a method body like any other method. AFAIK interface methods are also no longer guaranteed to be declared public. Interfaces still cannot contain any instance fields, though IIRC they can now have static ones. Oh, and all of this is only supported starting with netcoreapp3.0 / netstandard2.1 runtimes.

At the reflection level, you'll notice that default implementations remain in the interface they're defined in. That is, a class implementing an interface with default impls doesn't inherit that implementation, you'll have to go look for it directly in the interface. Generally speaking, default impl integration with Reflection isn't terribly well done, there are some gotchas with GetInterfaceMap and detecting overrides etc. I could probably say more about this but will stop here for brevity.

At the C# language level, when implementing an interface, you can implement a method that has a default impl in the interface... but you don't have to. If you do choose to implement the method, then want to call the base implementation, you cannot just do base.Blah(), instead of base. you need to cast the this pointer to the interface with the desired default impl. I suppose the language designers chose this path to solve diamond inheritance issues where base might be ambiguous. They also came up with the concept of a "most specific override" rule.

If it's any help, I've recently added support for default interface impls to Moq 4 (see devlooped/moq#1130) — it works via CallBase, which will call the most specific override. Because DynamicProxy doesn't yet support them properly and Reflection also isn't terribly useful, I had to do a workaround using IL generation... it's not pretty, but may give you some insight into how those things work.

I hope your path using Roslyn will be a little less rocky. :-)

P.S. I should mention that instead of .CallBase() always picking the most specific override, one could consider a new .CallBase<TInterfaceWithImpl>() to allow user code to choose which base implementation should be called (as there could be several!). Meaning, there can be more than just one "target instance".

[kzu]

Ok, this seems like it would be a bit of a show-stopper on the roslyn side: dotnet/roslyn#51560 😞

since I'm relying on the code fix to automatically implement all members. So, presently, generated avatars do not allow interception of interface members with default implementations, unless you manually provide that in a partial class...

[kzu]

Ok, I think I have a possible way to implement this from the codegen aspect:

We could generate an additional interface implementation that would (by "virtue" of the above "show-stopper") not include the default implementation members, which would be used to call those without resorting to reflection.

The actual avatar would need to implement all members, and the ones with default implementations would be implemented explicitly (this allows us to invoke the interface-specific default in this case), and use the "default" interface implementation singleton to get the default behavior. Pseudo-code:

    public interface IFoo
    {
        void Do();
        int Value => 5;
    }

    public class IFooAvatar : IFoo
    {
        public void Do() => pipeline.Invoke(...);

        int IDefault.Value => pipeline.Invoke(..., implementation = DefaultFoo.Instance.Value);
    }

    public class DefaultFoo : IFoo
    {
        public static IFoo Instance { get; } = new Default();

        public void Do() => throw new NotImplementedException();
    }

    public class Test
    {
        [Fact]
        public void Run()
        {
            IFoo foo = new IFooAvatar();

            Assert.Equal(5, foo.Value);
        }
    }

[stakx]

I do agree with Sam Harwell over in the Roslyn issue that the code fix's behavior is what most users will reasonably expect... after all, why would you want a code fix that throws a NotImplementedException when there is a implementation? A code fix that includes even those members with default implementations probably won't be useful to 99% of all users (unfortunately you're in the 1% here) so I don't expect they're going to add it...

That being said, I'm afraid I can't comment on the above proposal because I don't quite understand why the standard code fix is relevant to begin with — doesn't it generate the wrong code (throw new NotImplementedException(), when you really need code that forwards to the interception pipeline), and aren't you using a code fix / generator of your own making because of that...?

[kzu]

Fair points :). Let me elaborate on how things work right now from the codegen perspective:

I invoke the built-in code fix to generate the boilerplate members (initial scaffold) for all interfaces/virtual/abstract members. This ensures that the code I provide is exactly the same you'd get if you had auto-implemented the interface yourself. Subsequently, I have a pipeline of codegen extensions that patch it up and rewrite the syntax with the proper calls, adds fields and so on. This pipeline is what Moq extends on top of what Avatar provides to add its own IMocked interface (and implementation) without having to rewrite any of the Avatar built-in codegen.

I did it this way for two reasons: I didn't want to reimplement the logic on what's overridable and how myself (i.e. members with the same signature from two interfaces will not generate two explicitly implemented members by default when you use the code fix manually in the IDE), and I also wanted to be 100% equivalent with what a manual avatar would look like (so it's easier to understand how things work, less surprises).

Granted, this scenario makes it not-so-obvious again, so, 👎 . I might as well just go and replicate what Roslyn is doing in its own codefix provider. But that code is complicated, non-public and very hard to cleanly extract, so it would be a full rewrite most likely. It was far easier to just go and rewrite whatever I found after the initial scaffold... See https://github.com/devlooped/avatar/blob/main/src/Avatar.StaticProxy/AvatarScaffold.cs for reference