From 0176fa075e50505193492ef5fe4cae850a2609ae Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 30 Jun 2020 23:30:49 +0200 Subject: [PATCH 1/3] `VerifySet` fails for write-only indexers --- .../Regressions/IssueReportsFixture.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs b/tests/Moq.Tests/Regressions/IssueReportsFixture.cs index 084489238..83014f9fd 100644 --- a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs +++ b/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(); + mock.Object["key"] = "value"; + mock.VerifySet(m => m["key"] = It.IsAny(), Times.Once); + } + + public interface ITest + { + string this[string key] { set; } + } + } + #endregion // Old @ Google Code From 9351aba78f6288864e8059769d8222ae6f9b1f78 Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 30 Jun 2020 23:33:24 +0200 Subject: [PATCH 2/3] Prevent argument count mismatch The expression splitting logic is a little hacky when it comes to assignments to property or indexer setters: first, a split operation is performed as if we were looking at a property or indexer *getter*; the `InvocationShape` receives the correct method, but is one argument short. Once that first `InvocationShape` has been built, we can create a second, patched and fully correct version that includes the assign- ment (RHS) argument. This commit relaxes validation rules for the first `InvocationShape` (i.e. the one with the potential argument count mismatch). --- src/Moq/ExpressionExtensions.cs | 30 +++++++++++++++--------------- src/Moq/InvocationShape.cs | 6 +++--- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Moq/ExpressionExtensions.cs b/src/Moq/ExpressionExtensions.cs index 798d54919..1ce677f1e 100644 --- a/src/Moq/ExpressionExtensions.cs +++ b/src/Moq/ExpressionExtensions.cs @@ -162,7 +162,7 @@ internal static Stack 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; } diff --git a/src/Moq/InvocationShape.cs b/src/Moq/InvocationShape.cs index cb3ee4925..14d28603a 100644 --- a/src/Moq/InvocationShape.cs +++ b/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 arguments = null, bool exactGenericTypeArguments = false) + public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList 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; From f32a129a2987d278a42ec26bf32bafbadba41928 Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 30 Jun 2020 23:44:51 +0200 Subject: [PATCH 3/3] Update the changelog --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bfd33f9e..53dc3d31c 100644 --- a/CHANGELOG.md +++ b/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