Merge pull request #948 from stakx/param-arrays-in-recursive-expressions

Match `params` arrays in setup/verification expressions using structural equality

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)
 

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,34 @@ 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)
+				{
+					// 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)
+						{
+							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;

tests/Moq.Tests/InvocationShapeFixture.cs

@@ -0,0 +1,75 @@
+// 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;
+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 => a.Method(1, 2, 3));
+			var snd = ToInvocationShape<A>(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 => b.Method(1, 2, 3));
+			var snd = ToInvocationShape<B>(b => b.Method(1, 2, 3));
+
+			Assert.NotSame(fst, snd);
+			Assert.Equal(fst, snd);
+		}
+
+		[Fact]
+		public void Param_array_args_are_compared_partially_evaluated()
+		{
+			int x = 1;
+
+			var fst = ToInvocationShape<B>(b => b.Method(1, 2, 3));
+			var snd = ToInvocationShape<B>(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<T>(Expression<Action<T>> 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);
+		}
+	}
+}

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<IFoo>();
+			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);