Merge pull request #497 from stakx/setup-sealed-methods

Improve `Setup` recognition logic for sealed methods

CHANGELOG.md

@@ -22,6 +22,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 * Update a method's invocation count correctly, even when it is set up to throw an exception (@stakx, #473)
 * Sequences set up with `SetupSequence` are now thread-safe (@stakx, #476)
 * Record calls to methods that are named like event accessors (`add_X`, `remove_X`) so they can be verified (@stakx, #488)
+* Improve recognition logic for sealed methods so that `Setup` throws when an attempt is made to set one up (@stakx, #497)
 
 ## 4.7.142 (2017-10-11)
 

Moq.Tests/ExpressionExtensionsFixture.cs

@@ -114,10 +114,51 @@ public void ToMethodCallConvertsLambda()
 			Assert.Equal(typeof(IFoo).GetMethod("Do"), expr.ToMethodCall().Method);
 		}
 
+		// this doesn't test Moq, but documents a peculiarity of the C# compiler.
+		// once this test starts failing, verify whether the PropertyInfo "correction" applied
+		// in the `expression.ToPropertyInfo()` extension method is still required.
 		[Fact]
-		public void ToPropertyInfoConvertsExpression()
+		public void Compilers_put_wrong_PropertyInfo_into_MemberExpression_for_overridden_properties()
 		{
-
+			Expression<Func<Derived, object>> expression = derived => derived.Property;
+
+			var expected = typeof(Base).GetProperty(nameof(Base.Property));
+			var actual = (expression.Body as MemberExpression).Member as PropertyInfo;
+
+			Assert.Same(expected, actual);
+		}
+
+		[Fact]
+		public void ToPropertyInfo_corrects_wrong_DeclaringType_for_overridden_properties()
+		{
+			Expression<Func<Derived, object>> expression = derived => derived.Property;
+
+			var expected = typeof(Derived).GetProperty(nameof(Derived.Property));
+			var actual = expression.ToPropertyInfo();
+
+			Assert.Same(expected, actual);
+		}
+
+		[Fact]
+		public void ToPropertyInfo_does_not_correct_DeclaringType_for_upcast_overridden_properties()
+		{
+			Expression<Func<Derived, object>> expression = derived => ((Base)derived).Property;
+
+			var expected = typeof(Base).GetProperty(nameof(Base.Property));
+			var actual = expression.ToPropertyInfo();
+
+			Assert.Same(expected, actual);
+		}
+
+		[Fact]
+		public void ToPropertyInfo_does_not_correct_DeclaringType_for_base_properties()
+		{
+			Expression<Func<Base, object>> expression = @base => @base.Property;
+
+			var expected = typeof(Base).GetProperty(nameof(Base.Property));
+			var actual = expression.ToPropertyInfo();
+
+			Assert.Same(expected, actual);
 		}
 
 		private Expression ToExpression<T>(Expression<Func<T>> expression)
@@ -146,5 +187,15 @@ public interface IFoo
 			void Do();
 			object this[int index] { get; set; }
 		}
+
+		public abstract class Base
+		{
+			public abstract object Property { get; }
+		}
+
+		public class Derived : Base
+		{
+			public override sealed object Property => null;
+		}
 	}
 }

Moq.Tests/PropertiesFixture.cs

@@ -91,5 +91,98 @@ public void ShouldSetIndexerWithValueMatcher()
 		//
 		//	foo.Object[18] = "foo";
 		//}
