Default interface methods

[stakx]

The upcoming .NET Core 3.0 along with C# 8 will support default interface methods.

It is likely that DynamicProxy won't support those properly (e.g., Proceed?) without some changes. This is a reminder to look into this.

[thomaslevesque]

I just looked into it. Proceed indeed doesn't work:

NotImplementedException: This is a DynamicProxy2 error: The interceptor attempted to 'Proceed' for method 'Int32 get_Area()' which has no target. When calling method without target there is no implementation to 'proceed' to and it is the responsibility of the interceptor to mimic the implementation (set return value, out arguments etc)

(which makes sense, since default implementations aren't actually inherited by the implementing class)

Interface methods with default implementation have IsAbstract == false

[thomaslevesque]

The only workaround I found so far for FakeItEasy is to call the default interface method non-virtually, by emitting IL with a call opcode instead of callvirt.
https://gist.github.com/thomaslevesque/a4310070de3d44f81144aafd136bea7c
Hopefully it can be done in a cleaner fashion in Castle.Core...

[thomaslevesque]

Anyway, I wonder what the correct behavior should be...
Should Proceed call the default implementation automatically? Or should it be explicit?
I'm leaning toward the former, but it might have side effects that I'm not seeing...

[stakx]

outdated

Have you tried cleaning everything (bin/obj/.vs) and killing the build server processes?

Several times, yes. Even reinstalled the whole thing on several occasions, with no better luck on subsequent tries. There's no way on Earth that I am going to do these contortions every time I need to use VS. I'd rather stay with VS1017. 😆

Should Proceed call the default implementation automatically? Or should it be explicit?

I would also prefer the former, but implicit Proceed-ing might not always be possible: One of the more difficult problems the C# language design team had to solve with default interface implementations revolved around the case where the same interface method can have different default implementations (think diamond-shaped / multiple inheritance). IIRC, they resolved it with a new, parameterized form of the base keyword. (Edit: That seems to have been resolved differently in the final released product.) I'll need to study that in more detail, but I suspect the implications of that are that Proceed might not always be able to figure out by itself which base implementation to call, and you'd have to provide explicit disambiguation.

[stakx]

This may be much easier to implement than I had previously assumed.

Up until now, I was mostly thinking about how to implement Proceed for interface methods having a default implementation. This would be akin to class override methods invoking the base class' overridden method. However, base support for default interface methods was dropped for C# 8 and, AFAIK, hasn't been added since.

This reduces the problem to ensuring two things:

1. DynamicProxy should be prepared to find some new constructs in interfaces, e.g. sealed methods (Fix MissingMethodException when proxying interfaces containing sealed methods #621) and static or non-public members. The adjustments necessary to support those probably aren't too tough.

2. When one chooses (via IProxyGenerationHook) not to intercept an interface method that has a default implementation, DynamicProxy should forward to the latter. We've probably laid the mental groundwork for this already in Forward non-intercepted methods on class proxies to target #571, and we might be able to simply leverage MetaMethod.Ignore to tell DynamicProxy not to create an override for the default implementation.

I'll give this a try later today.

[andreminelli]

This may be much easier to implement than I had previously assumed.

Up until now, I was mostly thinking about how to implement Proceed for interface methods having a default implementation. This would be akin to class override methods invoking the base class' overridden method. However, base support for default interface methods was dropped for C# 8 and, AFAIK, hasn't been added since.

This reduces the problem to ensuring two things:

1. DynamicProxy should be prepared to find some new constructs in interfaces, e.g. sealed methods (Fix MissingMethodException when proxying interfaces containing sealed methods #621) and static or non-public members. The adjustments necessary to support those probably aren't too tough.
2. When one chooses (via IProxyGenerationHook) not to intercept an interface method that has a default implementation, DynamicProxy should forward to the latter. We've probably laid the mental groundwork for this already in Forward non-intercepted methods on class proxies to target #571, and we might be able to simply leverage MetaMethod.Ignore to tell DynamicProxy not to create an override for the default implementation.

I'll give this a try later today.

Hi, @stakx
Have you tried that at all?
I am trying to help on an issue in NSubstitute where this support would make thinks easier for sure.

[stakx]

@andreminelli, yes, I have. I've so far made two separate code drafts for DIM support, however got stuck every time due to there being many use cases that all need slightly different solutions (some very easy to implement, some still very tricky), and I haven't yet figured out how to structure the PR so it makes the easiest possible changes in the most logical order for all of those cases. I've also been severely busy at work the last few months, but I may give this another go sometime in the next few months (I'm often a little less busy during spring/summertime).

[andreminelli]

Nice.
If you could figure out a way to share all these pending work and drafts in a way that one could try to help you, please contact me - even though I have no implementation knowledge of this project at this time, I must say.

[stakx]

@andreminelli, I'll check whether I still have my old code drafts, and if so, I'll push them to my GitHub fork project and let you know here where to find them.

(Just so you know what you're getting yourself into, understanding DynamicProxy's internals takes some effort... it took me roughly a year of regularly working on it to acquire a fairly complete understanding of the internal architecture. I've started a developer-oriented document describing the architecture at a high level, #624, but it's only an incomplete draft at this time.)

[andreminelli]

Thanks for the warning, @stakx .
In fact my first priority is that NSubstitute issue I have mentioned before - and I barely started to know that codebase...
As soon as I could get a grip on it I expect to be able to help out here.

So take your time to check your drafts 👍

[stakx]

@andreminelli, I found 3 drafts on my old laptop (here, here, and here) created at various times and with various levels of incompleteness; generally speaking, I never got very far due to interruptions, and when I picked this up again, I forgot where I had left off, so I started anew. I was never quite happy with the commit progression and the tests. Also, without checking, I believe I never got to the one use case that you care about most, because IIRC it's the trickiest one to solve, so I would have tackled that one last.

[stakx]

I'm finally giving this another go, but I think for now I'll only look at inheritance-based proxy types (i. e. those without target) due to time constraints... these are likely the most important proxy types for our downstream testing libraries.

[stakx]

I've merged #661 which adds support for default interface members for the most common proxy types. It is incomplete but perhaps covers the most important use cases for now. I'm going to close this issue, we can always open a new one for any important remaining use cases (proxies with target, static abstract interface methods, etc.).

[thomaslevesque]

Hi @stakx,
Awesome, thanks for doing this!
Now I need to check if we need to do anything special to use this in FakeItEasy.
But I see you haven't published the NuGet package yet!

[stakx]

@thomaslevesque, you're welcome! I'm sorry that this took so long... but I needed to step away from FOSS for a while after retiring from the Moq project out of protest, several months ago. Still, I'm happy if my work here benefits downstream libraries!

Version 5.2.0 indeed is not released yet like it should be, I'll need assistance from @jonorossi to get it pushed to NuGet. Shouldn't be too long now.

In the meantime, feel free to test your library against DynamicProxy's current master and let us know if there are further problems.

[thomaslevesque]

I'm sorry that this took so long

You have nothing to apologize for. We all have a life, and it's more important than OSS.

Version 5.2.0 indeed is not released yet like it should be, I'll need assistance from @jonorossi to get it pushed to NuGet. Shouldn't be too long now.

No problem, I'll build from source in the meantime

[thomaslevesque]

Well, as far as I can tell, it "just works" in FakeItEasy, no changes needed except updating Castle.Core. I'll probably need more tests to be sure, but it looks promising!