dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

System.Reflection.DispatchProxy.Create that accepts parameters to pass to TProxy's constructor #42858

Open DaleStan opened 4 years ago

DaleStan commented 4 years ago

Background and Motivation

In my use of System.Reflection.DispatchProxy, I often find that I want to add readonly fields or properties to my proxy implementation. The clean way to do this is to pass the desired values to the constructor, for example

class Proxy : DispatchProxy
{
    private readonly string _debugName;
    public Proxy(string debugName) => _debugName = debugName;
    // ...
}

I cannot do this since DispatchProxy currently requires a public default constructor. I would like to relax this requirement to either "exactly one public constructor" or "at least one public constructor".

In this proposal, I have settled on "at least one public constructor with at most one parameter"; see the Alternative Designs section for more thoughts.

Proposed API

 namespace System.Reflection
 {
     public abstract class DispatchProxy
     {
+        public static T CreateWithParameter<T, TProxy, TConstructorParameter>(TConstructorParameter constructorParameter) where TProxy : DispatchProxy
     }

Usage Examples

One current use I have is interception of calls to IDataReader, where I want to allow arbitrary SQL queries (executed locally, on a SQLite database), but I also want to present the data in a useful fashion. The timestamps stored the table are milliseconds-since-midnight, which is useless for matching with "it happened about 8:25 PM" or for determining the time between events more than a few seconds apart, so I want to automatically convert them to hh:mm:ss.fff. With this new API, my interception class would be this:

internal class TimestampConvertingReader : DispatchProxy
{
    private static readonly MethodInfo _getString = typeof(IDataRecord).GetMethod(nameof(IDataRecord.GetString));
    private static readonly MethodInfo _getFieldType = typeof(IDataRecord).GetMethod(nameof(IDataRecord.GetFieldType));

    // Calls the new CreateWithParameter method
    public static IDataReader Create(IDataReader dr) => CreateWithParameter<IDataReader, TimestampConvertingReader, IDataReader>(dr);

    private readonly IDataReader _actualReader;

    // Called by the new CreateWithParameter method, passing the dr parameter from Create.
    public TimestampConvertingReader(IDataReader actualReader) => _actualReader = actualReader;

    protected override object Invoke(MethodInfo targetMethod, object[] args)
    {
        if (targetMethod == _getFieldType && _actualReader.GetName((int)args[0]).EndsWith("TimeStamp")) { return typeof(string); }
        if (targetMethod == _getString && _actualReader.GetName((int)args[0]).EndsWith("TimeStamp")) { return TimeSpan.FromMilliseconds(_actualReader.GetInt64((int)args[0])).ToString(@"hh\:mm\:ss\.fff"); }
        // ...
        return targetMethod.Invoke(_actualReader, args);
    }
}

If there are multiple constructors, CreateWithParameter would use the specialized type of TConstructorParam to select the one-parameter constructor to invoke.

Alternative Designs

Risks

The proposed addition is still a breaking change for anyone who calls typeof(DispatchProxy).GetMethods().Single(). I don't know what sorts of Reflection breaks are acceptable, but I seem to have decided that it is acceptable to assume "This class has only one method named Create; there will never be another one," but not acceptable to assume "This class has only one public method; there will never be another one."

The proposed addition requires (1a) additional time and memory when creating the additional constructors in Create, or (1b) additional memory for the creation of a second proxy type if the first was created using the existing default-constructor-only Create. In most cases, it also requires (2) additional time when creating a proxy object with CreateWithParameters, for runtime overload resolution. 1a is a regression for existing programs, while 1b is poor behavior for new programs that use both Create and CreateWithParameters. I expect the regression to be minimal, and would tend to choose 1b over 1a, but I don't know how to measure this. 2 is a general note that CreateWithParameters has to do more work per object than Create, and will be slower.

krwq commented 3 years ago

@DaleStan why not just create derived class which provides the needed arg? Alternatively take the argument through the property instead?

DaleStan commented 3 years ago

@krwq I don't think I understand your question about creating a derived class. Today, when I use DispatchProxy.Create, I have to create a class derived from DispatchProxy, and that class has to have a public parameterless constructor. DispatchProxy.Create will call that constructor, no matter what else I do in the derived class. If I create a class derived from the dynamically-generated type that DispatchProxy.Create returns, that removes the benefit of using DispatchProxy, plus the generated type's constructor takes an Action<object> that isn't (and probably shouldn't be) documented.