+
+		[Fact]
+		public void Can_Setup_virtual_property()
+		{
+			// verify our assumptions that A is indeed virtual (and non-sealed):
+			var propertyGetter = typeof(Foo).GetProperty("A").GetGetMethod();
+			Assert.True(propertyGetter.IsVirtual);
+			Assert.False(propertyGetter.IsFinal);
+
+			var mock = new Mock<Foo>();
+			mock.Setup(m => m.A).Returns("mocked A");
+
+			var a = mock.Object.A;
+
+			Assert.Equal("mocked A", a);
+		}
+
+		[Fact]
+		public void Can_Setup_virtual_property_that_implicitly_implements_a_property_from_inaccessible_interface()
+		{
+			// verify our assumptions that C is indeed virtual (and non-sealed):
+			var propertyGetter = typeof(Foo).GetProperty("C").GetGetMethod();
+			Assert.True(propertyGetter.IsVirtual);
+			Assert.False(propertyGetter.IsFinal);
+
+			var mock = new Mock<Foo>();
+			mock.Setup(m => m.C).Returns("mocked C");
+
+			var c = mock.Object.C;
+
+			Assert.Equal("mocked C", c);
+		}
+
+		[Fact]
+		public void Cannot_Setup_virtual_but_sealed_property()
+		{
+			// verify our assumptions that B is indeed virtual and sealed:
+			var propertyGetter = typeof(Foo).GetProperty("B").GetGetMethod();
+			Assert.True(propertyGetter.IsVirtual);
+			Assert.True(propertyGetter.IsFinal);
+
+			var mock = new Mock<Foo>();
+
+			var exception = Record.Exception(() =>
+			{
+				mock.Setup(m => m.B).Returns("mocked B");
+			});
+			var b = mock.Object.B;
+
+			Assert.NotEqual("mocked B", b); // it simply shouldn't be possible for Moq to intercept a sealed property;
+			Assert.NotNull(exception);      // and Moq should tell us by throwing an exception.
+		}
+
+
+		[Fact]
+		public void Cannot_Setup_virtual_but_sealed_property_that_implicitly_implements_a_property_from_inaccessible_interface()
+		{
+			// verify our assumptions that D is indeed virtual and sealed:
+			var propertyGetter = typeof(Foo).GetProperty("D").GetGetMethod();
+			Assert.True(propertyGetter.IsVirtual);
+			Assert.True(propertyGetter.IsFinal);
+
+			var mock = new Mock<Foo>();
+
+			var exception = Record.Exception(() =>
+			{
+				mock.Setup(m => m.D).Returns("mocked D");
+			});
+			var d = mock.Object.D;
+
+			Assert.NotEqual("mocked D", d); // it simply shouldn't be possible for Moq to intercept a sealed property;
+			Assert.NotNull(exception);      // and Moq should tell us by throwing an exception.
+		}
+
+		public abstract class FooBase
+		{
+			public abstract object A { get; }
+			public abstract object B { get; }
+		}
+
+		internal interface IFooInternals
+		{
+			object C { get; }
+			object D { get; }
+		}
+
+		public abstract class Foo : FooBase, IFooInternals
+		{
+			public override object A => "A";
+			public sealed override object B => "B";
+			public virtual object C => "C";
+			public object D => "D";
+		}
 	}
 }

Source/ExpressionExtensions.cs

@@ -99,12 +99,24 @@ public static MethodCallExpression ToMethodCall(this LambdaExpression expression
 		/// </summary>
 		public static PropertyInfo ToPropertyInfo(this LambdaExpression expression)
 		{
-			var prop = expression.Body as MemberExpression;
-			if (prop != null)
+			if (expression.Body is MemberExpression prop)
 			{
-				var info = prop.Member as PropertyInfo;
-				if (info != null)
+				if (prop.Member is PropertyInfo info)
 				{
+					// the following block is required because .NET compilers put the wrong PropertyInfo into MemberExpression
+					// for properties originally declared in base classes; they will put the base class' PropertyInfo into
+					// the expression. we attempt to correct this here by checking whether the type of the accessed object
+					// has a property by the same name whose base definition equals the property in the expression; if so,
+					// we "upgrade" to the derived property.
+					if (info.DeclaringType != prop.Expression.Type && info.CanRead)
+					{
+						var propertyInLeft = prop.Expression.Type.GetProperty(info.Name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
+						if (propertyInLeft != null && propertyInLeft.GetMethod.GetBaseDefinition() == info.GetMethod)
+						{
+							info = propertyInLeft;
+						}
+					}
+
 					return info;
 				}
 			}