Merge pull request #326 from lbmaian/typebyname

Fix AccessTools.TypeByName/GetTypesFromAssembly to not throw exception when there exist "invalid" assemblies

Harmony/Public/Harmony.cs

@@ -103,17 +103,7 @@ public ReversePatcher CreateReversePatcher(MethodBase original, HarmonyMethod st
 		/// 
 		public void PatchAll(Assembly assembly)
 		{
-			Type[] types;
-			try
-			{
-				types = assembly.GetTypes();
-			}
-			catch (ReflectionTypeLoadException ex)
-			{
-				types = ex.Types.Where(tp => tp != null).ToArray();
-			}
-
-			types.Do(type => CreateClassProcessor(type).Patch());
+			AccessTools.GetTypesFromAssembly(assembly).Do(type => CreateClassProcessor(type).Patch());
 		}
 
 		/// <summary>Creates patches by manually specifying the methods</summary>

Harmony/Tools/AccessTools.cs

@@ -50,25 +50,27 @@ public static Type TypeByName(string name)
 			return type;
 		}
 
-		/// <summary>Gets all type by name from a given assembly. This is a wrapper that respects different .NET versions</summary>
+		/// <summary>Gets all successfully loaded types from a given assembly</summary>
 		/// <param name="assembly">The assembly</param>
 		/// <returns>An array of types</returns>
+		/// <remarks>
+		/// This calls and returns <see cref="Assembly.GetTypes"/>, while catching any thrown <see cref="ReflectionTypeLoadException"/>.
+		/// If such an exception is thrown, returns the successfully loaded types (<see cref="ReflectionTypeLoadException.Types"/>,
+		/// filtered for non-null values).
+		/// </remarks>
 		/// 
 		public static Type[] GetTypesFromAssembly(Assembly assembly)
 		{
-			// TODO: Catch and eat ReflectionTypeLoadException potentially thrown by GetTypes() (indicating an invalid assembly), to avoid TypeByName()
-			// from throwing a ReflectionTypeLoadException during its assembly search for a type that's potentially unrelated to such an invalid assembly,
-			// which is very unexpected behavior to users.
-			// Open question: should it catch any exception, or just ReflectionTypeLoadException?
-			// Open question: should it return an empty array in this case, or should it return ReflectionTypeLoadException.Types,
-			// filtered for non-null Types, if possible?
-			// TODO: .NET Core supports Assembly.GetTypes(), DefinedTypes internally calls GetTypes(), and ToArray() is an unnecessary expense,
-			// so remove this unnecessary special casing for .NET Core?
-#if NETCOREAPP3_0 || NETCOREAPP3_1
-			return assembly.DefinedTypes.ToArray();
-#else
-			return assembly.GetTypes();
-#endif
+			try
+			{
+				return assembly.GetTypes();
+			}
+			catch (ReflectionTypeLoadException ex)
+			{
+				if (Harmony.DEBUG)
+					FileLog.Log($"AccessTools.GetTypesFromAssembly: assembly {assembly} => {ex}");
+				return ex.Types.Where(type => !(type is null)).ToArray();
+			}
 		}
 
 		/// <summary>Applies a function going up the type hierarchy and stops at the first non null result</summary>

HarmonyTests/Tools/TestAccessTools.cs

@@ -129,15 +129,17 @@ static void TestTypeByNameWithInvalidAssembly(ITestIsolationContext context)
 			// The current executing assembly (HarmonyTests.dll) was definitely already loaded before above loads.
 			Assert.NotNull(AccessTools.TypeByName(typeof(Test_AccessTools).FullName));
 			// HarmonyTestsDummyAssemblyA is explicitly missing, so it's the same as the unknown type case - see below.
-			Assert.Throws(typeof(ReflectionTypeLoadException), () => AccessTools.TypeByName("HarmonyTestsDummyAssemblyA.Class1"));
-			// HarmonyTestsDummyAssemblyB.GetTypes() should throw ReflectionTypeLoadException due to missing HarmonyTestsDummyAssemblyA.
-			Assert.Throws(typeof(ReflectionTypeLoadException), () => AccessTools.TypeByName("HarmonyTestsDummyAssemblyB.Class1"));
-			// Even for a type in HarmonyTestsDummyAssemblyB that doesn't depend on HarmonyTestsDummyAssemblyA.
-			Assert.Throws(typeof(ReflectionTypeLoadException), () => AccessTools.TypeByName("HarmonyTestsDummyAssemblyB.Class2"));
-			// TypeByName's search should find HarmonyTestsDummyAssemblyB before HarmonyTestsDummyAssemblyC.
-			Assert.Throws(typeof(ReflectionTypeLoadException), () => AccessTools.TypeByName("HarmonyTestsDummyAssemblyC.Class1"));
-			// TypeByName's search for an unknown type should always find HarmonyTestsDummyAssemblyB first, again resulting in an exception.
-			Assert.Throws(typeof(ReflectionTypeLoadException), () => AccessTools.TypeByName("IAmALittleTeaPot.ShortAndStout"));
+			Assert.Null(AccessTools.TypeByName("HarmonyTestsDummyAssemblyA.Class1"));
+			// HarmonyTestsDummyAssemblyB.GetTypes() should throw ReflectionTypeLoadException due to missing HarmonyTestsDummyAssemblyA,
+			// but this is caught and returns successfully loaded types.
+			// HarmonyTestsDummyAssemblyB.Class1 depends on HarmonyTestsDummyAssemblyA, so it's not loaded successfully.
+			Assert.Null(AccessTools.TypeByName("HarmonyTestsDummyAssemblyB.Class1"));
+			// HarmonyTestsDummyAssemblyB.Class2 doesn't depend on HarmonyTestsDummyAssemblyA, so it's loaded successfully.
+			Assert.NotNull(AccessTools.TypeByName("HarmonyTestsDummyAssemblyB.Class2"));
+			// TypeByName's search should find HarmonyTestsDummyAssemblyB before HarmonyTestsDummyAssemblyC, but this is fine.
+			Assert.NotNull(AccessTools.TypeByName("HarmonyTestsDummyAssemblyC.Class1"));
+			// TypeByName's search for an unknown type should always find HarmonyTestsDummyAssemblyB first, which is again fine.
+			Assert.Null(AccessTools.TypeByName("IAmALittleTeaPot.ShortAndStout"));
 		}
 
 		static void TestTypeByNameWithNoInvalidAssembly(ITestIsolationContext context)