Validate delegates passed to Returns more strictly

CHANGELOG.md

@@ -18,6 +18,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 #### Changed
 
 * **Breaking change:** `SetupSequence` now overrides pre-existing setups like all other `Setup` methods do. This means that exhausted sequences no longer fall back to previous setups to produce a "default" action or return value. (@stakx, #476)
+* Delegates passed to `Returns` are validated a little more strictly than before (return type and parameter count must match with method being set up) (@stakx, #520)
 
 #### Fixed
 

Moq.Tests/ReturnsValidationFixture.cs

@@ -0,0 +1,121 @@
+using System;
+using System.Reflection;
+
+using Moq.Language.Flow;
+
+using Xunit;
+
+namespace Moq.Tests
+{
+	public class ReturnsValidationFixture
+	{
+		private Mock<IType> mock;
+		private ISetup<IType, IType> setup;
+
+		public ReturnsValidationFixture()
+		{
+			this.mock = new Mock<IType>();
+			this.setup = this.mock.Setup(m => m.Method(It.IsAny<object>()));
+		}
+
+		[Fact]
+		public void Returns_does_not_accept_delegate_without_return_value()
+		{
+			Action<object> delegateWithoutReturnValue = (arg) => { };
+
+			var ex = Record.Exception(() =>
+			{
+				this.setup.Returns(delegateWithoutReturnValue);
+			});
+
+			Assert.IsType<ArgumentException>(ex);
+		}
+
+		[Fact]
+		public void Returns_does_not_accept_delegate_with_wrong_return_type()
+		{
+			Func<string> delegateWithWrongReturnType = () => "42";
+
+			var ex = Record.Exception(() =>
+			{
+				this.setup.Returns(delegateWithWrongReturnType);
+			});
+
+			Assert.IsType<ArgumentException>(ex);
+		}
+
+		[Fact]
+		public void Returns_accepts_parameterless_delegate_even_for_method_having_parameters()
+		{
+			Func<IType> delegateWithoutParameters = () => default(IType);
+			this.setup.Returns(delegateWithoutParameters);
+
+			var ex = Record.Exception(() =>
+			{
+				this.mock.Object.Method(42);
+			});
+
+			Assert.Null(ex);
+		}
+
+		[Fact]
+		public void Returns_does_not_accept_delegate_with_wrong_parameter_count()
+		{
+			Func<object, object, IType> delegateWithWrongParameterCount = (arg1, arg2) => default(IType);
+
+			var ex = Record.Exception(() =>
+			{
+				this.setup.Returns(delegateWithWrongParameterCount);
+			});
+
+			Assert.IsType<ArgumentException>(ex);
+		}
+
+		[Fact]
+		public void Returns_accepts_delegate_with_wrong_parameter_types_but_setup_invocation_will_fail()
+		{
+			Func<string, IType> delegateWithWrongParameterType = (arg) => default(IType);
+			this.setup.Returns(delegateWithWrongParameterType);
+
+			var ex = Record.Exception(() =>
+			{
+				mock.Object.Method(42);
+			});
+
+			Assert.IsType<ArgumentException>(ex);
+
+			// In case you're wondering why this use case isn't "fixed" by properly validating delegates
+			// passed to `Returns`... it's entirely possible that some people might do this:
+			//
+			// mock.Setup(m => m.Method(It.IsAny<int>()).Returns<int>(obj => ...);
+			// mock.Setup(m => m.Method(It.IsAny<string>()).Returns<string>(obj => ...);
+			//
+			// where `Method` has a parameter of type `object`. That is, people might rely on a matcher
+			// to ensure that the return callback delegate invocation (and the cast to `object` that has to
+			// happen) will always succeed. See also the next test, as well as old Google Code issue 267
+			// in `IssueReportsFixture.cs`.
+			//
+			// While not the cleanest of techniques, it might be useful to some people and probably
+			// shouldn't be broken by eagerly validating everything.
+		}
+
+		[Fact]
+		public void Returns_accepts_delegate_with_wrong_parameter_types_and_setup_invocation_will_succeed_if_args_convertible()
+		{
+			Func<string, IType> delegateWithWrongParameterType = (arg) => default(IType);
+			this.setup.Returns(delegateWithWrongParameterType);
+
+			var ex = Record.Exception(() =>
+			{
+				mock.Object.Method(null);
+			});
+
+			Assert.Null(ex);
+		}
+
+		public interface IType
+		{
+			IType Method(object arg);
+		}
+	}
+}

Source/MethodCall.cs

@@ -157,14 +157,14 @@ public string FailMessage
 			set => this.failMessage = value;
 		}
 
+		public MethodInfo Method => this.method;
+
 		bool IProxyCall.IsConditional => this.condition != null;
 
 		bool IProxyCall.IsVerifiable => this.verifiable;
 
 		bool IProxyCall.Invoked => this.callCount > 0;
 
-		MethodInfo IProxyCall.Method => this.method;
-
 		Expression IProxyCall.SetupExpression => this.originalExpression;
 
 		[Conditional("DESKTOP")]

Source/MethodCallReturn.cs

@@ -43,6 +43,7 @@
 using Moq.Properties;
 using Moq.Proxy;
 using System;
+using System.Globalization;
 using System.Linq.Expressions;
 using System.Reflection;
 
@@ -157,10 +158,48 @@ public IReturnsResult<TMock> CallBase()
 
 		private void SetReturnDelegate(Delegate value)
 		{
+			if (value != null)
+			{
+				ValidateReturnDelegate(value);
+			}
+
 			this.valueDel = value;
 			this.returnValueKind = ReturnValueKind.Explicit;
 		}
 
+		private void ValidateReturnDelegate(Delegate callback)
+		{
+			var callbackMethod = callback.GetMethodInfo();
+
+			var actualParameters = callbackMethod.GetParameters();
+			if (actualParameters.Length > 0)
+			{
+				var expectedParameters = this.Method.GetParameters();
+				if (actualParameters.Length != expectedParameters.Length)
+				{
+					throw new ArgumentException(
+						string.Format(
+							CultureInfo.CurrentCulture,
+							Resources.InvalidCallbackParameterCountMismatch,
+							expectedParameters.Length,
+							actualParameters.Length));
+				}
+			}
+
+			var expectedReturnType = this.Method.ReturnType;
+			var actualReturnType = callbackMethod.ReturnType;
+
+			if (!expectedReturnType.IsAssignableFrom(actualReturnType))
+			{
+				throw new ArgumentException(
+					string.Format(
+						CultureInfo.CurrentCulture,
+						Resources.InvalidCallbackReturnTypeMismatch,
+						expectedReturnType,
+						actualReturnType));
+			}
+		}
+
 		protected override void SetCallbackWithoutArguments(Action callback)
 		{
 			if (this.ProvidesReturnValue())

Source/Properties/Resources.resx

@@ -365,4 +365,10 @@ Expected invocation on the mock once, but was {4} times: {1}</value>
 	<data name="ProtectedMemberNotFound" xml:space="preserve">
 		<value>Type {0} does not have matching protected member: {1}</value>
 	</data>
+	<data name="InvalidCallbackReturnTypeMismatch" xml:space="preserve">
+		<value>Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}).</value>
+	</data>
+	<data name="InvalidCallbackParameterCountMismatch" xml:space="preserve">
+		<value>Invalid callback. Setup on method with {0} parameter(s) cannot invoke callback with different number of parameters ({1}).</value>
+	</data>
 </root>