Merge pull request #1234 from stakx/stubbedpropertiessetup

Create dedicated setup type for `SetupAllProperties`

CHANGELOG.md

@@ -5,6 +5,14 @@ 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
+
+* Property setups are ignored on mocks instantiated using `Mock.Of` (@stakx, #1066)
+* `SetupAllProperties` causes mocks to become race-prone (@estrizhok, #1231)
+
+
 ## 4.17.0 (2022-02-13)
 
 #### Added

src/Moq/AsInterface.cs

@@ -43,12 +43,6 @@ public override DefaultValueProvider DefaultValueProvider
 			set => this.owner.DefaultValueProvider = value;
 		}
 
-		internal override DefaultValueProvider AutoSetupPropertiesDefaultValueProvider
-		{
-			get => this.owner.AutoSetupPropertiesDefaultValueProvider;
-			set => this.owner.AutoSetupPropertiesDefaultValueProvider = value;
-		}
-
 		internal override EventHandlerCollection EventHandlers => this.owner.EventHandlers;
 
 		internal override Type[] InheritedInterfaces => this.owner.InheritedInterfaces;

src/Moq/Interception/InterceptionAspects.cs

@@ -3,9 +3,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Linq;
-using System.Linq.Expressions;
 using System.Reflection;
 
 using Moq.Behaviors;
@@ -175,59 +173,6 @@ public static void Handle(Invocation invocation, Mock mock)
 		}
 	}
 
-	internal static class HandleAutoSetupProperties
-	{
-		private static readonly int AccessorPrefixLength = "?et_".Length; // get_ or set_
-
-		public static bool Handle(Invocation invocation, Mock mock)
-		{
-			if (mock.AutoSetupPropertiesDefaultValueProvider == null)
-			{
-				return false;
-			}
-
-			MethodInfo invocationMethod = invocation.Method;
-			if (!invocationMethod.IsPropertyAccessor())
-			{
-				return false;
-			}
-
-			string propertyName = invocationMethod.Name.Substring(AccessorPrefixLength);
-			PropertyInfo property = invocationMethod.DeclaringType.GetProperty(propertyName, Type.EmptyTypes);
-			Debug.Assert(property != null);
-
-			bool accessorFound = property.CanRead(out var getter) | property.CanWrite(out var setter);
-			Debug.Assert(accessorFound);
-
-			var expression = GetPropertyExpression(invocationMethod.DeclaringType, property);
-			var initialValue = getter != null ? CreateInitialPropertyValue(mock, getter) : null;
-			var setup = new StubbedPropertySetup(mock, expression, getter, setter, initialValue);
-			mock.MutableSetups.Add(setup);
-			setup.Execute(invocation);
-
-			return true;
-		}
-
-		private static object CreateInitialPropertyValue(Mock mock, MethodInfo getter)
-		{
-			object initialValue = mock.GetDefaultValue(getter, out Mock innerMock,
-				useAlternateProvider: mock.AutoSetupPropertiesDefaultValueProvider);
-
-			if (innerMock != null)
-			{
-				Mock.SetupAllProperties(innerMock, mock.AutoSetupPropertiesDefaultValueProvider);
-			}
-
-			return initialValue;
-		}
-
-		private static LambdaExpression GetPropertyExpression(Type mockType, PropertyInfo property)
-		{
-			var param = Expression.Parameter(mockType, "m");
-			return Expression.Lambda(Expression.MakeMemberAccess(param, property), param);
-		}
-	}
-
 	internal static class FailForStrictMock
 	{
 		public static void Handle(Invocation invocation, Mock mock)

src/Moq/Interception/Mock.cs

@@ -19,11 +19,6 @@ void IInterceptor.Intercept(Invocation invocation)
 				return;
 			}
 
