Merge pull request castleproject#645 from stakx/refactor/method-finder

Refactor `MethodFinder.GetAllInstanceMethods`

Author: stakx

src/Castle.Core.Tests/DynamicProxy.Tests/GenericInterfaceProxyTestCase.cs

@@ -16,9 +16,8 @@ namespace Castle.DynamicProxy.Tests
 {
 	using System;
 	using System.Collections.Generic;
-	using System.Reflection;
 
-	using Castle.DynamicProxy.Generators;
+	using Castle.DynamicProxy.Internal;
 	using Castle.DynamicProxy.Tests.GenInterfaces;
 	using Castle.DynamicProxy.Tests.Interceptors;
 	using Castle.DynamicProxy.Tests.Interfaces;
@@ -415,17 +414,17 @@ public void TypeGetMethodsIsStable()
 		[Test(Description =
 			"There is a strange CLR bug resulting from our loading the tokens of methods in generic types. " +
 			"This test ensures we correctly work around it.")]
-		public void MethodFinderIsStable()
+		public void GetAllInstanceMethodsIsStable()
 		{
 			ProxyWithGenInterfaceWithBase();
-			Assert.AreEqual(4, MethodFinder.GetAllInstanceMethods(
-				typeof(IGenInterfaceHierarchyBase<int>), BindingFlags.Public | BindingFlags.Instance).Length);
+			Assert.AreEqual(4, typeof(IGenInterfaceHierarchyBase<int>).GetAllInstanceMethods().Length);
 		}
 
 #if FEATURE_APPDOMAIN
 		[Test(Description =
 			"There is a strange CLR bug resulting from our loading the tokens of methods in generic types. " +
-			"This test ensures we do not trigger it across AppDomains. If we do, MethodFinder must provide a cross-AppDomain workaround.")]
+			"This test ensures we do not trigger it across AppDomains. " +
+			"If we do, GetAllInstanceMethods must provide a cross-AppDomain workaround.")]
 		public void TypeGetMethodsIsStableInDifferentAppDomains()
 		{
 			ProxyWithGenInterfaceWithBase();

src/Castle.Core.Tests/DynamicProxy.Tests/MethodFinderTestCase.cs

@@ -0,0 +1,69 @@
+// Copyright 2004-2021 Castle Project - http://www.castleproject.org/
+// 
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+// 
+//     http://www.apache.org/licenses/LICENSE-2.0
+// 
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+namespace Castle.DynamicProxy.Tests
+{
+	using System.Linq;
+	using System.Reflection;
+
+	using Castle.DynamicProxy.Internal;
+
+	using NUnit.Framework;
+
+	[TestFixture]
+	public class TypeUtilTestCase
+	{
+		[Test]
+		public void GetAllInstanceMethods_GetsPublicAndNonPublicMethods()
+		{
+			MethodInfo[] methods =
+				typeof(object).GetAllInstanceMethods();
+			MethodInfo[] realMethods =
+				typeof(object).GetMethods(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public);
+			CollectionAssert.AreEquivalent(realMethods, methods);
+		}
+
+		// The test above suggests that `TypeUtil.GetAllInstanceMethods` and `Type.GetMethods`
+		// can be used interchangeably, but this is not always the case. See the test(s) below and
+		// `GenericInterfaceProxyTestCase.GetAllInstanceMethodsIsStable` for cases where the two methods
+		// may produce different results.
+
+#if NET5_0_OR_GREATER
+
+		[Test]
+		public void GetAllInstanceMethods_NoDuplicatesForMethodWithCovariantReturnType()
+		{
+			MethodInfo[] methods =
+				typeof(Derived).GetAllInstanceMethods();
+			MethodInfo[] realMethods =
+				typeof(Derived).GetMethods(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public);
+			CollectionAssert.AreNotEquivalent(realMethods, methods);
+			CollectionAssert.IsSubsetOf(subset: methods, superset: realMethods);
+			Assert.AreEqual(2, realMethods.Where(m => m.Name == nameof(Derived.Method)).Count());
+			Assert.AreEqual(1, methods.Where(m => m.Name == nameof(Derived.Method)).Count());
+		}
+
+		public abstract class Base
+		{
+			public abstract Base Method();
+		}
+
+		public abstract class Derived : Base
+		{
+			public override abstract Derived Method();
+		}
+
+#endif
+	}
+}

src/Castle.Core.Tests/DynamicProxy.Tests/TypeUtilTestCase.cs

@@ -72,7 +72,7 @@ void CollectEvents()
 
 			void CollectMethods()
 			{
-				var methodsFound = MethodFinder.GetAllInstanceMethods(type, Flags);
+				var methodsFound = type.GetAllInstanceMethods();
 				foreach (var method in methodsFound)
 				{
 					AddMethod(method, true);

src/Castle.Core/DynamicProxy/Contributors/MembersCollector.cs

@@ -73,7 +73,7 @@ private static MethodInfo ObtainMethod(MethodInfo proxiedMethod, Type type)
 			else
 			{
 				// NOTE: this implementation sucks, feel free to improve it.
-				var methods = MethodFinder.GetAllInstanceMethods(type, BindingFlags.Public | BindingFlags.NonPublic);
+				var methods = type.GetAllInstanceMethods();
 				foreach (var method in methods)
 				{
 					if (MethodSignatureComparer.Instance.Equals(method.GetBaseDefinition(), proxiedMethod))

src/Castle.Core/DynamicProxy/Generators/MethodFinder.cs

@@ -17,13 +17,17 @@ namespace Castle.DynamicProxy.Internal
 	using System;
 	using System.Collections.Generic;
 	using System.Diagnostics;
+	using System.Linq;
 	using System.Reflection;
 	using System.Reflection.Emit;
 
+	using Castle.DynamicProxy.Generators;
 	using Castle.DynamicProxy.Generators.Emitters;
 
 	public static class TypeUtil
 	{
+		private static readonly Dictionary<Type, MethodInfo[]> instanceMethodsCache = new Dictionary<Type, MethodInfo[]>();
+
 		internal static bool IsNullableType(this Type type)
 		{
 			return type.IsGenericType &&
@@ -55,6 +59,28 @@ internal static FieldInfo[] GetAllFields(this Type type)
 			return fields.ToArray();
 		}
 
+		/// <summary>
+		///   Returns the methods implemented by a type. Use this instead of Type.GetMethods to filter out duplicate MethodInfos
+		///   sometimes reported by the latter. The test suite documents cases where such duplicates may occur.
+		/// </summary>
+		internal static MethodInfo[] GetAllInstanceMethods(this Type type)
+		{
+			MethodInfo[] methodsInCache;
+
+			lock (instanceMethodsCache)
+			{
+				if (!instanceMethodsCache.TryGetValue(type, out methodsInCache))
+				{
+					methodsInCache = type.GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
+					                     .Distinct(MethodSignatureComparer.Instance)
+					                     .ToArray();
+					instanceMethodsCache.Add(type, methodsInCache);
+				}
+			}
+
+			return methodsInCache;
+		}
+
 		/// <summary>
 		///   Returns list of all unique interfaces implemented given types, including their base interfaces.
 		/// </summary>