DynamicProxy emits invalid metadata for redeclared event

[stakx]

We got a report over at devlooped/moq#1175 that appears to be caused by a bug in DynamicProxy. I've derived the following test case:

[Test]
public void Can_proxy_type_containing_redeclared_event()
{
    _ = generator.CreateInterfaceProxyWithoutTarget<IDerived>();
}

public interface IBase
{
    event Action Event;
}

public interface IDerived : IBase
{
    new event Action<bool> Event;
}

This will generate a proxy class containing two identically-named events:

.event [mscorlib]System.Action Event
{
    .addon instance void Castle.Proxies.IDerivedProxy::add_Event(class [mscorlib]System.Action)
    .removeon instance void Castle.Proxies.IDerivedProxy::remove_Event(class [mscorlib]System.Action)
}

.event class [mscorlib]System.Action`1<bool> Event
{
    .addon instance void Castle.Proxies.IDerivedProxy::add_Event(class [mscorlib]System.Action`1<bool>)
    .removeon instance void Castle.Proxies.IDerivedProxy::remove_Event(class [mscorlib]System.Action`1<bool>)
}

...which isn't legal, and PEVerify confirms that:

[MD]: Error: Event has a duplicate (token=0x14000002). [token:0x14000001]
[MD]: Error: Event has a duplicate (token=0x14000001). [token:0x14000002]

I haven't thought about possible solutions in detail, but I suspect we would need to resolve this name conflict by renaming one of the two event implementations (ideally the one for the shadowed base type's event) to e.g. Event_1.

[stakx]

resolve this name conflict by renaming one of the two event implementations [...] to e.g. Event_1.

On second thought, we already have the means to resolve name conflicts for interface members: switching to explicit implementation (see MetaTypeElementCollection<>.Add). This is what one would have to do when implementing IDerived manually in C#, too. So instead of an event named Event_1, we'd then end up with one named either IBase.Event or IDerived.Event.

DynamicProxy currently does not switch to explicit implementation for two identically-named events because it only detects a collision when they use the same delegate types.In MetaEvent.Equals:

Core/src/Castle.Core/DynamicProxy/Generators/MetaEvent.cs

Lines 131 to 134 in d88e944

	if (!Type.Equals(other.Type))
	{
	return false;
	}

If those lines were uncommented, the above test case would pass (as would all the other tests in the test suite).

---

I suspect we have the same fundamental issue with properties:

 public interface IBase
 {
-    event Action Event;
+    Action Property { get; } 
 }

 public interface IDerived : IBase
 {
-    new event Action<bool> Event;
+    new Action<bool> Property { get; }
 }

resulting in two identically-named properties being generated:

.property instance class [mscorlib]System.Action`1<bool> Property()
{
    .get instance class [mscorlib]System.Action`1<bool> Castle.Proxies.IDerivedProxy::get_Property()
}

.property instance class [mscorlib]System.Action Property()
{
    .get instance class [mscorlib]System.Action Castle.Proxies.IDerivedProxy::get_Property()
}

Interestingly, PEVerify does not complain about this. I'm not sure why it deems this fine when identically-named events are not. At the very least, overloading properties by type is a CLS rule violation (see CLS rules 37 and 38 e.g. here).

[jonorossi]

Interestingly, PEVerify does not complain about this. I'm not sure why it deems this fine when identically-named events are not.

Probably a PEVerify bug. There has been other cases in the past where PEVerify hasn't picked invalid IL, but the CLR did.