-			if (HandleAutoSetupProperties.Handle(invocation, this))
-			{
-				return;
-			}
-
 			if (HandleEventSubscription.Handle(invocation, this))
 			{
 				return;

src/Moq/Linq/MockSetupsBuilder.cs

@@ -132,7 +132,7 @@ private static Expression ConvertToSetupReturns(Expression left, Expression righ
 			var rewrittenLeft = v.Visit(left);
 
 			return Expression.Call(
-				Mocks.SetupReturnsMethod,
+				Mock.SetupReturnsMethod,
 				// mock:
 				Expression.Call(
 					Mock.GetMethod.MakeGenericMethod(v.MockObject.Type),

src/Moq/Linq/Mocks.cs

@@ -123,27 +123,5 @@ private static IEnumerable<T> CreateMocks<T>(MockBehavior behavior) where T : cl
 			}
 			while (true);
 		}
-
-		internal static readonly MethodInfo SetupReturnsMethod =
-			typeof(Mocks).GetMethod(nameof(SetupReturns), BindingFlags.NonPublic | BindingFlags.Static);
-
-		internal static bool SetupReturns(Mock mock, LambdaExpression expression, object value)
-		{
-			if (expression.Body is MemberExpression me
-				&& me.Member is PropertyInfo pi
-				&& !(pi.CanRead(out var getter) && getter.CanOverride() && ProxyFactory.Instance.IsMethodVisible(getter, out _))
-				&& pi.CanWrite(out _))
-			{
-				// LINQ to Mocks allows setting non-interceptable properties, which is handy e.g. when initializing DTOs.
-				Mock.SetupSet(mock, expression, propertyToSet: pi, value);
-			}
-			else
-			{
-				var setup = Mock.Setup(mock, expression, condition: null);
-				setup.SetReturnValueBehavior(value);
-			}
-
-			return true;
-		}
 	}
 }

src/Moq/Mock.cs

@@ -209,13 +209,6 @@ public DefaultValue DefaultValue
 		/// </summary>
 		public abstract DefaultValueProvider DefaultValueProvider { get; set; }
 
-		/// <summary>
-		/// The <see cref="Moq.DefaultValueProvider"/> used to initialize automatically stubbed properties.
-		/// It is equal to the value of <see cref="DefaultValueProvider"/> at the time when
-		/// <see cref="SetupAllProperties"/> was last called.
-		/// </summary>
-		internal abstract DefaultValueProvider AutoSetupPropertiesDefaultValueProvider { get; set; } 
-
 		internal abstract SetupCollection MutableSetups { get; }
 
 		/// <summary>
@@ -531,36 +524,61 @@ internal static MethodCall SetupSet(Mock mock, LambdaExpression expression, Cond
 			return Mock.Setup(mock, expression, condition);
 		}
 
