Merge pull request #1037 from moq/write-only-indexer-assignment

Fix `NullReferenceException` when using write-only indexer with `VerifySet`

CHANGELOG.md

@@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file.
 The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 
+## Unreleased
+
+#### Fixed
+
+* Regression since version 4.11.0: `VerifySet` fails with `NullReferenceException` for write-only indexers (@Epicycle23, #1036)
+
+
 ## 4.14.4 (2020-06-24)
 
 #### Fixed

src/Moq/ExpressionExtensions.cs

@@ -162,7 +162,7 @@ internal static Stack<InvocationShape> Split(this LambdaExpression expression)
 						remainder.ToStringFixed()));
 			}
 
-			void Split(Expression e, out Expression r /* remainder */, out InvocationShape p /* part */)
+			void Split(Expression e, out Expression r /* remainder */, out InvocationShape p /* part */, bool assignment = false)
 			{
 				const string ParameterName = "...";
 
@@ -173,7 +173,7 @@ void Split(Expression e, out Expression r /* remainder */, out InvocationShape p
 					case ExpressionType.SubtractAssign:  // unsubscription of event handler from event
 					{
 						var assignmentExpression = (BinaryExpression)e;
-						Split(assignmentExpression.Left, out r, out var lhs);
+						Split(assignmentExpression.Left, out r, out var lhs, assignment: true);
 						PropertyInfo property;
 						if (lhs.Expression.Body is MemberExpression me)
 						{
@@ -186,7 +186,7 @@ void Split(Expression e, out Expression r /* remainder */, out InvocationShape p
 							property = ((IndexExpression)lhs.Expression.Body).Indexer;
 						}
 						var parameter = Expression.Parameter(r.Type, r is ParameterExpression ope ? ope.Name : ParameterName);
-						var arguments = new Expression[lhs.Arguments.Count + 1];
+						var arguments = new Expression[lhs.Method.GetParameters().Length];
 						for (var ai = 0; ai < arguments.Length - 1; ++ai)
 						{
 							arguments[ai] = lhs.Arguments[ai];
@@ -196,7 +196,7 @@ void Split(Expression e, out Expression r /* remainder */, out InvocationShape p
 							expression: Expression.Lambda(
 								Expression.MakeBinary(e.NodeType, lhs.Expression.Body, assignmentExpression.Right),
 								parameter),
-							method: property.GetSetMethod(true),
+							method: lhs.Method,
 							arguments);
 						return;
 					}
@@ -255,12 +255,16 @@ void Split(Expression e, out Expression r /* remainder */, out InvocationShape p
 						var parameter = Expression.Parameter(r.Type, r is ParameterExpression ope ? ope.Name : ParameterName);
 						var indexer = indexExpression.Indexer;
 						var arguments = indexExpression.Arguments;
+						var method = !assignment && indexer.CanRead(out var getter)  ? getter
+						           :                indexer.CanWrite(out var setter) ? setter
+						           :                                                   null;
 						p = new InvocationShape(
 									expression: Expression.Lambda(
 										Expression.MakeIndex(parameter, indexer, arguments),
 										parameter),
-									method: indexer.GetGetMethod(true),
-									arguments);
+									method,
+									arguments,
+									skipMatcherInitialization: assignment);
 						return;
 					}
 
@@ -287,19 +291,15 @@ void Split(Expression e, out Expression r /* remainder */, out InvocationShape p
 						r = memberAccessExpression.Expression;
 						var parameter = Expression.Parameter(r.Type, r is ParameterExpression ope ? ope.Name : ParameterName);
 						var property = memberAccessExpression.GetReboundProperty();
-						var method = property.CanRead(out var getter) ? getter : property.GetSetMethod(true);
-						//                    ^^^^^^^                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-						// We're in the switch case block for property read access, therefore we prefer the
-						// getter. When a read-write property is being assigned to, we end up here, too, and
-						// select the wrong accessor. However, that doesn't matter because it will be over-
-						// ridden in the above `Assign` case. Finally, if a write-only property is being
-						// assigned to, we fall back to the setter here in order to not end up without a
-						// method at all.
+						var method = !assignment && property.CanRead(out var getter)  ? getter
+						           :                property.CanWrite(out var setter) ? setter
+						           :                                                    null;
 						p = new InvocationShape(
 									expression: Expression.Lambda(
 										Expression.MakeMemberAccess(parameter, property),
 										parameter),
-									method);
+									method,
+									skipMatcherInitialization: assignment);
 						return;
 					}
 

src/Moq/InvocationShape.cs

@@ -72,7 +72,7 @@ public static InvocationShape CreateFrom(Invocation invocation)
 #endif
 		private readonly bool exactGenericTypeArguments;
 
-		public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList<Expression> arguments = null, bool exactGenericTypeArguments = false)
+		public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList<Expression> arguments = null, bool exactGenericTypeArguments = false, bool skipMatcherInitialization = false)
 		{
 			Debug.Assert(expression != null);
 			Debug.Assert(method != null);
@@ -82,14 +82,14 @@ public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnly
 
 			this.Expression = expression;
 			this.Method = method;
-			if (arguments != null)
+			if (arguments != null && !skipMatcherInitialization)
 			{
 				(this.argumentMatchers, this.Arguments) = MatcherFactory.CreateMatchers(arguments, method.GetParameters());
 			}
 			else
 			{
 				this.argumentMatchers = noArgumentMatchers;
-				this.Arguments = noArguments;
+				this.Arguments = arguments ?? noArguments;
 			}
 
 			this.exactGenericTypeArguments = exactGenericTypeArguments;

tests/Moq.Tests/Regressions/IssueReportsFixture.cs

@@ -3405,6 +3405,26 @@ public interface IDataStore
 		}
 
 
+		#endregion
+
+		#region 1036
+
+		public class Issue1036
+		{
+			[Fact]
+			public void VerifySet_for_assignment_to_write_only_property()
+			{
+				var mock = new Mock<ITest>();
+				mock.Object["key"] = "value";
+				mock.VerifySet(m => m["key"] = It.IsAny<string>(), Times.Once);
+			}
+
+			public interface ITest
+			{
+				string this[string key] { set; }
+			}
+		}
+
 		#endregion
 
 		// Old @ Google Code