Improve recognition logic for faulty ISerializable

Until now, Moq checked the presence of a deserialization ctor and of
a virtual `GetObjectData` method without first checking whether a
type actually implements `ISerializable`.

These checks happen inside `SerializedTypesValueProvider`, so all
related methods are moved there. Also, this value provider is exclu-
ded from .NET Core builds, since .NET Core does not have `ISerializ-
able` in the first place.

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

@@ -173,7 +173,7 @@ public void DoTest()
 		#region 163
 
 #if !NETCORE
-		public class Issue163
+		public class Issue163  // see also issue 340 below
 		{
 			[Fact]
 			public void WhenMoqEncountersTypeThatDynamicProxyCannotHandleFallbackToEmptyDefaultValue()
@@ -634,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