-		// This specialized version of `SetupSet` exists to let `Mock.Of` support properties that are not overridable.
-		// Note that we generally prefer having a setup for a property's return value, but in this case, that isn't possible.
-		internal static void SetupSet(Mock mock, LambdaExpression expression, PropertyInfo propertyToSet, object value)
+		internal static readonly MethodInfo SetupReturnsMethod =
+			typeof(Mock).GetMethod(nameof(SetupReturns), BindingFlags.NonPublic | BindingFlags.Static);
+
+		// This specialized setup method is used to set up a single `Mock.Of` predicate.
+		// Unlike other setup methods, LINQ to Mocks can set non-interceptable properties, which is handy when initializing DTOs.
+		internal static bool SetupReturns(Mock mock, LambdaExpression expression, object value)
 		{
 			Guard.NotNull(expression, nameof(expression));
 
-			Mock.SetupRecursive<MethodCall>(mock, expression, setupLast: (targetMock, _, __) =>
+			Mock.SetupRecursive<MethodCall>(mock, expression, setupLast: (targetMock, oe, part) =>
 			{
-				// Setting a mock's property through reflection will only work (i.e. the property will only remember the value
-				// it's being set to) if it is being stubbed. In order to ensure it's stubbed, we temporarily enable
-				// auto-stubbing (if that isn't already switched on).
+				var originalExpression = (LambdaExpression)oe;
 
-				var temporaryAutoSetupProperties = targetMock.AutoSetupPropertiesDefaultValueProvider == null;
-				if (temporaryAutoSetupProperties)
-				{
-					targetMock.AutoSetupPropertiesDefaultValueProvider = targetMock.DefaultValueProvider;
-				}
-				try
-				{
-					propertyToSet.SetValue(targetMock.Object, value, null);
-				}
-				finally
+				// There are two special cases involving settable properties where we do something other than creating a new setup:
+
+				if (originalExpression.IsProperty())
 				{
-					if (temporaryAutoSetupProperties)
+					var pi = originalExpression.ToPropertyInfo();
+					if (pi.CanWrite(out var setter))
 					{
-						targetMock.AutoSetupPropertiesDefaultValueProvider = null;
+						if (pi.CanRead(out var getter) && getter.CanOverride() && ProxyFactory.Instance.IsMethodVisible(getter, out _))
+						{
+							if (setter.CanOverride() && ProxyFactory.Instance.IsMethodVisible(setter, out _)
+								&& targetMock.MutableSetups.FindLast(s => s is StubbedPropertiesSetup) is StubbedPropertiesSetup sps)
+							{
+								// (a) We have a mock where `SetupAllProperties` was called, and the property can be fully stubbed.
+								//     (A property can be "fully stubbed" if both its accessors can be intercepted.)
+								//     In this case, we set the property's internal backing field directly on the setup.
+								sps.SetProperty(pi.Name, value);
+								return null;
+							}
+						}
+						else
+						{
+							// (b) The property is settable, but Moq is unable to intercept the getter,
+							//     so setting up the setter would be pointless and the property also cannot be fully stubbed.
+							//     In this case, the best thing we can do is to simply invoke the setter.
+							pi.SetValue(targetMock.Object, value, null);
+							return null;
+						}
 					}
 				}
+
+				// For all other cases, we create a regular setup.
+
+				Guard.IsOverridable(part.Method, part.Expression);
+				Guard.IsVisibleToProxyFactory(part.Method);
+
+				var setup = new MethodCall(originalExpression, targetMock, condition: null, expectation: part);
+				setup.SetReturnValueBehavior(value);
+				targetMock.MutableSetups.Add(setup);
 				return null;
 			}, allowNonOverridableLastProperty: true);
+
+			return true;
 		}
 
 		internal static MethodCall SetupAdd(Mock mock, LambdaExpression expression, Condition condition)
@@ -637,21 +655,7 @@ private static TSetup SetupRecursive<TSetup>(Mock mock, LambdaExpression origina
 
 		internal static void SetupAllProperties(Mock mock)
 		{
-			SetupAllProperties(mock, mock.DefaultValueProvider);
-		}
-
-		internal static void SetupAllProperties(Mock mock, DefaultValueProvider defaultValueProvider)
-		{
-			mock.MutableSetups.RemoveAllPropertyAccessorSetups();
-			// Removing all the previous properties setups to keep the behaviour of overriding
-			// existing setups in `SetupAllProperties`.
-
-			mock.AutoSetupPropertiesDefaultValueProvider = defaultValueProvider;
-			// `SetupAllProperties` no longer performs properties setup like in previous versions.
-			// Instead it just enables a switch to setup properties on-demand at the moment of first access.
-			// In order for `SetupAllProperties`'s new mode of operation to be indistinguishable
-			// from how it worked previously, it's important to capture the default value provider at this precise
-			// moment, since it might be changed later (before queries to properties).
+			mock.MutableSetups.Add(new StubbedPropertiesSetup(mock));
 		}
 
 		#endregion

src/Moq/MockDefaultValueProvider.cs

@@ -36,7 +36,10 @@ protected override object GetFallbackDefaultValue(Type type, Mock mock)
 				var mockType = typeof(Mock<>).MakeGenericType(type);
 				Mock newMock = (Mock)Activator.CreateInstance(mockType, mock.Behavior);
 				newMock.DefaultValueProvider = mock.DefaultValueProvider;
-				newMock.AutoSetupPropertiesDefaultValueProvider = mock.AutoSetupPropertiesDefaultValueProvider;
+				if (mock.MutableSetups.FindLast(s => s is StubbedPropertiesSetup) is StubbedPropertiesSetup sts)
+				{
+					newMock.MutableSetups.Add(new StubbedPropertiesSetup(newMock, sts.DefaultValueProvider));
+				}
 				if(!type.IsDelegateType())
 				{
 					newMock.CallBase = mock.CallBase;

src/Moq/Mock`1.cs

@@ -265,8 +265,6 @@ public override DefaultValueProvider DefaultValueProvider
 			set => this.defaultValueProvider = value ?? throw new ArgumentNullException(nameof(value));
 		}
 
-		internal override DefaultValueProvider AutoSetupPropertiesDefaultValueProvider { get; set; }
-
 		internal override EventHandlerCollection EventHandlers => this.eventHandlers;
 
 		internal override List<Type> AdditionalInterfaces => this.additionalInterfaces;

src/Moq/SetupCollection.cs

@@ -4,7 +4,6 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
-using System.Linq;
 
 namespace Moq
 {
@@ -72,26 +71,6 @@ private void MarkOverriddenSetups()
 			}
 		}
 
-		public void RemoveAllPropertyAccessorSetups()
-		{
-			// Fast path (no `lock`) when there are no setups:
-			if (this.setups.Count == 0)
-			{
-				return;
-			}
-
-			lock (this.setups)
-			{
-				this.setups.RemoveAll(s => s is StubbedPropertySetup || (s is MethodSetup ms && ms.Method.IsPropertyAccessor()));
-
-				// NOTE: In the general case, removing a setup means that some overridden setups might no longer
-				// be shadowed, and their `IsOverridden` flag should go back from `true` to `false`.
-				//
-				// In this particular case however, we don't need to worry about this because we are categorically
-				// removing all property accessors, and they could only have overridden other property accessors.
-			}
-		}
-
 		public void Clear()
 		{
 			lock (this.setups)