Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Make DispatchProxy declares Property and EventInfos of the interface #9546

Merged
1 commit merged into from
Jun 23, 2016
Merged

Make DispatchProxy declares Property and EventInfos of the interface #9546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 174 additions & 2 deletions src/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,13 +426,78 @@ internal void AddInterfaceImpl(Type iface)
_assembly.EnsureTypeIsVisible(iface);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mellinoe
cc @weshaggard
cc @stephentoub


_tb.AddInterfaceImplementation(iface);

// AccessorMethods -> Metadata mappings.
var propertyMap = new Dictionary<MethodInfo, PropertyAccessorInfo>(MethodInfoEqualityComparer.Instance);
foreach (PropertyInfo pi in iface.GetRuntimeProperties())
{
var ai = new PropertyAccessorInfo(pi.GetMethod, pi.SetMethod);
if (pi.GetMethod != null)
propertyMap[pi.GetMethod] = ai;
if (pi.SetMethod != null)
propertyMap[pi.SetMethod] = ai;
}

var eventMap = new Dictionary<MethodInfo, EventAccessorInfo>(MethodInfoEqualityComparer.Instance);
foreach (EventInfo ei in iface.GetRuntimeEvents())
{
var ai = new EventAccessorInfo(ei.AddMethod, ei.RemoveMethod, ei.RaiseMethod);
if (ei.AddMethod != null)
eventMap[ei.AddMethod] = ai;
if (ei.RemoveMethod != null)
eventMap[ei.RemoveMethod] = ai;
if (ei.RaiseMethod != null)
eventMap[ei.RaiseMethod] = ai;
}

foreach (MethodInfo mi in iface.GetRuntimeMethods())
{
AddMethodImpl(mi);
MethodBuilder mdb = AddMethodImpl(mi);
PropertyAccessorInfo associatedProperty;
if (propertyMap.TryGetValue(mi, out associatedProperty))
{
if (MethodInfoEqualityComparer.Instance.Equals(associatedProperty.InterfaceGetMethod, mi))
associatedProperty.GetMethodBuilder = mdb;
else
associatedProperty.SetMethodBuilder = mdb;
}

EventAccessorInfo associatedEvent;
if (eventMap.TryGetValue(mi, out associatedEvent))
{
if (MethodInfoEqualityComparer.Instance.Equals(associatedEvent.InterfaceAddMethod, mi))
associatedEvent.AddMethodBuilder = mdb;
else if (MethodInfoEqualityComparer.Instance.Equals(associatedEvent.InterfaceRemoveMethod, mi))
associatedEvent.RemoveMethodBuilder = mdb;
else
associatedEvent.RaiseMethodBuilder = mdb;
}
}

foreach (PropertyInfo pi in iface.GetRuntimeProperties())
{
PropertyAccessorInfo ai = propertyMap[pi.GetMethod ?? pi.SetMethod];
PropertyBuilder pb = _tb.DefineProperty(pi.Name, pi.Attributes, pi.PropertyType, pi.GetIndexParameters().Select(p => p.ParameterType).ToArray());
if (ai.GetMethodBuilder != null)
pb.SetGetMethod(ai.GetMethodBuilder);
if (ai.SetMethodBuilder != null)
pb.SetSetMethod(ai.SetMethodBuilder);
}

foreach (EventInfo ei in iface.GetRuntimeEvents())
{
EventAccessorInfo ai = eventMap[ei.AddMethod ?? ei.RemoveMethod];
EventBuilder eb = _tb.DefineEvent(ei.Name, ei.Attributes, ei.EventHandlerType);
if (ai.AddMethodBuilder != null)
eb.SetAddOnMethod(ai.AddMethodBuilder);
if (ai.RemoveMethodBuilder != null)
eb.SetRemoveOnMethod(ai.RemoveMethodBuilder);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the event's Raise method also be given the same treatment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can implement it easily, but it is hard to test because C# compiler does not emit "raising event" method as long as I know. So I need to emit raise method by IL generation. It looks overkill...

if (ai.RaiseMethodBuilder != null)
eb.SetRaiseMethod(ai.RaiseMethodBuilder);
}
}

private void AddMethodImpl(MethodInfo mi)
private MethodBuilder AddMethodImpl(MethodInfo mi)
{
ParameterInfo[] parameters = mi.GetParameters();
Type[] paramTypes = ParamTypes(parameters, false);
Expand Down Expand Up @@ -541,6 +606,7 @@ private void AddMethodImpl(MethodInfo mi)
il.Emit(OpCodes.Ret);

_tb.DefineMethodOverride(mdb, mi);
return mdb;
}