I do know I don't want to use a property, because then I can't store the property value in a readonly field, nor can I use an auto-implemented get-only or get/init property.

krwq commented 3 years ago

I meant to create derived class per usage, i.e.:

class FooProxy : Proxy
{
    public FooProxy() : base("foo") {}
}
DaleStan commented 3 years ago

In production code, I'm passing the result of a SQL query, instead of a compile-time constant. I can do something like this:

public class Proxy : DispatchProxy
{
    private class ProxyHelper : Proxy
    {
        [ThreadStatic]
        internal static new IDataReader _target;
        public ProxyHelper() : base(_target) { }
    }

    private readonly IDataReader _target;
    private Proxy(IDataReader target) => _target = target;

    // Call to get a proxy for an IDataReader
    public static Proxy Create(IDataReader target)
    {
        ProxyHelper._target = target;
        return (Proxy)Create<IDataReader, ProxyHelper>();
    }
    // ...
}

This is a little better than the code I'm using currently (the Proxy constructor is no longer public) but I was hoping to get rid of that [ThreadStatic] field entirely.

steveharter commented 2 years ago

It is possible to wrap the DispatchProxy.Create() with your own static "Create()" method and set state on your proxy after it's created. Below is a sample that does this where the state is just the proxy's target instance. It is a general-use proxy (can work against any Type).

Console app sample of wrapping DispatchProxy.Create() ```cs using System.Reflection; ISample proxy = MyProxy.Create(new SampleClass()); proxy.CallMe("Hello"); class SampleClass { public void CallMe(string s) { Console.WriteLine($"Called CallMe with '{s}'"); } } interface ISample { void CallMe(string s); } public class MyProxy : DispatchProxy { public TTarget Target { get; private set; } = default!; public static TInterface Create(TTarget? target = default) { TInterface proxyInterface = Create>()!; MyProxy proxy = (MyProxy)(object)proxyInterface; proxy.Target = target ?? Activator.CreateInstance(); return proxyInterface; } protected override object? Invoke(MethodInfo? targetMethod, object?[]? args) { Console.WriteLine($"Calling {targetMethod!}"); return Target!.GetType().GetMethod(targetMethod!.Name, BindingFlags.Instance | BindingFlags.Public)!.Invoke(Target, args); // If the target is known to implement the interface, we could just do this instead: // return targetMethod.Invoke(Target, args); } } ```
DaleStan commented 2 years ago

It is possible to wrap the DispatchProxy.Create() with your own static "Create()" method

That is one of the "No API change" options I mentioned. For the sake of comparison, my proposed API would let the MyProxy class look like this instead:

MyProxy class after proposed API change ``` cs public class MyProxy : DispatchProxy { // No set/init public TTarget Target { get; } // Maybe protected (or even private) instead of public, depending how the constructor is selected by DispatchProxy public MyProxy(TTarget target) => Target = target; // No casting. On the other hand, that generic argument list got uglier. public static TInterface Create(TTarget? target = default) => CreateWithParameter, TTarget>(target ?? Activator.CreateInstance()); // Same as before protected override object? Invoke(MethodInfo? targetMethod, object?[]? args) { Console.WriteLine($"Calling {targetMethod!}"); return Target!.GetType().GetMethod(targetMethod!.Name, BindingFlags.Instance | BindingFlags.Public)!.Invoke(Target, args); // If the target is known to implement the interface, we could just do this instead: // return targetMethod.Invoke(Target, args); } } ```
steveharter commented 2 years ago

Note that the similar issue https://github.com/dotnet/runtime/issues/16614 was closed because it overlaps with this issue -- both need an alternative way to create a proxy in order to easily pass or set parameters.