Ability to specify proxy generator or invalidate it #925

[Znurre]

This is a draft for the changes as per our discussion in issue #1083.

I am aware that it might need cleaning up, documentation, and so on. In this first stage, I mainly want feedback on this approach, if it's the right way to go at all.

I opted to make ProxyFactory public, which I assume is desirable if we want external projects to provide their own implementations. That in turn cascaded a bit, and required me to make some more types public.

In order to not leak too much implementation, I decided to expose the interface types instead of the abstract base classes. This might require some more boilerplate for alternative proxy factory implementations, but I would argue that it's worth it.

I can also confirm that with these rather crude changes, we are now able to plug in our own proxy generator, and by utilizing Cecil, we have managed to cut our test execution times nearly 2 hours down to 15 minutes.

[stakx]

Thanks for your interest in working on this!

I haven't finished looking through all of your PR, however my first impression is that this exposes far too many Moq internals that shouldn't be made public. Exposing those would needlessly tie our hands in refactoring Moq in the future.

The main problem in this PR is that ProxyFactory isn't ready to be exposed publicly in its current form. (I mentioned this in #1083 (comment). Much of Moq v4's internals are specifically geared towards DynamicProxy, and if we want to allow completely different proxy generators, we first need to cleanly decouple Moq's internals from DynamicProxy's capabilities. This would very likely affect the ProxyFactory's contract. If we expose ProxyFactory as it is now, you might implement that seemingly straightforward contract but then run into weird malfunctions because under the hood, Moq still expects the behaviour and restrictions from your new proxy generator that DynamicProxy exhibits.)

Many other parts of your PR appear to be a direct consequence of exposing ProxyFactory's methods, which IMHO is a no-go, so I won't go into more detail at this point.

[stakx]

I can also confirm that with these rather crude changes, we are now able to plug in our own proxy generator, and by utilizing Cecil, we have managed to cut our test execution times nearly 2 hours down to 15 minutes.

This is promising. Would you be willing to share your Cecil-based implementation of ProxyFactory? This might give us some further hints at how we'd go about decoupling Moq internals from DynamicProxy specifics.

[Znurre]

Thank you for your reply :)

Our Cecil implementation is still very messy and needs cleaning up, but I can at least share the code of our CecilProxyFactory:

internal class CecilProxyFactory : ProxyFactory
{
	private readonly ConcurrentDictionary<CacheKey, Type> proxyTypes;

	public CecilProxyFactory()
	{
		this.proxyTypes = new ConcurrentDictionary<CacheKey, Type>();

	}

	public override object CreateProxy(Type mockType, IInterceptor interceptor, Type[] interfaces, object[] arguments)
	{
		var type = this.GetProxyType(mockType, interfaces, arguments.Length);
		var argsWithInterceptor = new object[arguments.Length + 1];
		argsWithInterceptor[0] = interceptor;
		arguments.CopyTo(argsWithInterceptor, 1);
		var instance = Activator.CreateInstance(type, argsWithInterceptor);
		if(mockType.IsDelegateType())
		{
			return Delegate.CreateDelegate(mockType, instance, instance.GetType().GetMethod("Invoke"));
		}
		return instance;
	}

	private Type GetProxyType(Type mockType, Type[] interfaces, int length)
	{
		var key = CacheKey.Create(mockType, interfaces);
		return this.proxyTypes.GetOrAdd(key, key => this.CreateProxyType(mockType, interfaces));
	}


	private Type CreateProxyType(Type mockType, Type[] interfaces)
	{
		using (var builder = new AssemblyGenerator())
		{
			if (mockType.IsInterface)
			{
				var typeName = builder.DefineInterfaceProxy(mockType, interfaces);
				var type = builder.BuildAssembly().GetType(typeName);
				return type;
			}
			else if (mockType.IsDelegateType())
			{
				var typeName = builder.DefineDelegateContainer(mockType, interfaces);
				var type = builder.BuildAssembly().GetType(typeName);
				return type;
			}
			else
			{
				// Class proxy
				var typeName = builder.DefineClassProxy(mockType, interfaces);
				var type = builder.BuildAssembly().GetType(typeName);
				return type;
			}
		}
		throw new NotImplementedException();
	}

	public override bool IsMethodVisible(System.Reflection.MethodInfo method, out string messageIfNotVisible)
	{
		messageIfNotVisible = null;
		return true;
	}

	public override bool IsTypeVisible(Type type)
	{
		return true;
	}

	private sealed class CacheKey
	{
		private readonly MemberInfo[] orderedTypes;
		private readonly int hashCode;

		private CacheKey(params System.Reflection.MemberInfo[] orderedTypes)
		{
			this.orderedTypes = orderedTypes;
			this.hashCode = this.CalculateHashCode();
		}

