Merge pull request #950 from stakx/split-properties

Ensure `SetupAllProperties` stubs all accessors of partially overridden properties

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 
 #### Fixed
 
+* `SetupAllProperties` does not recognize property as read-write if only setter is overridden (@stakx, #886)
 * Regression: `InvalidCastException` caused by Moq erroneously reusing a cached auto-mocked (`DefaultValue.Mock`) return value for a different generic method instantiation (@BrunoJuchli, #932)
 * 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)

src/Moq/Extensions.cs

@@ -19,6 +19,78 @@ public static bool CanCreateInstance(this Type type)
 			return type.IsValueType || type.GetConstructor(Type.EmptyTypes) != null;
 		}
 
+		public static bool CanRead(this PropertyInfo property, out MethodInfo getter)
+		{
+			if (property.CanRead)
+			{
+				// The given `PropertyInfo` should be able to provide a getter:
+				getter = property.GetGetMethod(nonPublic: true);
+				Debug.Assert(getter != null);
+				return true;
+			}
+			else
+			{
+				// The given `PropertyInfo` cannot provide a getter... but there may still be one in a base class'
+				// corresponding `PropertyInfo`! We need to find that base `PropertyInfo`, and because `PropertyInfo`
+				// does not have `.GetBaseDefinition()`, we'll find it via the setter's `.GetBaseDefinition()`.
+				// (We may assume that there's a setter because properties/indexers must have at least one accessor.)
+				Debug.Assert(property.CanWrite);
+				var setter = property.GetSetMethod(nonPublic: true);
+				Debug.Assert(setter != null);
+
+				var baseSetter = setter.GetBaseDefinition();
+				if (baseSetter != setter)
+				{
+					var baseProperty =
+						baseSetter
+						.DeclaringType
+						.GetMember(property.Name, MemberTypes.Property, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
+						.Cast<PropertyInfo>()
+						.First(p => p.GetSetMethod(nonPublic: true) == baseSetter);
+					return baseProperty.CanRead(out getter);
+				}
+			}
+
+			getter = null;
+			return false;
+		}
+
+		public static bool CanWrite(this PropertyInfo property, out MethodInfo setter)
+		{
+			if (property.CanWrite)
+			{
+				// The given `PropertyInfo` should be able to provide a setter:
+				setter = property.GetSetMethod(nonPublic: true);
+				Debug.Assert(setter != null);
+				return true;
+			}
+			else
+			{
+				// The given `PropertyInfo` cannot provide a setter... but there may still be one in a base class'
+				// corresponding `PropertyInfo`! We need to find that base `PropertyInfo`, and because `PropertyInfo`
+				// does not have `.GetBaseDefinition()`, we'll find it via the getter's `.GetBaseDefinition()`.
+				// (We may assume that there's a getter because properties/indexers must have at least one accessor.)
+				Debug.Assert(property.CanRead);
+				var getter = property.GetGetMethod(nonPublic: true);
+				Debug.Assert(getter != null);
+
+				var baseGetter = getter.GetBaseDefinition();
+				if (baseGetter != getter)
+				{
+					var baseProperty =
+						baseGetter
+						.DeclaringType
+						.GetMember(property.Name, MemberTypes.Property, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
+						.Cast<PropertyInfo>()
+						.First(p => p.GetGetMethod(nonPublic: true) == baseGetter);
+					return baseProperty.CanWrite(out setter);
+				}
+			}
+
+			setter = null;
+			return false;
+		}
+
 		/// <summary>
 		///   Gets the default value for the specified type. This is the Reflection counterpart of C#'s <see langword="default"/> operator.
 		/// </summary>

src/Moq/Interception/InterceptionAspects.cs

@@ -314,9 +314,8 @@ public static bool Handle(Invocation invocation, Mock mock)
 				object propertyValue;
 
 				Setup getterSetup = null;
-				if (property.CanRead)
+				if (property.CanRead(out var getter))
 				{
-					var getter = property.GetGetMethod(true);
 					if (ProxyFactory.Instance.IsMethodVisible(getter, out _))
 					{
 						propertyValue = CreateInitialPropertyValue(mock, getter);
@@ -336,9 +335,8 @@ public static bool Handle(Invocation invocation, Mock mock)
 				}
 
 				Setup setterSetup = null;
-				if (property.CanWrite)
+				if (property.CanWrite(out var setter))
 				{
-					MethodInfo setter = property.GetSetMethod(nonPublic: true);
 					if (ProxyFactory.Instance.IsMethodVisible(setter, out _))
 					{
 						setterSetup = new AutoImplementedPropertySetterSetup(expression, setter, (newValue) =>

tests/Moq.Tests/ExtensionsFixture.cs

@@ -118,6 +118,56 @@ private string GetParameterTypeList(string methodName)
 			return method.GetParameterTypeList();
 		}
 
+		[Fact]
+		public void CanRead_returns_false_for_true_write_only_property()
+		{
+			var property = typeof(WithWriteOnlyProperty).GetProperty("Property");
+			Assert.False(property.CanRead(out var getter));
+			Assert.Null(getter);
+		}
+
+		[Fact]
+		public void CanRead_identifies_getter_in_true_read_only_property()
+		{
+			var property = typeof(WithReadOnlyProperty).GetProperty("Property");
+			Assert.True(property.CanRead(out var getter));
+			Assert.Equal(typeof(WithReadOnlyProperty), getter.DeclaringType);
+		}
+
+		[Fact]
+		public void CanRead_identifies_getter_when_declared_in_base_class()
+		{
+			var property = typeof(OverridesOnlySetter).GetProperty("Property");
+			Assert.False(property.CanRead);
+			Assert.True(property.CanRead(out var getter));
+			Assert.Equal(typeof(WithAutoProperty), getter.DeclaringType);
+		}
+
+		[Fact]
+		public void CanWrite_returns_false_for_true_read_only_property()
+		{
+			var property = typeof(WithReadOnlyProperty).GetProperty("Property");
+			Assert.False(property.CanWrite(out var setter));
+			Assert.Null(setter);
+		}
+
+		[Fact]
+		public void CanWrite_identifies_setter_in_true_write_only_property()
+		{
+			var property = typeof(WithWriteOnlyProperty).GetProperty("Property");
+			Assert.True(property.CanWrite(out var setter));
+			Assert.Equal(typeof(WithWriteOnlyProperty), setter.DeclaringType);
+		}
+
+		[Fact]
+		public void CanWrite_identifies_setter_when_declared_in_base_class()
+		{
+			var property = typeof(OverridesOnlyGetter).GetProperty("Property");
+			Assert.False(property.CanWrite);
+			Assert.True(property.CanWrite(out var setter));
+			Assert.Equal(typeof(WithAutoProperty), setter.DeclaringType);
+		}
+
 		public interface IMethods
 		{
 			void Empty();
@@ -128,6 +178,31 @@ public interface IMethods
 			void OutInt(out int arg1);
 			void BoolAndParamsString(bool arg1, params string[] arg2);
 		}
+
+		public class WithReadOnlyProperty
+		{
+			public virtual object Property => null;
+		}
+
+		public class WithWriteOnlyProperty
+		{
+			public virtual object Property { set { } }
+		}
+
+		public class WithAutoProperty
+		{
+			public virtual object Property { get; set; }
+		}
+
+		public class OverridesOnlyGetter : WithAutoProperty
+		{
+			public override object Property { get => base.Property; }
+		}
+
+		public class OverridesOnlySetter : WithAutoProperty
+		{
+			public override object Property { set => base.Property = value; }
+		}
 	}
 
 	public interface IFooReset

tests/Moq.Tests/StubExtensionsFixture.cs

@@ -258,6 +258,15 @@ public void SetupAllProperties_should_override_regular_setups()
 			Assert.Equal(0, mock.Object.Value);
 		}
 
+		[Fact]
+		public void SetupAllProperties_retains_value_of_derived_read_write_property_that_overrides_only_setter()
+		{
+			var mock = new Mock<OverridesOnlySetter>();
+			mock.SetupAllProperties();
+			mock.Object.Property = "value";
+			Assert.Equal("value", mock.Object.Property);
+		}
+
 		private object GetValue() { return new object(); }
 
 		public interface IFoo
@@ -298,5 +307,15 @@ public interface IHierarchy
 		{
 			IHierarchy Hierarchy { get; set; }
 		}
+
+		public class WithAutoProperty
+		{
+			public virtual object Property { get; set; }
+		}
+
+		public class OverridesOnlySetter : WithAutoProperty
+		{
+			public override object Property { set => base.Property = value; }
+		}
 	}
 }