From d682ac0468233809e968a0dc016440bfd51a53c4 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 18 Oct 2019 15:09:28 +0200 Subject: [PATCH 1/4] `params` isn't handled properly in recursive setups Other parts of Moq match `params` arrays not by reference equality but by structural equality (i.e. by comparing their elements). This does not appear to work in recursive expressions. --- tests/Moq.Tests/RecursiveMocksFixture.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/Moq.Tests/RecursiveMocksFixture.cs b/tests/Moq.Tests/RecursiveMocksFixture.cs index 383ee53fc..0e743c574 100644 --- a/tests/Moq.Tests/RecursiveMocksFixture.cs +++ b/tests/Moq.Tests/RecursiveMocksFixture.cs @@ -315,6 +315,16 @@ public void FullMethodInvocationInsideFluentCanUseMatchers() Assert.Equal(5, fooMock.Object.Bar.GetBaz("foo").Value); } + [Fact] + public void Param_array_args_in_setup_expression_parts_are_compared_by_structural_equality_not_reference_equality() + { + var mock = new Mock(); + mock.Setup(m => m.GetBar(1).Value).Returns(1); + mock.Setup(m => m.GetBar(1).OtherValue).Returns(2); + Assert.Equal(1, mock.Object.GetBar(1).Value); + Assert.Equal(2, mock.Object.GetBar(1).OtherValue); + } + public class Verify_can_tell_apart_different_arguments_in_intermediate_part_of_fluent_expressions { [Fact] @@ -418,6 +428,7 @@ public class Foo : IFoo public IBar BarField; public IBar Bar { get; set; } public IBar GetBar() { return null; } + public IBar GetBar(params int[] indices) { return null; } public IBar this[int index] { get { return null; } set { } } public string Do(string command) @@ -432,11 +443,13 @@ public interface IFoo IBar this[int index] { get; set; } string Do(string command); IBar GetBar(); + IBar GetBar(params int[] indices); } public interface IBar { int Value { get; set; } + int OtherValue { get; set; } string Do(string command); IBaz Baz { get; set; } IBaz GetBaz(string value); From 0a8b5b8b86ed279978328872244bf4c8c9c32151 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 18 Oct 2019 15:20:03 +0200 Subject: [PATCH 2/4] Special-case `InvocationShape.Equals` for `params` --- src/Moq/InvocationShape.cs | 24 +++++++++- tests/Moq.Tests/InvocationShapeFixture.cs | 58 +++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 tests/Moq.Tests/InvocationShapeFixture.cs diff --git a/src/Moq/InvocationShape.cs b/src/Moq/InvocationShape.cs index 14ecd775b..c0498947e 100644 --- a/src/Moq/InvocationShape.cs +++ b/src/Moq/InvocationShape.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Linq.Expressions; using System.Reflection; @@ -160,8 +161,29 @@ public bool Equals(InvocationShape other) other.partiallyEvaluatedArguments = PartiallyEvaluateArguments(other.Arguments); } - for (int i = 0, n = this.partiallyEvaluatedArguments.Length; i < n; ++i) + var lastParameter = this.Method.GetParameters().LastOrDefault(); + var lastParameterIsParamArray = lastParameter != null && lastParameter.ParameterType.IsArray && lastParameter.IsDefined(typeof(ParamArrayAttribute)); + + for (int i = 0, li = this.partiallyEvaluatedArguments.Length - 1; i <= li; ++i) { + // Special case for final `params` parameters, which need to be compared by structural equality, + // not array reference equality: + if (i == li && lastParameterIsParamArray) + { + if (this.Arguments[li] is NewArrayExpression e1 && other.Arguments[li] is NewArrayExpression e2 && e1.Expressions.Count == e2.Expressions.Count) + { + for (int j = 0, nj = e1.Expressions.Count; j < nj; ++j) + { + if (!ExpressionComparer.Default.Equals(e1.Expressions[j], e2.Expressions[j])) + { + return false; + } + } + + continue; + } + } + if (!ExpressionComparer.Default.Equals(this.partiallyEvaluatedArguments[i], other.partiallyEvaluatedArguments[i])) { return false; diff --git a/tests/Moq.Tests/InvocationShapeFixture.cs b/tests/Moq.Tests/InvocationShapeFixture.cs new file mode 100644 index 000000000..76caa7d8d --- /dev/null +++ b/tests/Moq.Tests/InvocationShapeFixture.cs @@ -0,0 +1,58 @@ +// Copyright (c) 2007, Clarius Consulting, Manas Technology Solutions, InSTEDD. +// All rights reserved. Licensed under the BSD 3-Clause License; see License.txt. + +using System; +using System.Diagnostics; +using System.Linq.Expressions; + +using Xunit; + +namespace Moq.Tests +{ + public class InvocationShapeFixture + { + // This test is unspectacular but sets the stage for the one following it. See comment below. + [Fact] + public void Regular_parameters_are_compared_using_equality() + { + var fst = ToInvocationShape(a => a.Method(1, 2, 3)); + var snd = ToInvocationShape(a => a.Method(1, 2, 3)); + + Assert.NotSame(fst, snd); + Assert.Equal(fst, snd); + } + + // If you look at just this test code, and not at the definition for `B.Method`, + // you'd rightly expect the same outcome as for the above test, which looks almost the same. + // What we're testing here is that when the compiler silently transforms literal values to + // `params` arrays, we care about their structural equality, not about their differing identities. + [Fact] + public void Param_array_args_are_compared_using_structural_equality_not_reference_equality() + { + var fst = ToInvocationShape(b => b.Method(1, 2, 3)); + var snd = ToInvocationShape(b => b.Method(1, 2, 3)); + + Assert.NotSame(fst, snd); + Assert.Equal(fst, snd); + } + + private static InvocationShape ToInvocationShape(Expression> expression) + { + Debug.Assert(expression != null); + Debug.Assert(expression.Body is MethodCallExpression); + + var methodCall = (MethodCallExpression)expression.Body; + return new InvocationShape(expression, methodCall.Method, methodCall.Arguments); + } + + public interface A + { + void Method(int arg1, int arg2, int arg3); + } + + public interface B + { + void Method(params int[] args); + } + } +} From 73f2a4fdae4b4852f2ac8012d9fef64326af0114 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 18 Oct 2019 22:01:03 +0200 Subject: [PATCH 3/4] Should compare `params` array values partially evaluated --- src/Moq/InvocationShape.cs | 5 +++++ tests/Moq.Tests/InvocationShapeFixture.cs | 17 +++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/Moq/InvocationShape.cs b/src/Moq/InvocationShape.cs index c0498947e..738160e9d 100644 --- a/src/Moq/InvocationShape.cs +++ b/src/Moq/InvocationShape.cs @@ -170,6 +170,11 @@ public bool Equals(InvocationShape other) // not array reference equality: if (i == li && lastParameterIsParamArray) { + // In the following, if we retrieved the `params` arrays via `partiallyEvaluatedArguments`, + // we might see them either as `NewArrayExpression`s or reduced to `ConstantExpression`s. + // By retrieving them via `Arguments` we always see them as non-reduced `NewArrayExpression`s, + // so we don't have to distinguish between two cases. (However, the expressions inside those + // have already been partially evaluated by `MatcherFactory` earlier on!) if (this.Arguments[li] is NewArrayExpression e1 && other.Arguments[li] is NewArrayExpression e2 && e1.Expressions.Count == e2.Expressions.Count) { for (int j = 0, nj = e1.Expressions.Count; j < nj; ++j) diff --git a/tests/Moq.Tests/InvocationShapeFixture.cs b/tests/Moq.Tests/InvocationShapeFixture.cs index 76caa7d8d..f90e4ce62 100644 --- a/tests/Moq.Tests/InvocationShapeFixture.cs +++ b/tests/Moq.Tests/InvocationShapeFixture.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.Linq; using System.Linq.Expressions; using Xunit; @@ -36,6 +37,22 @@ public void Param_array_args_are_compared_using_structural_equality_not_referenc Assert.Equal(fst, snd); } + [Fact] + public void Param_array_args_are_compared_partially_evaluated() + { + int x = 1; + + var fst = ToInvocationShape(b => b.Method(1, 2, 3)); + var snd = ToInvocationShape(b => b.Method(x, 2, 3)); + // ^ + // `x` will be captured and represented in the expression tree as a display class field access: + var xExpr = ((snd.Expression.Body as MethodCallExpression).Arguments.Last() as NewArrayExpression).Expressions.First(); + + Assert.False(xExpr is ConstantExpression); + Assert.NotSame(fst, snd); + Assert.Equal(fst, snd); + } + private static InvocationShape ToInvocationShape(Expression> expression) { Debug.Assert(expression != null); From 0502d338d2bd11eae212ab59104712ffada84308 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 18 Oct 2019 15:35:06 +0200 Subject: [PATCH 4/4] Update the changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f94f34ebc..b6ab533ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 * AmbiguousMatchException when setting up the property, that hides another one (@ishatalkin, #939) * `ArgumentException` ("Interface not found") when setting up `object.ToString` on an interface mock (@vslynko, #942) * Cannot "return" to original mocked type after downcasting with `Mock.Get` and then upcasting with `mock.As<>` (@pjquirk, #943) +* `params` arrays in recursive setup expressions are matched by reference equality instead of by structural equality (@danielcweber, #946) ## 4.13.0 (2019-08-31)