"Duplicate element: MetaMethod" for specific combinations of namespace & type names

[stakx]

Originally reported at nsubstitute/NSubstitute#585, this looks like a bug in DynamicProxy.

Code to reproduce:

using Castle.DynamicProxy;

namespace B
{
	public interface I
	{
		void M();
	}
}

namespace A
{
	public interface I
	{
		void M();
	}

	public interface H : I
	{
		new void M();
	}
}

class Program
{
	static void Main()
	{
		var generator = new ProxyGenerator();
		generator.CreateClassProxy(
			classToProxy: typeof(object),
			additionalInterfacesToProxy: new[] { typeof(B.I), typeof(A.H) },
			interceptors: new StandardInterceptor());
	}
}

The above program triggers the following exception:

Castle.DynamicProxy.ProxyGenerationException: Duplicate element: Castle.DynamicProxy.Generators.MetaMethod
   at Castle.DynamicProxy.Generators.TypeElementCollection`1.Add(TElement item) in ...\src\Castle.Core\DynamicProxy\Generators\TypeElementCollection.cs:line 51
   at Castle.DynamicProxy.Generators.MetaType.AddMethod(MetaMethod method) in ...\src\Castle.Core\DynamicProxy\Generators\MetaType.cs:line 48
   at Castle.DynamicProxy.Contributors.CompositeTypeContributor.CollectElementsToProxy(IProxyGenerationHook hook, MetaType model) in ...\src\Castle.Core\DynamicProxy\Contributors\CompositeTypeContributor.cs:line 55
   at Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateType(String name, Type[] interfaces, INamingScope namingScope) in ...\src\Castle.Core\DynamicProxy\Generators\ClassProxyGenerator.cs:line 57
   at Castle.DynamicProxy.Generators.ClassProxyGenerator.<>c__DisplayClass1_0.<GenerateCode>b__0(String n, INamingScope s) in ...\src\Castle.Core\DynamicProxy\Generators\ClassProxyGenerator.cs:line 45
   at Castle.DynamicProxy.Generators.BaseProxyGenerator.<>c__DisplayClass33_0.<ObtainProxyType>b__0(CacheKey _) in ...\src\Castle.Core\DynamicProxy\Generators\BaseProxyGenerator.cs:line 401
   at Castle.Core.Internal.SynchronizedDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory) in ...\src\Castle.Core\Core\Internal\SynchronizedDictionary.cs:line 74
   at Castle.DynamicProxy.Generators.BaseProxyGenerator.ObtainProxyType(CacheKey cacheKey, Func`3 factory) in ...\src\Castle.Core\DynamicProxy\Generators\BaseProxyGenerator.cs:line 393
   at Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateCode(Type[] interfaces, ProxyGenerationOptions options) in ...\src\Castle.Core\DynamicProxy\Generators\ClassProxyGenerator.cs:line 45
   at Castle.DynamicProxy.DefaultProxyBuilder.CreateClassProxyType(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options) in ...\src\Castle.Core\DynamicProxy\DefaultProxyBuilder.cs:line 68
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyType(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options) in ...\src\Castle.Core\DynamicProxy\ProxyGenerator.cs:line 1538
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options, Object[] constructorArguments, IInterceptor[] interceptors) in ...\src\Castle.Core\DynamicProxy\ProxyGenerator.cs:line 1440
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options, IInterceptor[] interceptors) in ...\src\Castle.Core\DynamicProxy\ProxyGenerator.cs:line 1392
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, IInterceptor[] interceptors) in ...\src\Castle.Core\DynamicProxy\ProxyGenerator.cs:line 1257
   at Program.Main()

As noted in the original issue, namespace and type names matter a lot here. Swapping the namespace names, renaming the namespace A to a, or renaming the interface H to h, will make the problem go away.

Preliminary analysis

• The reason why three identically-named types are needed is the same as in Prevent member name collision when proxy implements same generic interface more than twice #285:

  1. The first interface type's method M gets implicitly implemented as M.
  2. The second interface type's method M will get named I.M on the proxy type, i.e. DynamicProxy switches to explicit interface implementation to prevent a name collision.
  3. For the third interface type's method M, DynamicProxy tries the same strategy to prevent a name collision with M from step (1), which leads to a name collision with I.M from step (2).

• The fact that small name differences like upper/lower case makes a difference points at the type sorting that DynamicProxy uses (IIRC to encourage cache hits).

I am not yet sure how these two things interact with one another to provoke failure.

My current guess is that the error will happen iff the M method from the interface that isn't named like the two others gets processed first; because that will mean the others will both have to be explicitly named as I.M.

If the types, on the other hand, get sorted such that an I.M method get processed first, there won't be a collision as the proxy type ends up with three methods M, I.M, and H.M.

/cc @compact-github, @zvirja

[stakx]

A possible solution could work by changing code in this location:

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

Lines 43 to 51 in e40539c

	if (Contains(item))
	{
	item.SwitchToExplicitImplementation();
	if (Contains(item))
	{
	// there is something *really* wrong going on here
	throw new ProxyGenerationException("Duplicate element: " + item);
	}
	}

roughly as follows:

 if (Contains(item))
 {
 	item.SwitchToExplicitImplementation();
 	if (Contains(item))
 	{
-		// there is something *really* wrong going on here
-		throw new ProxyGenerationException("Duplicate element: " + item);
+		item.SwitchToFullyQualifiedTypeNameInImplementationName();
 	}
 }

...where that imaginary method would change an implementation method name such as Type.Method to Namespace.Type.Method.

(Even that might theoretically not be sufficient when people do really weird stuff like spreading their identically named types in identically named namespaces over several assemblies, using extern alias to be able to reference them in the same source file.)

[stakx]

It appears that the Roslyn C# compiler will generally play it safe and include namespace names when explicitly implementing a method. We could follow its lead and apply the following straightforward bug fix in MetaTypeElementUtil.CreateNameForExplicitImplementation:

1. Core/src/Castle.Core/DynamicProxy/Generators/MetaTypeElementUtil.cs

   Line 23 in e40539c

   public static string CreateNameForExplicitImplementation(Type sourceType, string name)

   +else if (!string.IsNullOrEmpty(sourceType.Namespace))
   +{
   +	return string.Concat(sourceType.Namespace, ".", sourceType.Name, ".", name);
   +}
    else
    {
    	return string.Concat(sourceType.Name, ".", name);
    }

2. Core/src/Castle.Core/DynamicProxy/Generators/MetaTypeElementUtil.cs

   Line 39 in e40539c

   private static void AppendNameOf(this StringBuilder nameBuilder, Type type)

   +if (!string.IsNullOrEmpty(type.Namespace))
   +{
   +	nameBuilder.Append(type.Namespace);
   +	nameBuilder.Append('.');
   +}
   +
    nameBuilder.Append(type.Name);

I think I'll submit a PR including test cases and whatnot shortly.

[jonorossi]

@stakx I like your suggested fix to include the namespace, interesting that we weren't doing this before, maybe the old C# compiler just prefixed with type name. I wondered if Roslyn puts long namespaces into the method name too, yep it does.