Ensure incorrect implementations of ISerializable are caught properly

Source/Extensions.cs

@@ -165,34 +165,6 @@ public static bool IsMockeable(this Type typeToMock)
 			return typeToMock.GetTypeInfo().IsInterface || typeToMock.GetTypeInfo().IsAbstract || typeToMock.IsDelegate() || (typeToMock.GetTypeInfo().IsClass && !typeToMock.GetTypeInfo().IsSealed);
 		}
 
-		public static bool IsSerializableMockable(this Type typeToMock)
-		{
-			return typeToMock.ContainsDeserializationConstructor() && typeToMock.IsGetObjectDataVirtual();
-		}
-
-		private static bool IsGetObjectDataVirtual(this Type typeToMock)
-		{
-#if NETCORE
-			return false;
-#else
-			var getObjectDataMethod = typeToMock.GetInterfaceMap(typeof (ISerializable)).TargetMethods[0];
-			return !getObjectDataMethod.IsPrivate && getObjectDataMethod.IsVirtual && !getObjectDataMethod.IsFinal;
-#endif
-		}
-
-		private static bool ContainsDeserializationConstructor(this Type typeToMock)
-		{
-#if NETCORE
-			return false;
-#else
-			return typeToMock.GetConstructor(
-				BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic,
-				null,
-				new[] {typeof (SerializationInfo), typeof (StreamingContext)},
-				null) != null;
-#endif
-		}
-
 		public static bool CanOverride(this MethodBase method)
 		{
 			return method.IsVirtual && !method.IsFinal && !method.IsPrivate;

Source/Mock.cs

@@ -746,13 +746,15 @@ private static object GetInitialValue(IDefaultValueProvider valueProvider, Stack
 				// to deal with loops in the property graph
 				valueProvider = new EmptyDefaultValueProvider();
 			}
+#if !NETCORE
 			else
 			{
-				// to make sure that properties of types that don't impelemt ISerializable properly (Castle throws ArgumentException)
+				// to make sure that properties of types that don't implement ISerializable properly (Castle throws ArgumentException)
 				// are mocked with default value instead.
 				// It will only result in exception if the properties are accessed.
 				valueProvider = new SerializableTypesValueProvider(valueProvider);
 			}
+#endif
 			return valueProvider.ProvideDefault(property.GetGetMethod());
 		}
 

Source/SerializableTypesValueProvider.cs

@@ -1,23 +1,16 @@
-using System.Reflection;
-#if !NETCORE
+#if !NETCORE
+
+using System;
+using System.Reflection;
 using System.Runtime.Serialization;
-#endif
 
 namespace Moq
 {
-#if !NETCORE
 	/// <summary>
 	/// A <see cref="IDefaultValueProvider"/> that returns an empty default value 
 	/// for serializable types that do not implement <see cref="ISerializable"/> properly, 
 	/// and returns the value provided by the decorated provider otherwise.
 	/// </summary>
-#else
-	/// <summary>
-	/// A <see cref="IDefaultValueProvider"/> that returns an empty default value 
-	/// for serializable types that do not implement ISerializable properly, 
-	/// and returns the value provided by the decorated provider otherwise.
-	/// </summary>
-#endif
 	internal class SerializableTypesValueProvider : IDefaultValueProvider
 	{
 		private readonly IDefaultValueProvider decorated;
@@ -35,9 +28,33 @@ public void DefineDefault<T>(T value)
 
 		public object ProvideDefault(MethodInfo member)
 		{
-			return !member.ReturnType.GetTypeInfo().IsSerializable || member.ReturnType.IsSerializableMockable()
-				? decorated.ProvideDefault(member)
-				: emptyDefaultValueProvider.ProvideDefault(member);
+			return IsSerializableWithIncorrectImplementationForISerializable(member.ReturnType)
+				? emptyDefaultValueProvider.ProvideDefault(member)
+				: decorated.ProvideDefault(member);
+		}
+
+		private static bool IsSerializableWithIncorrectImplementationForISerializable(Type typeToMock)
+		{
+			return typeToMock.IsSerializable
+				&& typeof(ISerializable).IsAssignableFrom(typeToMock)
+				&& !(ContainsDeserializationConstructor(typeToMock) && IsGetObjectDataVirtual(typeToMock));
+		}
+
+		private static bool ContainsDeserializationConstructor(Type typeToMock)
+		{
+			return typeToMock.GetConstructor(
+				BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic,
+				null,
+				new[] { typeof(SerializationInfo), typeof(StreamingContext) },
+				null) != null;
+		}
+
+		private static bool IsGetObjectDataVirtual(Type typeToMock)
+		{
+			var getObjectDataMethod = typeToMock.GetInterfaceMap(typeof(ISerializable)).TargetMethods[0];
+			return !getObjectDataMethod.IsPrivate && getObjectDataMethod.IsVirtual && !getObjectDataMethod.IsFinal;
 		}
 	}
 }
