Merge pull request #455 from stakx/invisible-internals

Make setups for inaccessible internal members fail fast by throwing an exception

CHANGELOG.md

@@ -7,6 +7,10 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 
 ## Unreleased
 
+#### Changed
+
+* Make setups for inaccessible internal members fail fast by throwing an exception (@stakx, #455)
+
 #### Fixed
 
 * Make `SetupAllProperties` work correctly for same-typed sibling properties (@stakx, #442)

Source/Mock.Generic.cs

@@ -58,7 +58,6 @@ namespace Moq
 	/// <include file='Mock.Generic.xdoc' path='docs/doc[@for="Mock{T}"]/*'/>
     public partial class Mock<T> : Mock, IMock<T> where T : class
 	{
-		private static IProxyFactory proxyFactory = new CastleProxyFactory();
 		private static int serialNumberCounter = 0;
 		private T instance;
 		private object[] constructorArguments;
@@ -195,10 +194,10 @@ private void InitializeInstance()
 					// We're mocking a delegate.
 					// Firstly, get/create an interface with a method whose signature
 					// matches that of the delegate.
-					var delegateInterfaceType = proxyFactory.GetDelegateProxyInterface(typeof(T), out delegateInterfaceMethod);
+					var delegateInterfaceType = Mock.ProxyFactory.GetDelegateProxyInterface(typeof(T), out delegateInterfaceMethod);
 
 					// Then create a proxy for that.
-					var delegateProxy = proxyFactory.CreateProxy(
+					var delegateProxy = Mock.ProxyFactory.CreateProxy(
 						delegateInterfaceType,
 						this.Interceptor,
 						this.ImplementedInterfaces.ToArray(),
@@ -210,7 +209,7 @@ private void InitializeInstance()
 				}
 				else
 				{
-					this.instance = (T)proxyFactory.CreateProxy(
+					this.instance = (T)Mock.ProxyFactory.CreateProxy(
 						typeof(T),
 						this.Interceptor,
 						this.ImplementedInterfaces.Skip(this.InternallyImplementedInterfaceCount - 1).ToArray(),

Source/Mock.cs

@@ -55,6 +55,8 @@ namespace Moq
 	/// <include file='Mock.xdoc' path='docs/doc[@for="Mock"]/*'/>
 	public abstract partial class Mock : IFluentInterface
 	{
+		internal static IProxyFactory ProxyFactory => CastleProxyFactory.Instance;
+
 		private bool isInitialized;
 		private DefaultValue defaultValue = DefaultValue.Empty;
 		private IDefaultValueProvider defaultValueProvider = new EmptyDefaultValueProvider();
@@ -434,6 +436,7 @@ internal static MethodCall<T> Setup<T>(Mock<T> mock, Expression<Action<T>> expre
 				var args = methodCall.Arguments.ToArray();
 
 				ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, method);
+				ThrowIfSetupMethodNotVisibleToProxyFactory(method);
 				var call = new MethodCall<T>(mock, condition, expression, method, args);
 
 				var targetInterceptor = GetInterceptor(methodCall.Object, mock);
@@ -462,6 +465,7 @@ internal static MethodCallReturn<T, TResult> Setup<T, TResult>(
 				var args = methodCall.Arguments.ToArray();
 
 				ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, method);
+				ThrowIfSetupMethodNotVisibleToProxyFactory(method);
 				var call = new MethodCallReturn<T, TResult>(mock, condition, expression, method, args);
 
 				var targetInterceptor = GetInterceptor(methodCall.Object, mock);
@@ -491,6 +495,7 @@ internal static MethodCallReturn<T, TProperty> SetupGet<T, TProperty>(
 
 				var propGet = prop.GetGetMethod(true);
 				ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, propGet);
+				ThrowIfSetupMethodNotVisibleToProxyFactory(propGet);
 
 				var call = new MethodCallReturn<T, TProperty>(mock, condition, expression, propGet, new Expression[0]);
 				// Directly casting to MemberExpression is fine as ToPropertyInfo would throw if it wasn't
@@ -549,6 +554,7 @@ internal static SetterMethodCall<T, TProperty> SetupSet<T, TProperty>(
 
 			var propSet = prop.GetSetMethod(true);
 			ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, propSet);
+			ThrowIfSetupMethodNotVisibleToProxyFactory(propSet);
 
 			var call = new SetterMethodCall<T, TProperty>(mock, expression, propSet);
 			var targetInterceptor = GetInterceptor(((MemberExpression)expression.Body).Expression, mock);
@@ -800,6 +806,19 @@ private static void ThrowIfPropertyNotReadable(PropertyInfo prop)
 			}
 		}
 
+		private static void ThrowIfSetupMethodNotVisibleToProxyFactory(MethodInfo method)
+		{
+			if (Mock.ProxyFactory.IsMethodVisible(method, out string messageIfNotVisible) == false)
+			{
+				throw new ArgumentException(string.Format(
+					CultureInfo.CurrentCulture,
+					Resources.MethodNotVisibleToProxyFactory,
+					method.DeclaringType.Name,
+					method.Name,
+					messageIfNotVisible));
+			}
+		}
+
 		private static void ThrowIfSetupExpressionInvolvesUnsupportedMember(Expression setup, MethodInfo method)
 		{
 			if (method.IsStatic)

Source/Properties/Resources.resx

@@ -352,4 +352,8 @@ Expected invocation on the mock once, but was {4} times: {1}</value>
 	<data name="UseItExprIsNullRatherThanNullArgumentValue" xml:space="preserve">
 		<value>Use ItExpr.IsNull&lt;TValue&gt; rather than a null argument value, as it prevents proper method lookup.</value>
 	</data>
+	<data name="MethodNotVisibleToProxyFactory" xml:space="preserve">
+		<value>Cannot set up {0}.{1} because it is not accessible to the proxy generator used by Moq:
+{2}</value>
+	</data>
 </root>

Source/Proxy/CastleProxyFactory.cs

@@ -58,6 +58,8 @@ namespace Moq.Proxy
 {
 	internal class CastleProxyFactory : IProxyFactory
 	{
+		public static CastleProxyFactory Instance { get; } = new CastleProxyFactory();
+
 		private static readonly ProxyGenerator generator = CreateProxyGenerator();
 
 		[SuppressMessage("Microsoft.Performance", "CA1810:InitializeReferenceTypeStaticFieldsInline", Justification = "By Design")]
@@ -104,6 +106,11 @@ public object CreateProxy(Type mockType, ICallInterceptor interceptor, Type[] in
 			}
 		}
 
+		public bool IsMethodVisible(MethodInfo method, out string messageIfNotVisible)
+		{
+			return ProxyUtil.IsAccessible(method, out messageIfNotVisible);
+		}
+
 		private static readonly Dictionary<Type, Type> delegateInterfaceCache = new Dictionary<Type, Type>();
 		private static readonly ProxyGenerationOptions proxyOptions;
 		private static int delegateInterfaceSuffix;

Source/Proxy/IProxyFactory.cs

@@ -47,6 +47,8 @@ internal interface IProxyFactory
 	{
 		object CreateProxy(Type mockType, ICallInterceptor interceptor, Type[] interfaces, object[] arguments);
 
+		bool IsMethodVisible(MethodInfo method, out string messageIfNotVisible);
+
 		/// <summary>
 		/// Gets an autogenerated interface with a method on it that matches the signature of the specified
 		/// <paramref name="delegateType"/>.

UnitTests/MockFixture.cs

@@ -1084,5 +1084,46 @@ public interface INewFoo : IFoo
 		public interface INewBar : IBar
 		{
 		}
+
+		// Note that this test requires that there be no [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]
+		// or similar defined in this test assembly. If some other test requires that internals be made
+		// visible to DynamicProxy, then this test must be disabled.
+		[Fact]
+		public void SetupOnInaccessibleMethodThrowsException()
+		{
+			var mock = new Mock<Accessibility.ClassWithAccessibleAndInaccessibleMethod>();
+
+			var error = Record.Exception(() =>
+			{
+				mock.Setup(m => m.Internal());
+			});
+
+			Assert.NotNull(error);
+			Assert.IsType<ArgumentException>(error);
+			Assert.Contains("accessible", error.Message);
+			Assert.Contains("proxy generator", error.Message);
+		}
+
+		[Fact]
+		public void SetupOnAccessibleMethodDoesNotThrowException()
+		{
+			var mock = new Mock<Accessibility.ClassWithAccessibleAndInaccessibleMethod>();
+
+			var error = Record.Exception(() =>
+			{
+				mock.Setup(m => m.Public());
+			});
+
+			Assert.Null(error);
+		}
+
+		public class Accessibility
+		{
+			public class ClassWithAccessibleAndInaccessibleMethod
+			{
+				public virtual void Public() => throw new InvalidOperationException("Public");
+				internal virtual void Internal() => throw new InvalidOperationException("Internal");
+			}
+		}
 	}
 }