Add new methods ProxyGenerator.CreateDelegateProxy[WithTarget]
#403
Conversation
stakx
force-pushed
the
delegate-proxies
branch
7 times, most recently
from
7defa23
to
2f7ca73
Compare
stakx
changed the title
ProxyGenerator.CreateDelegateProxyProxyGenerator.CreateDelegateProxy[WithTarget]
| throw new ArgumentNullException(nameof(target)); | ||
| } | ||
|
|
||
| return (TDelegate)(object)CreateDelegateProxyImpl(typeof(TDelegate), (Delegate)(object)target, interceptors); |
There was a problem hiding this comment.
These ugly type casts could be avoided by adding a where TDelegate : Delegate constraint. This would require C# 7.3, which unfortunately Mono does not yet support, even in the current stable version (5.12).
| @@ -78,6 +78,14 @@ public Type CreateClassProxyType(Type classToProxy, Type[] additionalInterfacesT | |||
| return generator.GetGeneratedType(); | |||
| } | |||
|
|
|||
| public Type CreateDelegateProxyType(Type delegateToProxy) | |||
| { | |||
| AssertValidType(delegateToProxy); | |||
There was a problem hiding this comment.
As an aside, I noticed a fair amount of code duplication in the methods used for validating arguments.
- For
DefaultProxyBuilder, there'sAssertValidTypeandAssertValidTypes. - For
ProxyGenerator, there isCheckNotGenericTypeDefinitionandCheckNotGenericTypeDefinitions. - In
ProxyGenerator, I opted not to use the latter because they throw the wrong type of exception (GeneratorExceptionwhen I neededArgumentException), so I duplicated the same validation logic once again.
These all do very similar things. Perhaps we could merge them sometime.
There was a problem hiding this comment.
I'm interpreting your 👍 as, "Yes, let's get rid of this code duplication (but in a separate PR)." Let me know if I misinterpreted. :)
| /// <returns> | ||
| /// New delegate of type <paramref name="delegateToProxy"/> that, when invoked, calls the specified <paramref name="interceptors"/>. | ||
| /// </returns> | ||
| /// <exception cref="ArgumentNullException">Thrown when the given <paramref name="delegateToProxy"/> object is a null reference (Nothing in Visual Basic).</exception> |
There was a problem hiding this comment.
I forget to mention "[...] or when interceptors is a null reference [...]". Same in other XML documentation comments. Going to rectify this soon.
| /// <remarks> | ||
| /// Implementers should return a proxy type that contains an <c>Invoke</c> method with the same signature as | ||
| /// the <c>Invoke</c> method of the given delegate type <paramref name="delegateToProxy"/>. | ||
| /// This <c>Invoke</c> method should delegate all executions to the interceptors passed to the proxy type's constructor. |
There was a problem hiding this comment.
This should also mention "[...] delegate all executions to the target and to the interceptors passed to the proxy type's constructor [...]". Going to add this soon.
|
@stakx it looks great so far. There were a bunch of small things looking over the code but I expect many will be sorted when you finish it.
What was the plan for the Moq/NSubstitute interface delegates?
It's been there since Windsor 2.5 in 2010. I don't think any of the Windsor typed factory problems are related to this, mostly lifetimes and other things.
I don't really foresee much use, however it might come in handy for someone, especially that you can't implement it yourself (unless if you added an interceptor last) for the target to be called in the middle of the interceptor chain.
Yep, I think we do that one separately. We've still got the issue open.
Maybe I'm missing the semantics you are seeing, but |
|
@jonorossi, thanks for reviewing.
I'll tick this one off then... unless and until I hear anything else about it.
I take this to mean that you're not actively against that method. Since I've already implemented it and it doesn't add a lot of extra code to DynamicProxy, I'd say let's include it.
Sounds good.
Generally speaking, we'd ideally get things working downstream with the new I'm hoping @zvirja will chime in and say how he plans to proceed with NSubstitute. Speaking for Moq, I'm still working out all the required changes caused by going from a custom-emitted container interface type to simply using DP's There's one subtle change that caused me trouble in Moq. When using the new
If you're satisfied with the name |
That sounds like a defect in |
|
I actually just re-read my above post and came to the opposite conclusion: |
The tests in there are really about `DelegateProxyGenerator`.
Create a new fixture (having the same name as the one we previously renamed) with tests for the new `CreateDelegateProxy` functionality. The tests targeting `in` parameters are currently failing. We'll need to fix this in subsequent commits.
It turns out that the tests from the previous commit fail due to a bug in the runtime (`in` parameters + generic types cause `ldtoken` IL to reference a non-existent method) that we've previously encountered. Skip these tests and replace them with similar ones that do not rely on a generic delegate type. These will pass just fine.
... and tests verifying that the proxy calls (interceptors and) the target as it should.
Note that these tests don't opt into the same testing pattern used with other proxy kinds; i.e. we don't expand the `ProxyKind` enum be- cause the tests using it often wouldn't work for delegate proxies. This is because they are more limited than other proxy types and don't support most of the usual things like hooks or additional interfaces.
These tests attempt to document what `IInvocation.Method` and `IInvoc- ation.MethodInvocationTarget` should be for the various proxy types. These tests essentially document two facts: * `IInvocation.Method` points to the corresponding method in the proxied type. * `IInvocation.MethodInvocationTarget` points to the method that will get invoked when invocation proceeds to the target, or `null` if there is no target. No tests are added for `IInvocation.GetConcreteMethod()` and `IInvoc- ation.GetConcreteMethodInvocationTarget()`. These deal mostly with open generics and don't appear to be relevant for delegate proxies.
|
@jonorossi - Here's a summary of the latest changes:
This PR looks more or less finished for me, let me know if you find anything else that needs changing. |
| @@ -70,7 +70,7 @@ public void InterfaceProxyWithTarget_MethodInvocationTarget_should_be_methodOnTa | |||
| } | |||
|
|
|||
| [Test] | |||
| public void InterfaceProxyWithTarget_MethodInvocationTarget_should_be_null() | |||
| public void InterfaceProxyWithoutTarget_MethodInvocationTarget_should_be_null() | |||
There was a problem hiding this comment.
Fixed a typo here; otherwise unrelated to this PR.
| using NUnit.Framework; | ||
|
|
||
| [TestFixture] | ||
| public class InvocationMethodTestCase : BasePEVerifyTestCase |
There was a problem hiding this comment.
This whole file is more or less a copy of InvocationMethodInvocationTargetTestCase.cs adjusted to .Method (instead of .MethodInvocationTarget). I removed a few non-delegate proxy tests where I'm simply not sufficiently knowledgeable to know what the expected behavior should be. We can always add these back at a later time.
|
@jonorossi: You've given us a lot to think about. I'm hoping we don't have to take such a large step back... but if we do, that's OK. I'll need to think about this carefully for some time. In the meantime, another possible approach for discussion: we could introduce a options.AdditionalMethods.Add(typeof(SomeDelegateType).GetMethod("Invoke"));
generator.Create...(..., options); // business as usualThat's likely the smallest additive change to the public API I can come up with. Quite similar to mixins. It might also be useful in scenarios we don't even know yet (DLR scenarios maybe?). It would mean mocking libraries still need to call If we went that way we might want to limit |
I wasn't intending for it to be a step backwards but a step forward, we improve the existing
Interesting, however I'm struggling to see a good use case for this other than using it for delegates here, we want users adding a contract (interface or mixin). This would still require you specify a base class or interface and how would you specify the target?
This is where I think it gets a little ugly, we'd be introducing something that we don't really want, and then asking all the mocking libraries to use it, and then we'd have to support it. |
|
If we take some of what I spoke about and did one step at a time. I'm thinking we want 4 methods something like these: TDelegate CreateDelegateProxy<TDelegate>(TDelegate target, params IInterceptor[] interceptors) where TDelegate : System.Delegate;
TDelegate CreateDelegateProxy<TDelegate>(TDelegate target, ProxyGenerationOptions options, params IInterceptor[] interceptors) where TDelegate : System.Delegate;
object CreateDelegateProxy(Type delegateToProxy, object target, params IInterceptor[] interceptors);
object CreateDelegateProxy(Type delegateToProxy, object target, ProxyGenerationOptions options, params IInterceptor[] interceptors);Straight away the code in the pull request can do the first of the pairs. We just need to look at getting the |
Sorry for not being more precise, I meant "taking a step back" in the sense of "looking at the big picture & at the fundamentals", not in the sense of "regressing to a worse state". Going back to look at the very basics and work forward from there just seems like a ton of work and I was hoping we wouldn't have to go back quite that far. But I agree it might become necessary eventually, and I'm ready for it.
Same here. But it has the advantage of being a generic mechanism (as opposed to the earlier, overly specific
True. Probably best indeed to drop this idea. 😁 Regarding your latest comment, that brings us back to where we were before: Sure, we can follow this solution, if we're all fine with this (IMO) minor API design flaw. This approach is entirely practical, after all. I guess it wouldn't be too hard to propagate the |
|
Btw. like with any other class proxy, I believe a |
No problem. It just felt a little like we are going around in circles so I was trying to work out what we need the implementation to have to support the required scenarios.
Hmmm yes, there was something about the I'm definitely liking
Very true, I guess we've come down to the point where there is no way to provide the control over the type without exposing the fact that the type exists in the first place.
But class proxies always have targets, either you provide one and virtual members will go there or the proxy's base class is essentially the target.
Your I don't think I wonder if we can change mixins to support this rather than another property which works similar. How do you see the API being used, just throwing something together at the moment: // Example: Moq mocking MyClass
var options = new ProxyGenerationOptions();
// add any attributes
options.AdditionalMethods.Add(typeof(SomeDelegate).GetMethod("Invoke"));
Type t = typeof(IMocked<>).MakeGenericType(typeof(MyClass));
var proxy = generator.CreateInterfaceProxyWithoutTarget(t, options, interceptors);
var @delegate = Delegate.CreateDelegate(typeof(SomeDelegate), proxy, "Invoke");
@delegate.Invoke();With |
I think we should just allow the target to be null rather than another set of overloads/methods. |
Right. Having a container type in the mix might give some clients more control than they need (FakeItEasy, Windsor), but at the same time doesn't overly restrict others (Moq, and to some degree NSubstitute) and won't cause anyone much additional work to actually create a delegate if that's all they need. (We could provide a convenience extension method for that.)
Right. That was probably a little too simplistic. We'd perhaps need to define some kind of sealed class AdditionalMethodInfo
{
public MethodInfo TemplateMethodForSignature { get; }
// later, perhaps also (for NSubstitute):
public IList<CustomAttributeInfo> AdditionalAttributes { get; }
...
}
Perhaps by special-casing the mixin facility so it accepts delegate types as mixins? Delegate types don't implement any explicit interface but they do fulfill a well-known contract (of which we'd probably be forgiven for cherrypicking just (Btw., I don't have a clear favourite solution so far. They all have their distinct [dis]advantages.) |
We'd started mapping types outside of the generated proxy type specifically for delegates, because our hand-rolled delegates didn't lend themselves to intrinsic mapping to the "fake manager". Previously we'd used a mechanism similar to I'd have to confirm with @thomaslevesque, but I don't think FakeItEasy would reject changing the mechanism we use to link proxies to our managers. |
I have no objection to change it if it makes sense 👍 |
|
Sorry for the delay, I too didn't know which way to go.
I was wrong, just realised there is no reason you couldn't just do
For some reason I was actually thinking we couldn't make it of type
Having more of a look at mixins I don't think there will even be much special casing for Here is a runnable test case with a mixin, along with a bunch of comments for what I'm thinking: using System;
using Castle.DynamicProxy;
public delegate int AdditionDelegate(int x, int y);
class Program
{
static void Main(string[] args)
{
var options = new ProxyGenerationOptions();
options.AddMixinInstance(new Addition()); // working mixin for running code
//options.AddMixinInstance(typeof(AdditionDelegate)); // without target
//options.AddMixinInstance(new AdditionDelegate((x, y) => x + y)); // with target
// MixinData.MixinInterfaces[0] - the contract is the delegate type
// MixinData.Mixins[0] - the delegate instance if one, or null
var generator = new ProxyGenerator();
var proxy = generator.CreateClassProxy(typeof(object), options, new HelloInterceptor());
// Obviously not required, but we can just document that all delegates used as mixins will
// be given the method name "Invoke" since that is the name in the metadata of the class,
// however would be nice to have a method so a user doesn't need to call CreateDelegate
var @delegate = (AdditionDelegate)Delegate.CreateDelegate(
typeof(AdditionDelegate), (object)proxy, "Invoke");
//var @delegate = ProxyUtil.CreateDelegateToMixin<AdditionDelegate>(proxy);
//var @delegate = ProxyUtil.CreateDelegateToMixin(proxy, typeof(AdditionDelegate));
int z = @delegate.Invoke(10, 20);
Console.WriteLine(z);
}
}
// Working mixin implementation that we'd be able to do via just a delegate
public interface IAddition
{
int Invoke(int x, int y);
}
public class Addition : IAddition
{
public int Invoke(int x, int y)
{
Console.WriteLine("Calculating...");
return x + y;
}
}
public class HelloInterceptor : IInterceptor
{
public void Intercept(IInvocation invocation)
{
Console.WriteLine("Before");
invocation.Proceed();
Console.WriteLine("After");
}
}It doesn't allow you to emit custom attributes on the proxy's methods (which sounded like a nice to have, not a requirement), but it does still allow custom attributes on the proxy type, as well as doing whatever you like with the container in terms of base class, additional interfaces, etc. The implementation of this should be quite easy because the InvokeMethodOnTarget will call on the mixin field typed after the delegate (rather than an interface like normal), which is much cleaner than having a new set of fields if we went with the Does this proposal fit everyone's requirements? If you are not sure, please just ask. Please let me know which bits you like or dislike. |
|
@jonorossi - apologies for being a bit passive myself regarding this issue. I'm still quite busy settling into my new daytime job, trying to be new best buddies with PHP. (It's challenging. 😃)
Speaking for Moq, this proposal should work.
The last point brings me to...
As noted above, if we added a method such as That being said, I believe it would be very reasonable to call all delegate mixins' methods Calling all delegate mixin methods options.AddMixinInstance(new Func<object, bool>(obj => true));
options.AddMixinInstance(new Predicate<object>(obj => true));Not sure why anyone would want to do this, but we'd probably have to identify the signature collision and throw an
I cannot comment on this bit yet, as I'm still somewhat unfamiliar with how mixins are kept track of internally. Will look into it. |
|
P.S. I am not sure whether the "with target" / "without target" mapping to |
To be honest, I didn't follow very closely and I'm not sure what you're proposing exactly... With your proposal, how should we create a delegate proxy? |
|
@thomaslevesque, I'll attempt an answer by example. Following @jonorossi's proposal, if all you needed were a delegate proxy, then usage might look like this in the most basic case: // You'll need distinct `options` per delegate type. Specify additional options as needed:
var options = new ProxyGenerationOptions();
options.AddMixinInstance(typeof(SomeDelegate));
// Then use it to generate a proxy. That proxy will have an `Invoke` method with the specified
// delegate type's signature. Proxy type, base class, interceptors etc. are chosen as usual:
var proxy = proxyGenerator.CreateClassProxy(typeof(object), options, new MyInterceptor(...));
// Either call `Delegate.CreateDelegate` for the generated `Invoke` method yourself, or use a
// helper method provided by DynamicProxy:
var someDelegate = (SomeDelegate)ProxyUtil.CreateDelegateToMixin(proxy, typeof(SomeDelegate));
// Invoking the returned delegate will trigger the interceptor(s) specified above:
someDelegate(...) |
|
@stakx, thanks for the summary (and @jonorossi for the proposal!). It seems like an easy enough recipe to follow, and I've no doubt that FakeItEasy could use it. I've never used DynamicProxy outside of FakeItEasy, and have no experience with the mixin functionality, so maybe this is all idiomatic, but the |
|
@stakx, thanks. I guess we could use this, but it doesn't seem very straightforward, especially compared to the |
|
@thomaslevesque, it's true, but if I understand things, it does offer the ability to have the proxy implement an interface, which would be helpful if we want to use that mechanism for providing a link to the FakeManager… |
|
I going to start by explaining how the current mixin functionality works, I obviously assumed that was obvious which it isn't and isn't even a common or well documented feature. DynamicProxy Mixins aren't very complicated. You add an object instance as a "mixin" during proxy creation, DP will look at the interfaces the class implements and it'll additionally implement those interfaces on the proxy class and pass calls to those methods through to the mixin object. Proxies that have a target obviously hold a reference to the target (in DP it is just a field of the proxy class), the same occurs with mixin "targets", it creates a field for each interface the mixin implements. You can't cast the proxy instance to the mixin class but you can to any interface the mixin implements. My proposal is to do the same sort of thing with delegates. Since we can't explicitly expose the delegate's contract on a proxy class (a bit like the mixin isn't explicitly exposed) the proxy class would implement the delegate contract and optionally hold a reference to a target delegate if you do "Proceed" to the delegate. I think this proposal is cleaner than adding a completely different type of proxy for delegates. @stakx I know I wrote a lot in my last comment, but yes I agree we'd want a different method name for adding the delegate type. I didn't mention it in my last comment but one feature of mixins that should have been supported was to specify which interfaces you want DP to implement for you rather than it just adding all, I think this would fit in well with the add mixin method we'd add for delegates. @stakx Mixin support already requires the interfaces of mixins to be unique, i.e. two mixins can't implement the same interface; and we'd do the same thing here so you can't add two delegates with the same signature to the same proxy. @blairconrad @thomaslevesque At first it might seem more complicated than the |
Yes, it's perfectly clear. Thanks @jonorossi! |
|
Ah, yes. That is very clear. Thanks for detailed explanation, @jonorossi. I'm sorry you had to put so much time into it! |
|
Apologies once again for the long delay. Above, you wrote:
Let me attempt a sketch for a new mixin API: public void AddMixin(object instance, params Type[] mixedInTypes)This should cover what you mentioned.
public void AddMixin(object instance, Type mixedInType)This wouldn't be strictly necessary, as it is a special case of the previous method. It would be especially useful when you want to specify only one type to be mixed in, as with delegate mixins. It could also prevent an unnecessary public void AddMixin(Type mixedInType)This would be needed for delegate mixins when you do not want to specify an We could optionally relax constraints a little further and allow public void AddMixinInstance(object instance)This is what we have today. We could deprecate it in favor of (reimplement it by delegating to) the first method listed above. Finally, we could go a little further and have a Any thoughts? If this sounds about right, I could attempt an implementation for it. |
|
Closing in favour of the PR referenced above. |
|
@stakx apologies I didn't respond to your questions here on 2 Oct 2018, not sure how I missed it. |
|
@stakx were you planning to make any of your proposed changes on 2 Oct 2018 around being able to add a mixin and specifying the interfaces? If so, did you want to create another issue so we can look over the API changes. |
TBH, the mixin API in general isn't super-dear to me. I cannot say whether it gets used much at all, but I suspect not. So the time spent on improving it (purely for its own sake) would perhaps be better invested elsewhere. When I prepared my delegate mixin PR I decided on more specifically-named methods, because it's more obvious to the end user what can be mixed in. With the proposal I originally made above, you'd have had to check the XML documentation to figure out that you can use it to mix-in delegates. That being said, if you'd like, I'll open a new issue so we can track this properly. |
Good answer, that is what I expected. I'm happy for us to improve things in the future when/if there is demand. |
This is in response to #399 (comment):
and #399 (comment):
This PR would enable you to do this:
I've taken a look at how this could be implemented in DynamicProxy, and lo and behold! Most of the machinery required for delegate proxying is already there (created by @kkozmic in d3ec3bf for Castle Windsor), just not hooked up to
ProxyGeneratoryet.There's a lot of things that still need to be worked out, or worked on:Write unit tests to check whether delegate proxiess' behavior regardingIInvocation.Target,.InvocationTargetMethod,.GetConcreteMethod(),.GetConcreteMethodInvocationTarget()is in line with that of other proxy types.Let's skip
.GetConcreteMethod()and.GetConcreteMethodInvocationTarget()since those are like the properties but ensure the methods aren't open generic; since it's already impossible to build a delegate proxy for open generic delegate types, there's probably no need to test this.Write unit tests ensuring that delegate proxy types are cached.We need to add documentation (indocs/).Regarding the stability ofDelegateProxyGenerator: @fir3pho3nixx: Are you aware of any problems with the use ofDelegateProxyGeneratorin Windsor?I'll be assuming that this hasn't caused any trouble in Windsor until I hear anything to the contrary. See Add new methods
ProxyGenerator.CreateDelegateProxy[WithTarget]#403 (comment).We need to decide whether there should be aCreateDelegateProxyWithTargetas well. I have included it for now in a separate commit (which we can easily drop if we want).I'll keep that method in the PR for now. See Add new methods
ProxyGenerator.CreateDelegateProxy[WithTarget]#403 (comment) below.We might want to deprecateModuleScope.DefineTypeand all methods that allow third parties to get hold of DynamicProxy'sModuleBuilders.We'll deal with deprecations in a separate PR. See Add new methods
ProxyGenerator.CreateDelegateProxy[WithTarget]#403 (comment).The method name:CreateDelegateProxyis in line with the names of other methods onProxyGenerator, but semantically, I'm not sure if it's correct. Should it be calledCreateDelegateorCreateProxyDelegateinstead?Let's use the name as currently proposed in this PR. See Add new methods
ProxyGenerator.CreateDelegateProxy[WithTarget]#403 (comment).I have no idea how stableDelegateProxyGeneratoris. I've already found out that it cannot deal withinparameters properly (perhaps because it doesn't copy custom attributes and custom modifiers?).DelegateProxyGeneratorappears to be reasonably stable, it reuses a lot of the same classes that regular class proxy generators use. I'm adding unit tests to verify the most common operations one would perform on a delegate proxy.It might be worthwhile seeing whether Windsor is actually using itYes, it is.
We need to add XML documentation.We need to add argument validation.There are tests forDelegateProxyGenerator, but we'd need tests forProxyGenerator.CreateDelegateProxy.Reviewers: This is at an early stage, feel free to add any input or additional requirements that you might have.
/cc @zvirja