private static Type[] ParamTypes(ParameterInfo[] parms, bool noByRef)
Expand Down Expand Up @@ -831,6 +897,112 @@ internal void EndSet(Type stackType)
_il.Emit(OpCodes.Stelem_Ref);
}
}

private sealed class PropertyAccessorInfo
{
public MethodInfo InterfaceGetMethod { get; }
public MethodInfo InterfaceSetMethod { get; }
public MethodBuilder GetMethodBuilder { get; set; }
public MethodBuilder SetMethodBuilder { get; set; }

public PropertyAccessorInfo(MethodInfo interfaceGetMethod, MethodInfo interfaceSetMethod)
{
InterfaceGetMethod = interfaceGetMethod;
InterfaceSetMethod = interfaceSetMethod;
}
}

private sealed class EventAccessorInfo
{
public MethodInfo InterfaceAddMethod { get; }
public MethodInfo InterfaceRemoveMethod { get; }
public MethodInfo InterfaceRaiseMethod { get; }
public MethodBuilder AddMethodBuilder { get; set; }
public MethodBuilder RemoveMethodBuilder { get; set; }
public MethodBuilder RaiseMethodBuilder { get; set; }

public EventAccessorInfo(MethodInfo interfaceAddMethod, MethodInfo interfaceRemoveMethod, MethodInfo interfaceRaiseMethod)
{
InterfaceAddMethod = interfaceAddMethod;
InterfaceRemoveMethod = interfaceRemoveMethod;
InterfaceRaiseMethod = interfaceRaiseMethod;
}
}

private sealed class MethodInfoEqualityComparer : EqualityComparer<MethodInfo>
{
public static readonly MethodInfoEqualityComparer Instance = new MethodInfoEqualityComparer();

private MethodInfoEqualityComparer() { }

public sealed override bool Equals(MethodInfo left, MethodInfo right)
{
if (ReferenceEquals(left, right))
return true;

if (left == null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's legal to have both of these defined in the same type.

    public void Foo() {}
    public void Foo<T>() {}

So you may want to add a check for generic vs. non-generic methods. Since accessor methods will never be generic, it may be better to filter that out in your main loop rather than adding logic to the comparator to handle these. Your call.

Also, I wonder if it's worth checking the method attributes (such as "static" vs. "instance") C# seems to disallow this:

public static void Foo() {}
public void Foo() {}

but I'm not sure if this is a C# restriction or an ECMA restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are happy if we could use MetadataToken... as of ECMA 335, we should check static or instance as well as generic parameters. In addition, we should check their calling conventions. I'll fix it.

return right == null;
else if (right == null)
return false;

// This assembly should work in netstandard1.3,
// so we cannot use MemberInfo.MetadataToken here.
// Therefore, it compares honestly referring ECMA-335 I.8.6.1.6 Signature Matching.
if (!Equals(left.DeclaringType, right.DeclaringType))
return false;

if (!Equals(left.ReturnType, right.ReturnType))
return false;

if (left.CallingConvention != right.CallingConvention)
return false;

if (left.IsStatic != right.IsStatic)
return false;

if ( left.Name != right.Name)
return false;

Type[] leftGenericParameters = left.GetGenericArguments();
Type[] rightGenericParameters = right.GetGenericArguments();
if (leftGenericParameters.Length != rightGenericParameters.Length)
return false;

for (int i = 0; i < leftGenericParameters.Length; i++)
{
if (!Equals(leftGenericParameters[i], rightGenericParameters[i]))
return false;
}

ParameterInfo[] leftParameters = left.GetParameters();
ParameterInfo[] rightParameters = right.GetParameters();
if (leftParameters.Length != rightParameters.Length)
return false;

for (int i = 0; i < leftParameters.Length; i++)
{
if (!Equals(leftParameters[i].ParameterType, rightParameters[i].ParameterType))
return false;
}

return true;
}

public sealed override int GetHashCode(MethodInfo obj)
{
if (obj == null)
return 0;

int hashCode = obj.DeclaringType.GetHashCode();
hashCode ^= obj.Name.GetHashCode();
foreach (ParameterInfo parameter in obj.GetParameters())
{
hashCode ^= parameter.ParameterType.GetHashCode();
}

return hashCode;
}
}
}
}
}
109 changes: 108 additions & 1 deletion src/System.Reflection.DispatchProxy/tests/DispatchProxyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
using System.Text;
using Xunit;