+
+#endif

UnitTests/Regressions/IssueReportsFixture.cs

@@ -8,13 +8,15 @@
 using System.Reflection;
 using System.Text;
 using System.Text.RegularExpressions;
+using Castle.DynamicProxy;
 using Moq;
 using Moq.Properties;
 using Moq.Protected;
 using Xunit;
 
 #if !NETCORE
 using System.Web.UI.HtmlControls;
+using System.Runtime.Serialization;
 #endif
 using System.Threading;
 using System.Threading.Tasks;
@@ -168,6 +170,160 @@ public void DoTest()
 #endif
 		#endregion
 
+		#region 163
+
+#if !NETCORE
+		public class Issue163  // see also issue 340 below
+		{
+			[Fact]
+			public void WhenMoqEncountersTypeThatDynamicProxyCannotHandleFallbackToEmptyDefaultValue()
+			{
+				// First, establish that we're looking at situation involving a type that DynamicProxy
+				// cannot handle:
+				var proxyGenerator = new ProxyGenerator();
+				Assert.Throws<ArgumentException>(() => proxyGenerator.CreateClassProxy<NoDeserializationCtor>());
+
+				// With such a type, Moq should fall back to the empty default value provider:
+				var foo = Mock.Of<Foo>();
+				Assert.Null(foo.SomeSerializableObject);
+			}
+
+			public abstract class Foo
+			{
+				public abstract NoDeserializationCtor SomeSerializableObject { get; }
+			}
+
+			[Serializable]
+			public abstract class NoDeserializationCtor : ISerializable
+			{
+				public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+			}
+
+			/// <summary>
+			/// The tests in this class document some of Moq's assumptions about
+			/// how Castle DynamicProxy handles various correct and incorrect
+			/// implementations of `ISerializable`. If any one these tests start
+			/// failing, this is a signal that DynamicProxy has changed, and that
+			/// Moq might have to be adjusted accordingly.
+			/// </summary>
+			public class AssumptionsAboutDynamicProxy
+			{
+				[Theory]
+				[InlineData(typeof(CorrectImplementation))]
+				public void CanCreateProxyForCorrectImplementaiton(Type classToProxy)
+				{
+					var proxyGenerator = new ProxyGenerator();
+					var proxy = proxyGenerator.CreateClassProxy(classToProxy);
+					Assert.NotNull(proxy);
+				}
+
+				[Theory]
+				[InlineData(typeof(NoSerializableAttribute))]
+				[InlineData(typeof(NoSerializableAttributeAndNoDeserializationCtor))]
+				[InlineData(typeof(NoSerializableAttributeAndGetObjectDataNotVirtual))]
+				public void DoesNotMindPossibleErrorsIfNoSerializableAttribute(Type classToProxy)
+				{
+					var proxyGenerator = new ProxyGenerator();
+					var proxy = proxyGenerator.CreateClassProxy(classToProxy);
+					Assert.NotNull(proxy);
+				}
+
+				[Theory]
+				[InlineData(typeof(NotISerializable))]
+				[InlineData(typeof(NotISerializableAndNoDeserializationCtor))]
+				[InlineData(typeof(NotISerializableAndGetObjectDataNotVirtual))]
+				public void DoesNotMindPossibleErrorsIfNotISerializable(Type classToProxy)
+				{
+					var proxyGenerator = new ProxyGenerator();
+					var proxy = proxyGenerator.CreateClassProxy(classToProxy);
+					Assert.NotNull(proxy);
+				}
+
+
+				[Theory]
+				[InlineData(typeof(NoDeserializationCtor))]
+				public void DoesMindMissingDeserializationCtor(Type classToProxy)
+				{
+					var proxyGenerator = new ProxyGenerator();
+					Assert.Throws<ArgumentException>(() => proxyGenerator.CreateClassProxy(classToProxy));
+				}
+
+				[Theory]
+				[InlineData(typeof(GetObjectDataNotVirtual))]
+				public void DoesMindNonVirtualGetObjectData(Type classToProxy)
+				{
+					var proxyGenerator = new ProxyGenerator();
+					Assert.Throws<ArgumentException>(() => proxyGenerator.CreateClassProxy(classToProxy));
+				}
+
+				public abstract class NoSerializableAttribute : ISerializable
+				{
+					protected NoSerializableAttribute() { }
+					protected NoSerializableAttribute(SerializationInfo info, StreamingContext context) { }
+					public void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				public abstract class NoSerializableAttributeAndNoDeserializationCtor : ISerializable
+				{
+					public void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				public abstract class NoSerializableAttributeAndGetObjectDataNotVirtual : ISerializable
+				{
+					protected NoSerializableAttributeAndGetObjectDataNotVirtual() { }
+					protected NoSerializableAttributeAndGetObjectDataNotVirtual(SerializationInfo info, StreamingContext context) { }
+					public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class CorrectImplementation : ISerializable
+				{
+					protected CorrectImplementation() { }
+					protected CorrectImplementation(SerializationInfo info, StreamingContext context) { }
+					public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class NotISerializable
+				{
+					protected NotISerializable() { }
+					protected NotISerializable(SerializationInfo info, StreamingContext context) { }
+					public void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class NotISerializableAndNoDeserializationCtor
+				{
+					public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class NotISerializableAndGetObjectDataNotVirtual
+				{
+					protected NotISerializableAndGetObjectDataNotVirtual() { }
+					protected NotISerializableAndGetObjectDataNotVirtual(SerializationInfo info, StreamingContext context) { }
+					public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class NoDeserializationCtor : ISerializable
+				{
+					public virtual void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+
+				[Serializable]
+				public abstract class GetObjectDataNotVirtual : ISerializable
+				{
+					protected GetObjectDataNotVirtual() { }
+					protected GetObjectDataNotVirtual(SerializationInfo info, StreamingContext context) { }
+					public void GetObjectData(SerializationInfo info, StreamingContext context) { }
+				}
+			}
+		}
+#endif
+
+		#endregion
+
 		#region #176
 
 		public class Issue176
@@ -478,6 +634,51 @@ public void Strict_mock_expecting_calls_with_nonequal_BadlyHashed_values_should_
 
 		#endregion // #328
 
+		#region 340
+
+#if !NETCORE
+		/// <summary>
+		/// These tests check whether the presence of a deserialization ctor and/or a GetObjectData
+		/// method alone can fool Moq into assuming that a type is ISerializable, or implements
+		/// it incompletely when it isn't ISerializable at all.
+		/// </summary>
+		public class Issue340  // see also issue 163 above
+		{
+			[Fact]
+			public void ClaimsPrincipal_has_ISerializable_contract_but_is_not_ISerializable()
+			{
+				var ex = Record.Exception(() => Mock.Of<Repro1>());
+				Assert.Null(ex);
+			}
+
+			public abstract class Repro1
+			{
+				public abstract System.Security.Claims.ClaimsPrincipal Principal { get; }
+			}
+
+			[Fact]
+			public void Foo_has_incomplete_ISerializable_contract_but_is_not_ISerializable()
+			{
+				var ex = Record.Exception(() => Mock.Of<Repro2>());
+				Assert.Null(ex);
+			}
+
+			public abstract class Repro2
+			{
+				public abstract Foo FooProperty { get; }
+			}
+
+			[Serializable]
+			public class Foo
+			{
+				public Foo() { }
+				protected Foo(SerializationInfo info, StreamingContext context) { }
+			}
+		}
+#endif
+
+#endregion
+
 		// Old @ Google Code
 
 		#region #47