From dc3a00f414da08d42b773a37ace5f6581e06276c Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Sep 2017 21:01:32 +0200 Subject: [PATCH 1/4] Move `Mock.proxyFactory` to `Mock.ProxyFactory` For two reasons: 1. We will need to have it available inside the `Mock` class. 2. We don't need several factories, a single one is enough, so having it in a generic class means that more factories will get instantiated than necessary. This can be avoided by the move from `Mock` to `Mock`. --- Source/Mock.Generic.cs | 7 +++---- Source/Mock.cs | 2 ++ Source/Proxy/CastleProxyFactory.cs | 2 ++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Source/Mock.Generic.cs b/Source/Mock.Generic.cs index 6e3b3cb4d..8d6fc1e8b 100644 --- a/Source/Mock.Generic.cs +++ b/Source/Mock.Generic.cs @@ -58,7 +58,6 @@ namespace Moq /// public partial class Mock : Mock, IMock 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(), diff --git a/Source/Mock.cs b/Source/Mock.cs index bb1888c28..8ee5637fe 100644 --- a/Source/Mock.cs +++ b/Source/Mock.cs @@ -55,6 +55,8 @@ namespace Moq /// 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(); diff --git a/Source/Proxy/CastleProxyFactory.cs b/Source/Proxy/CastleProxyFactory.cs index 5ed30159a..7ce5aab2d 100644 --- a/Source/Proxy/CastleProxyFactory.cs +++ b/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")] From c9145038fe313ba32dd2dd75be1ba4306a402853 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Sep 2017 21:46:10 +0200 Subject: [PATCH 2/4] Setup: Throw when method inaccessible to proxy generator --- Source/Mock.cs | 17 +++++++++++++++++ Source/Properties/Resources.Designer.cs | 14 ++++++++++++-- Source/Properties/Resources.resx | 4 ++++ Source/Proxy/CastleProxyFactory.cs | 5 +++++ Source/Proxy/IProxyFactory.cs | 2 ++ 5 files changed, 40 insertions(+), 2 deletions(-) diff --git a/Source/Mock.cs b/Source/Mock.cs index 8ee5637fe..78bdc086d 100644 --- a/Source/Mock.cs +++ b/Source/Mock.cs @@ -436,6 +436,7 @@ internal static MethodCall Setup(Mock mock, Expression> expre var args = methodCall.Arguments.ToArray(); ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, method); + ThrowIfSetupMethodNotVisibleToProxyFactory(method); var call = new MethodCall(mock, condition, expression, method, args); var targetInterceptor = GetInterceptor(methodCall.Object, mock); @@ -464,6 +465,7 @@ internal static MethodCallReturn Setup( var args = methodCall.Arguments.ToArray(); ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, method); + ThrowIfSetupMethodNotVisibleToProxyFactory(method); var call = new MethodCallReturn(mock, condition, expression, method, args); var targetInterceptor = GetInterceptor(methodCall.Object, mock); @@ -493,6 +495,7 @@ internal static MethodCallReturn SetupGet( var propGet = prop.GetGetMethod(true); ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, propGet); + ThrowIfSetupMethodNotVisibleToProxyFactory(propGet); var call = new MethodCallReturn(mock, condition, expression, propGet, new Expression[0]); // Directly casting to MemberExpression is fine as ToPropertyInfo would throw if it wasn't @@ -551,6 +554,7 @@ internal static SetterMethodCall SetupSet( var propSet = prop.GetSetMethod(true); ThrowIfSetupExpressionInvolvesUnsupportedMember(expression, propSet); + ThrowIfSetupMethodNotVisibleToProxyFactory(propSet); var call = new SetterMethodCall(mock, expression, propSet); var targetInterceptor = GetInterceptor(((MemberExpression)expression.Body).Expression, mock); @@ -802,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) diff --git a/Source/Properties/Resources.Designer.cs b/Source/Properties/Resources.Designer.cs index ed1502b47..360eb6b4f 100644 --- a/Source/Properties/Resources.Designer.cs +++ b/Source/Properties/Resources.Designer.cs @@ -19,7 +19,7 @@ namespace Moq.Properties { // class via a tool like ResGen or Visual Studio. // To add or remove a member, edit your .ResX file then rerun ResGen // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "4.0.0.0")] + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "15.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class Resources { @@ -243,7 +243,17 @@ internal static string MethodIsPublic { return ResourceManager.GetString("MethodIsPublic", resourceCulture); } } - + + /// + /// Looks up a localized string similar to Cannot set up {0}.{1} because it is not accessible to the proxy generator used by Moq: + ///{2}. + /// + internal static string MethodNotVisibleToProxyFactory { + get { + return ResourceManager.GetString("MethodNotVisibleToProxyFactory", resourceCulture); + } + } + /// /// Looks up a localized string similar to Mininum delay has to be lower than maximum delay.. /// diff --git a/Source/Properties/Resources.resx b/Source/Properties/Resources.resx index 53daae6fe..8ae8bf56b 100644 --- a/Source/Properties/Resources.resx +++ b/Source/Properties/Resources.resx @@ -352,4 +352,8 @@ Expected invocation on the mock once, but was {4} times: {1} Use ItExpr.IsNull<TValue> rather than a null argument value, as it prevents proper method lookup. + + Cannot set up {0}.{1} because it is not accessible to the proxy generator used by Moq: +{2} + diff --git a/Source/Proxy/CastleProxyFactory.cs b/Source/Proxy/CastleProxyFactory.cs index 7ce5aab2d..dff9d7d96 100644 --- a/Source/Proxy/CastleProxyFactory.cs +++ b/Source/Proxy/CastleProxyFactory.cs @@ -106,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 delegateInterfaceCache = new Dictionary(); private static readonly ProxyGenerationOptions proxyOptions; private static int delegateInterfaceSuffix; diff --git a/Source/Proxy/IProxyFactory.cs b/Source/Proxy/IProxyFactory.cs index 087f28eca..ac5a9d660 100644 --- a/Source/Proxy/IProxyFactory.cs +++ b/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); + /// /// Gets an autogenerated interface with a method on it that matches the signature of the specified /// . From 8c9aa69e174d03046ccc0ed73a21fbec0d10d72f Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Sep 2017 22:02:28 +0200 Subject: [PATCH 3/4] Add unit tests for new functionality --- UnitTests/MockFixture.cs | 41 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/UnitTests/MockFixture.cs b/UnitTests/MockFixture.cs index 0060575f1..454e2a9e6 100644 --- a/UnitTests/MockFixture.cs +++ b/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(); + + var error = Record.Exception(() => + { + mock.Setup(m => m.Internal()); + }); + + Assert.NotNull(error); + Assert.IsType(error); + Assert.Contains("accessible", error.Message); + Assert.Contains("proxy generator", error.Message); + } + + [Fact] + public void SetupOnAccessibleMethodDoesNotThrowException() + { + var mock = new Mock(); + + 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"); + } + } } } From 1a75ba523c31bdac03a70d7cbcb86bd5b476db42 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Sep 2017 22:23:43 +0200 Subject: [PATCH 4/4] Update the changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e9714eac..6bd9f607c 100644 --- a/CHANGELOG.md +++ b/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)