Expand Down Expand Up @@ -381,10 +382,116 @@ public static void Invoke_Property_Setter_And_Getter_Invokes_Correct_Methods()
expectedMethod.Name, invokedMethods[0]));

expectedMethod = propertyInfo.GetMethod;
Assert.True(invokedMethods[1] != null && expectedMethod == invokedMethods[1], String.Format("First invoke should have been {0} but actual was {1}",
Assert.True(invokedMethods[1] != null && expectedMethod == invokedMethods[1], String.Format("Second invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[1]));

Assert.Null(actualValue);
}


[Fact]
public static void Proxy_Declares_Interface_Properties()
{
TestType_IPropertyService proxy = DispatchProxy.Create<TestType_IPropertyService, TestDispatchProxy>();
PropertyInfo propertyInfo = proxy.GetType().GetTypeInfo().GetDeclaredProperty("ReadWrite");
Assert.NotNull(propertyInfo);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An Invoke_Correct_Methods() test for non-indexed properties would be appropriate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you request more test, or merging Proxy_Declares_Interface_Properties to Invoke_Property_Setter_And_Getter_Invokes_Correct_Methods ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More test. I saw a test that invokes the method for an indexed property (“Item”), but not the equivalent for a non-indexed property (“ReadWrite”) (If it’s there and I just didn’t see it, feel free to ignore.)

The non-indexed property is a pretty important edge case so I figured it deserved the same test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I intentionally did not implement it because existing Invoke_Property_Setter_And_Getter_Invokes_Correct_Methods test had already included non-indexed property (ReadWrite) invocation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then – disregard this one. Thx.

[Fact]
public static void Invoke_Event_Add_And_Remove_And_Raise_Invokes_Correct_Methods()
{
// C# cannot emit raise_Xxx method for the event, so we must use System.Reflection.Emit to generate such event.
AssemblyBuilder ab = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("EventBuilder"), AssemblyBuilderAccess.Run);
ModuleBuilder modb = ab.DefineDynamicModule("mod");
TypeBuilder tb = modb.DefineType("TestType_IEventService", TypeAttributes.Public | TypeAttributes.Interface | TypeAttributes.Abstract);
EventBuilder eb = tb.DefineEvent("AddRemoveRaise", EventAttributes.None, typeof(EventHandler));
eb.SetAddOnMethod(tb.DefineMethod("add_AddRemoveRaise", MethodAttributes.Public | MethodAttributes.Abstract | MethodAttributes.Virtual, typeof(void), new Type[] { typeof(EventHandler) }));
eb.SetRemoveOnMethod( tb.DefineMethod("remove_AddRemoveRaise", MethodAttributes.Public | MethodAttributes.Abstract | MethodAttributes.Virtual, typeof(void), new Type[] { typeof(EventHandler) }));
eb.SetRaiseMethod(tb.DefineMethod("raise_AddRemoveRaise", MethodAttributes.Public | MethodAttributes.Abstract | MethodAttributes.Virtual, typeof(void), new Type[] { typeof(EventArgs) }));
TypeInfo ieventServiceTypeInfo = tb.CreateTypeInfo();

List<MethodInfo> invokedMethods = new List<MethodInfo>();
object proxy =
typeof(DispatchProxy)
.GetRuntimeMethod("Create", Array.Empty<Type>()).MakeGenericMethod(ieventServiceTypeInfo.AsType(), typeof(TestDispatchProxy))
.Invoke(null, null);
((TestDispatchProxy)proxy).CallOnInvoke = (method, args) =>
{
invokedMethods.Add(method);
return null;
};

EventHandler handler = new EventHandler((sender, e) => {});

proxy.GetType().GetRuntimeMethods().Single(m => m.Name == "add_AddRemoveRaise").Invoke(proxy, new object[] { handler });
proxy.GetType().GetRuntimeMethods().Single(m => m.Name == "raise_AddRemoveRaise").Invoke(proxy, new object[] { EventArgs.Empty });
proxy.GetType().GetRuntimeMethods().Single(m => m.Name == "remove_AddRemoveRaise").Invoke(proxy, new object[] { handler });

Assert.True(invokedMethods.Count == 3, String.Format("Expected 3 method invocations but received {0}", invokedMethods.Count));

EventInfo eventInfo = ieventServiceTypeInfo.GetDeclaredEvent("AddRemoveRaise");
Assert.NotNull(eventInfo);

MethodInfo expectedMethod = eventInfo.AddMethod;
Assert.True(invokedMethods[0] != null && expectedMethod == invokedMethods[0], String.Format("First invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[0]));

expectedMethod = eventInfo.RaiseMethod;
Assert.True(invokedMethods[1] != null && expectedMethod == invokedMethods[1], String.Format("Second invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[1]));

expectedMethod = eventInfo.RemoveMethod;
Assert.True(invokedMethods[2] != null && expectedMethod == invokedMethods[2], String.Format("Third invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[1]));
}

[Fact]
public static void Proxy_Declares_Interface_Events()
{
TestType_IEventService proxy = DispatchProxy.Create<TestType_IEventService, TestDispatchProxy>();
EventInfo eventInfo = proxy.GetType().GetTypeInfo().GetDeclaredEvent("AddRemove");
Assert.NotNull(eventInfo);
}


[Fact]
public static void Invoke_Indexer_Setter_And_Getter_Invokes_Correct_Methods()
{
List<MethodInfo> invokedMethods = new List<MethodInfo>();

TestType_IIndexerService proxy = DispatchProxy.Create<TestType_IIndexerService, TestDispatchProxy>();
((TestDispatchProxy)proxy).CallOnInvoke = (method, args) =>
{
invokedMethods.Add(method);
return null;
};


proxy["key"] = "testValue";
string actualValue = proxy["key"];

Assert.True(invokedMethods.Count == 2, String.Format("Expected 2 method invocations but received {0}", invokedMethods.Count));

PropertyInfo propertyInfo = typeof(TestType_IIndexerService).GetTypeInfo().GetDeclaredProperty("Item");
Assert.NotNull(propertyInfo);

MethodInfo expectedMethod = propertyInfo.SetMethod;
Assert.True(invokedMethods[0] != null && expectedMethod == invokedMethods[0], String.Format("First invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[0]));

expectedMethod = propertyInfo.GetMethod;
Assert.True(invokedMethods[1] != null && expectedMethod == invokedMethods[1], String.Format("Second invoke should have been {0} but actual was {1}",
expectedMethod.Name, invokedMethods[1]));

Assert.Null(actualValue);
}

[Fact]
public static void Proxy_Declares_Interface_Indexers()
{
TestType_IIndexerService proxy = DispatchProxy.Create<TestType_IIndexerService, TestDispatchProxy>();
PropertyInfo propertyInfo = proxy.GetType().GetTypeInfo().GetDeclaredProperty("Item");
Assert.NotNull(propertyInfo);
}
}
}
12 changes: 12 additions & 0 deletions src/System.Reflection.DispatchProxy/tests/TestTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ public interface TestType_IPropertyService
string ReadWrite { get; set; }
}

// Demonstrates proxies can be made for events.
public interface TestType_IEventService
{
event EventHandler AddRemove;
}

// Demonstrates proxies can be made for indexed properties.
public interface TestType_IIndexerService
{
string this[string key] { get; set; }
}

// Demonstrates proxies can be made for internal types
internal interface TestType_InternalInterfaceService
{
Expand Down
3 changes: 2 additions & 1 deletion src/System.Reflection.DispatchProxy/tests/project.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
"System.Linq.Expressions": "4.1.1-beta-24223-03",
"System.ObjectModel": "4.0.13-beta-24223-03",
"System.Reflection": "4.1.1-beta-24223-03",
"System.Reflection.Emit": "4.0.2-beta-24223-03",
"System.Reflection.Emit.ILGeneration": "4.0.2-beta-24223-03",
"System.Runtime": "4.1.1-beta-24223-03",
"System.Text.RegularExpressions": "4.2.0-beta-24223-03",
"test-runtime": {
Expand All @@ -20,7 +22,6 @@
"netstandard1.3": {}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect netcore50 needed to be removed because you took a dependency on Reflection.Emit. That will block these tests from running in .NET Native.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is required to test raise_Xxxbecause C# code cannot emit raise_ methods of events.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK at a future point we may need to split these tests into 2 projects to enable us to run the majority on both .NET Core and .NET Native.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for follow-up.

"supports": {
"coreFx.Test.netcore50": {},
"coreFx.Test.netcoreapp1.0": {},
"coreFx.Test.net46": {},
"coreFx.Test.net461": {},
Expand Down