diff --git a/CHANGELOG.md b/CHANGELOG.md index d27dbd568..8f7688213 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Moq.Tests/ReturnsValidationFixture.cs b/Moq.Tests/ReturnsValidationFixture.cs new file mode 100644 index 000000000..5a0d5c217 --- /dev/null +++ b/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 mock; + private ISetup setup; + + public ReturnsValidationFixture() + { + this.mock = new Mock(); + this.setup = this.mock.Setup(m => m.Method(It.IsAny())); + } + + [Fact] + public void Returns_does_not_accept_delegate_without_return_value() + { + Action delegateWithoutReturnValue = (arg) => { }; + + var ex = Record.Exception(() => + { + this.setup.Returns(delegateWithoutReturnValue); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_does_not_accept_delegate_with_wrong_return_type() + { + Func delegateWithWrongReturnType = () => "42"; + + var ex = Record.Exception(() => + { + this.setup.Returns(delegateWithWrongReturnType); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_parameterless_delegate_even_for_method_having_parameters() + { + Func 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 delegateWithWrongParameterCount = (arg1, arg2) => default(IType); + + var ex = Record.Exception(() => + { + this.setup.Returns(delegateWithWrongParameterCount); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_parameter_types_but_setup_invocation_will_fail() + { + Func delegateWithWrongParameterType = (arg) => default(IType); + this.setup.Returns(delegateWithWrongParameterType); + + var ex = Record.Exception(() => + { + mock.Object.Method(42); + }); + + Assert.IsType(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()).Returns(obj => ...); + // mock.Setup(m => m.Method(It.IsAny()).Returns(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 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); + } + } +} diff --git a/Source/MethodCall.cs b/Source/MethodCall.cs index 31d55fd71..ab07540c1 100644 --- a/Source/MethodCall.cs +++ b/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")] diff --git a/Source/MethodCallReturn.cs b/Source/MethodCallReturn.cs index c0ac1ea1c..9ea5229b2 100644 --- a/Source/MethodCallReturn.cs +++ b/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 @@ IReturnsThrowsGetter ICallbackGetter.Callback(Ac 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()) diff --git a/Source/Properties/Resources.Designer.cs b/Source/Properties/Resources.Designer.cs index 42397a2c2..f7e874ddb 100644 --- a/Source/Properties/Resources.Designer.cs +++ b/Source/Properties/Resources.Designer.cs @@ -177,6 +177,15 @@ internal static string InvalidCallbackNotADelegateWithReturnTypeVoid { } } + /// + /// Looks up a localized string similar to Invalid callback. Setup on method with {0} parameter(s) cannot invoke callback with different number of parameters ({1}).. + /// + internal static string InvalidCallbackParameterCountMismatch { + get { + return ResourceManager.GetString("InvalidCallbackParameterCountMismatch", resourceCulture); + } + } + /// /// Looks up a localized string similar to Invalid callback. Setup on method with parameters ({0}) cannot invoke callback with parameters ({1}).. /// @@ -186,6 +195,15 @@ internal static string InvalidCallbackParameterMismatch { } } + /// + /// Looks up a localized string similar to Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}).. + /// + internal static string InvalidCallbackReturnTypeMismatch { + get { + return ResourceManager.GetString("InvalidCallbackReturnTypeMismatch", resourceCulture); + } + } + /// /// Looks up a localized string similar to Type to mock must be an interface or an abstract or non-sealed class. . /// diff --git a/Source/Properties/Resources.resx b/Source/Properties/Resources.resx index d80d1b94f..81831732b 100644 --- a/Source/Properties/Resources.resx +++ b/Source/Properties/Resources.resx @@ -365,4 +365,10 @@ Expected invocation on the mock once, but was {4} times: {1} Type {0} does not have matching protected member: {1} + + Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}). + + + Invalid callback. Setup on method with {0} parameter(s) cannot invoke callback with different number of parameters ({1}). + \ No newline at end of file