TypeLoadException when generating proxies for custom collection types

[PleasantD]

Ran into this when trying to get our Envers integration updated.

System.TypeLoadException : Declaration referenced in a method implementation cannot be a final method.  Type: 'PersistentObservableBag`1Proxy'.  Assembly: 'PersistentObservableBag`1ProxyAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
at System.Reflection.Emit.TypeBuilder.TermCreateClass(RuntimeModule module, Int32 tk, ObjectHandleOnStack type)
   at System.Reflection.Emit.TypeBuilder.CreateTypeNoLock()
   at System.Reflection.Emit.TypeBuilder.CreateTypeInfo()
   at NHibernate.Proxy.DynamicProxy.ProxyFactory.CreateUncachedProxyType(Type baseType, IReadOnlyCollection`1 baseInterfaces)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at NHibernate.Proxy.DynamicProxy.ProxyFactory.CreateProxy(Type instanceType, IInterceptor interceptor, Type[] baseInterfaces)
   at NHibernate.Envers.Entities.Mapper.Relation.AbstractCollectionMapper.MapToEntityFromMap(AuditConfiguration verCfg, Object obj, IDictionary data, Object primaryKey, IAuditReaderImplementor versionsReader, Int64 revision)
   at NHibernate.Envers.Entities.Mapper.MultiPropertyMapper.MapToEntityFromMap(AuditConfiguration verCfg, Object obj, IDictionary data, Object primaryKey, IAuditReaderImplementor versionsReader, Int64 revision)
   at NHibernate.Envers.Entities.EntityInstantiator.CreateInstanceFromVersionsEntity(String entityName, IDictionary versionsEntity, Int64 revision)
   at NHibernate.Envers.Query.Impl.AllEntitiesAtRevisionQuery`1.<Results>b__2_0(IDictionary versionsEntity)
   at System.Linq.Enumerable.WhereSelectListIterator`2.MoveNext()
   at NHibernate.Envers.Query.Impl.AbstractRevisionsQuery`1.Single()
...

[fredericDelaporte]

Related discussion on the development group:
https://groups.google.com/forum/#!topic/nhibernate-development/Tg86457N4UU

Discussion triggered from here.

For me, this introduces bugs in the NHibernate proxy factory.

@hazzik wrote:

The patch is correct and, in fact, matches what C# compiler generates for explicit interface members.

I do not contest it matches what C# compiler generates for explicit interface members. I contest the member should be implemented as an explicit interface member. Doing so may cause a base implicit interface member to be not proxified, defeating the proxy purpose.

But Frédéric's point is also correct and despite the TypeLoadException is not thrown the code will have a different behaviour based on a variable type (as shown in Frédéric's gist). However, because we use ILazyCollectionInitializer everywhere the effects of this inconsistency would be neglected.

Why not if the patch was restricted to ILazyCollectionInitializer members. But as it stands currently, it will affect any interface members. This may cause bugs for users having entities implementing interfaces, while being proxified and used as concrete classes, not as interfaces.

I have put "may" in my sentences because what will generate the proxy factory with the change will maybe be indeed something like:

public interface IA
{
	string SomeProperty { get; set; }
}

public class A : IA
{		
	public virtual string SomeProperty { get; set; } = "non-initialized";
}
	
public class FakeAProxy : A, IA
{
	public override string SomeProperty
	{
		get => ActualState.SomeProperty;
		set => ActualState.SomeProperty = value;
	}

	string IA.SomeProperty
	{
		get => ActualState.SomeProperty;
		set => ActualState.SomeProperty = value;
	}

	internal A ActualState { get; set;} = new A { SomeProperty = "initialized" };
}

This is because the proxy factory generates methods for all members of ancestors, classes and interfaces, without de-duplicating those present as both a class member and an interface member.

(This does not create issues with current code because a special method for declaring members is (ab)used: DefineMethodOverride. Contrary to what its name suggest, it is not meant to define overrides of ancestors, but it is meant to allow defining a method body which name does not match the method declaration name. Regular overrides of ancestor do not need this method to be declared. But this method has the additional characteristics of erasing any conflicting previous method declaration made on the constructed type. So the fact that our current proxy building mechanism tends to declare duplicates of members in some cases is compensated by the use of this DefineMethodOverride method. By the way, using this method is required for explicit implementations of interface members.)

In such case, the proposed change will not fix anything indeed: invalid code in the Enver's case will still be generated.

[hazzik]

Closing because of following:

1. Envers does not use the NHibernate's ProxyFactory anymore
2. The proxy validator correctly identifies that the alleged type is not proxyable