		public override bool Equals(object obj)
		{
			var other = obj as CacheKey;
			if (other == null || this.orderedTypes.Length != other.orderedTypes.Length)
			{
				return false;
			}
			for (var i = 0; i < this.orderedTypes.Length; i++)
			{
				if (!this.orderedTypes[i].Equals(other.orderedTypes[i]))
				{
					return false;
				}
			}
			return true;
		}

		public override int GetHashCode()
		{
			return this.hashCode;
		}

		private int CalculateHashCode()
		{
			// TODO: Improve whenever
			var hash = this.orderedTypes.Length;
			for (var i = 0; i < this.orderedTypes.Length; i++)
			{
				hash = (hash * 7) ^ this.orderedTypes[i].GetHashCode();
			}
			return hash;
		}

		internal static CacheKey Create(Type t1, Type[] moreTypes)
		{
			var h = new HashSet<MemberInfo>() { t1 };
			h.UnionWith(moreTypes);
			var orderedTypes = h.OrderBy(x => x.Name, StringComparer.Ordinal).ToArray();
			return new CacheKey(orderedTypes);
		}
	}
}

As you can see, we are using the provided IInterceptor directly, which pretty much forces these changes, but we also have our own implementation of IInvocation, which pretty much only changes the behavior of CallBase.

using Moq.Async;
using System;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;

namespace Moq
{
	/// <summary/>
	public class CecilInvocation : Moq.IInvocation
	{
		/// <summary>
		/// Internal type to mark invocation results as "exception occurred during execution". The type just
		/// wraps the Exception so a thrown exception can be distinguished from an <see cref="System.Exception"/>
		/// return value.
		/// </summary>
		private readonly struct ExceptionResult
		{
			public ExceptionResult(Exception exception)
			{
				Exception = exception;
			}

			public Exception Exception { get; }
		}

		private readonly object proxyInstance;

		private object result;

		private MethodInfo methodImplementation;

		/// <summary/>
		public CecilInvocation(Type proxyType
			, System.Reflection.MethodInfo method
			, object proxyInstance
			, params object[] arguments
			)
		{
			this.proxyInstance = proxyInstance;

			this.ProxyType = proxyType;
			this.Method = method;
			this.Arguments = arguments;
		}

		/// <summary>
		/// 
		/// </summary>
		public MethodInfo Method { get; }
		/// <summary>
		/// 
		/// </summary>
		public MethodInfo MethodImplementation
		{
			get
			{
				if (this.methodImplementation == null)
				{
					this.methodImplementation = this.Method.GetImplementingMethod(this.ProxyType);
				}

				return this.methodImplementation;
			}
		}
		/// <summary>
		/// 
		/// </summary>
		public object[] Arguments { get; }
		/// <summary>
		/// 
		/// </summary>
		public ISetup MatchingSetup { get; private set; }
		/// <summary>
		/// 
		/// </summary>
		public Type ProxyType { get; }
		/// <summary>
		/// 
		/// </summary>
		public bool IsVerified { get; }
		/// <summary>
		/// 
		/// </summary>
		public object ReturnValue
		{
			get => this.result is ExceptionResult ? null : this.result;
			set
			{
				this.result = value;
			}
		}

		/// <summary>
		/// 
		/// </summary>
		public Exception Exception
		{
			get => this.result is ExceptionResult r ? r.Exception : null;
			set
			{
				this.result = new ExceptionResult(value);
			}
		}

		/// <summary/>
		public object CallBase()
		{
			if (this.Method.IsAbstract)
			{
				throw new NotSupportedException();
			}
			// call the base method
			var ftn = this.Method.MethodHandle.GetFunctionPointer();
			var delegateType = this.GetDelegateType();
			var d =(Delegate)Activator.CreateInstance(delegateType, this.proxyInstance, ftn);
			var result = d.DynamicInvoke(this.Arguments);
			return result;
		}

		/// <summary>
		/// 
		/// </summary>
		/// <param name="awaitableFactory"></param>
		public void ConvertResultToAwaitable(IAwaitableFactory awaitableFactory)
		{
			if (this.result is ExceptionResult r)
			{
				this.result = awaitableFactory.CreateFaulted(r.Exception);
			}
			else if (this.result != null && !this.Method.ReturnType.IsAssignableFrom(this.result.GetType()))
			{
				this.result = awaitableFactory.CreateCompleted(this.result);
			}
		}

		/// <summary>
		/// 
		/// </summary>
		/// <param name="setup"></param>
		public void MarkAsMatchedBy(ISetup setup)
		{
			this.MatchingSetup = setup;
		}

		/// <summary>
		/// 
		/// </summary>
		/// <returns></returns>
		private Type GetDelegateType()
		{
			var types = this.Method.GetParameters()
				.Select(p => p.ParameterType)
				.Concat(new[] { this.Method.ReturnType });
			return Expression.GetDelegateType(types.ToArray());
		}
	}
}

Perhaps it could be possible to create another abstraction on top of IInterceptor and IInvocation instead?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}