castleproject / Core

Castle Core, including Castle DynamicProxy, Logging Services and DictionaryAdapter
http://www.castleproject.org/
Other
2.2k stars 467 forks source link

`ArgumentException`: "Could not find method overriding method" with overridden class method having generic by-ref parameter #655

Closed rbeurskens closed 1 year ago

rbeurskens commented 1 year ago

Note that I encountered this using NSubstitute and I'm not using Castle.Core directly, but the stack trace comes from Castle.Core and tells me it is likely a bug and asks me to report it. (NSubstitute issue: https://github.com/nsubstitute/NSubstitute/issues/716 ) Version used is 5.0.0, running on .NET Framework 4.8

System.ArgumentException : Could not find method overriding Boolean TryCreateControl[TResult](IBaseControl, IHasGridLinesDefined, TResult ByRef) on type MyControlFactory. This is most likely a bug. Please report it.
   at Castle.DynamicProxy.Internal.InvocationHelper.ObtainMethod(MethodInfo proxiedMethod, Type type)
   at Castle.Core.Internal.SynchronizedDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Castle.DynamicProxy.Internal.InvocationHelper.GetMethodOnType(Type type, MethodInfo proxiedMethod)
   at NSubstitute.Proxies.CastleDynamicProxy.CastleInvocationMapper.Map(IInvocation castleInvocation)
   at NSubstitute.Proxies.CastleDynamicProxy.CastleForwardingInterceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.MyControlFactoryProxy.TryCreateControl[TResult](IBaseControl control, IHasGridLinesDefined parentGrid, TResult& result)

This occurs when trying to configure an interception for a (generic) method (with an out argument) that is overridden but not defined on the class being proxied.

public interface IControlFactory
{
    bool TryCreateControl<TResult>(IBaseControl control, IHasGridLinesDefined? parentGrid, [MaybeNullWhen(false)] out TResult result);
}
public class ControlFactory: IControlFactory
{
    public virtual bool TryCreateControl<TResult>(IBaseControl control, IHasGridLinesDefined? parentGrid, [MaybeNullWhen(false)] out TResult result) { /* ... */ }
}
public class MyControlFactory: ControlFactory
{
    // If this override is removed, code runs as expected
    public override bool TryCreateControl<TResult>(IBaseControl control, IHasGridLinesDefined? parentGrid, [MaybeNullWhen(false)] out TResult result) { /* ... */ }
}

[Test]
void MyTest()
{
    // sorry, this is nsubstitute syntax. I don't know how this translates to Castle.Core calls, but I hope it provides enough information on what I am trying to do
    var control = Substitute.For<IStringTextBoxControl>();
    var controlFactory = Substitute.For<MyControlFactory>();
    controlFactory.Configure().TryCreateControl<object>(default, default, out var _)
          .ReturnsForAnyArgs(ci => { ci[2] = control; return true; }); // <== exception here
}
stakx commented 1 year ago

Thanks for reporting this @rbeurskens. Don't worry about your repro code example making use of NSubstitute for now, I expect it shouldn't be too difficult figuring out the underlying calls and deriving DynamicProxy-only repro code.

stakx commented 1 year ago

OK, it's taken me a while, but here is a corresponding DynamicProxy-only repro code example:

var generator = new ProxyGenerator();
var factory = generator.CreateClassProxy<DerivedFactory>(new QueryMethodInvocationTargetInterceptor());
factory.Create<object>(out _);

class QueryMethodInvocationTargetInterceptor : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        _ = invocation.MethodInvocationTarget;
    }
}

public interface IFactory  // NOTE: the issue is also reproducible without this interface
{
    void Create<T>(out T result);
}

public class Factory : IFactory
{
    public virtual void Create<T>(out T result) => result = default;
}

public class DerivedFactory : Factory
{
    public override void Create<T>(out T result) => result = default;
}

It seems that the error is caused in this method: https://github.com/castleproject/Core/blob/dca4ed09df545dd7512c82778127219795668d30/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs#L104-L153

When given two matching generic types (or type parameters) by-ref (in our repro code: the out T result parameters' types), this method claims that they do not match. This could possibly be fixed by introducing some new logic that erases by-ref-ness for further comparisons:

 private bool EqualSignatureTypes(Type x, Type y)
 {
     if (x.IsGenericParameter != y.IsGenericParameter)
     {
         return false;
     }
     else if (x.IsGenericType != y.IsGenericType)
     {
         return false;
     }

+    if (x.IsByRef != y.IsByRef)
+    {
+        return false;
+    }
+
+    if (x.IsByRef)
+    {
+        x = x.GetElementType();
+        y = y.GetElementType();
+    }
+
     if (x.IsGenericParameter)
     ...

But I haven't checked yet whether this code breaks any existing test cases.

The suggested code addition does not appear to break any existing test cases, but we should still double-check its validity, and whether it can possibly be added in a more optimal place (from a run-time performance standpoint).

stakx commented 1 year ago

Scratch the code fix suggested above, the following would be more correct (because it doesn't skip the first two checks):

 private bool EqualSignatureTypes(Type x, Type y)
 {
+    if (x.IsByRef != y.IsByRef)
+    {
+        return false;
+    }
+    else if (x.IsByRef)
+    {
+        return EqualSignatureTypes(x.GetElementType(), y.GetElementType());
+    }
+
     if (x.IsGenericParameter != y.IsGenericParameter)
     ...