From 3de4d124bbab6d8f7546dfd50db245354daa6f7a Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Tue, 23 Jan 2024 11:43:37 +0100 Subject: [PATCH 1/7] Fix nullref in GetOriginalMethod with missing getter/setter --- Harmony/Internal/PatchTools.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Harmony/Internal/PatchTools.cs b/Harmony/Internal/PatchTools.cs index 3905e29b..65355073 100644 --- a/Harmony/Internal/PatchTools.cs +++ b/Harmony/Internal/PatchTools.cs @@ -65,13 +65,13 @@ internal static MethodBase GetOriginalMethod(this HarmonyMethod attr) case MethodType.Getter: if (attr.methodName is null) - return AccessTools.DeclaredIndexer(attr.declaringType, attr.argumentTypes).GetGetMethod(true); - return AccessTools.DeclaredProperty(attr.declaringType, attr.methodName).GetGetMethod(true); + return AccessTools.DeclaredIndexer(attr.declaringType, attr.argumentTypes)?.GetGetMethod(true); + return AccessTools.DeclaredProperty(attr.declaringType, attr.methodName)?.GetGetMethod(true); case MethodType.Setter: if (attr.methodName is null) - return AccessTools.DeclaredIndexer(attr.declaringType, attr.argumentTypes).GetSetMethod(true); - return AccessTools.DeclaredProperty(attr.declaringType, attr.methodName).GetSetMethod(true); + return AccessTools.DeclaredIndexer(attr.declaringType, attr.argumentTypes)?.GetSetMethod(true); + return AccessTools.DeclaredProperty(attr.declaringType, attr.methodName)?.GetSetMethod(true); case MethodType.Constructor: return AccessTools.DeclaredConstructor(attr.GetDeclaringType(), attr.argumentTypes); From a066adfa409aa6549b03bd146a95523414b30af2 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Tue, 23 Jan 2024 11:45:05 +0100 Subject: [PATCH 2/7] Throw proper exception if reverse patch target is missing --- Harmony/Public/PatchClassProcessor.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Harmony/Public/PatchClassProcessor.cs b/Harmony/Public/PatchClassProcessor.cs index d05e55e2..e5817b11 100644 --- a/Harmony/Public/PatchClassProcessor.cs +++ b/Harmony/Public/PatchClassProcessor.cs @@ -128,6 +128,8 @@ void ReversePatch(ref MethodBase lastOriginal) var annotatedOriginal = patchMethod.info.GetOriginalMethod(); if (annotatedOriginal is object) lastOriginal = annotatedOriginal; + if (lastOriginal is null) + throw new ArgumentException($"Undefined target method for reverse patch method {patchMethod.info.method.FullDescription()}"); var reversePatcher = instance.CreateReversePatcher(lastOriginal, patchMethod.info); lock (PatchProcessor.locker) _ = reversePatcher.Patch(); From baa51ffea1bf25f4900bbe4b85ea8efbc6f103a6 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Tue, 23 Jan 2024 11:46:45 +0100 Subject: [PATCH 3/7] Add HarmonyOptional attribute --- Harmony/Public/Attributes.cs | 27 +++++++++++++++++++ Harmony/Public/HarmonyMethod.cs | 4 +++ Harmony/Public/PatchClassProcessor.cs | 28 ++++++++++++++++++- HarmonyTests/Patching/Assets/Specials.cs | 34 ++++++++++++++++++++++++ HarmonyTests/Patching/Specials.cs | 13 ++++++++- 5 files changed, 104 insertions(+), 2 deletions(-) diff --git a/Harmony/Public/Attributes.cs b/Harmony/Public/Attributes.cs index 80eb0103..ac37b59d 100644 --- a/Harmony/Public/Attributes.cs +++ b/Harmony/Public/Attributes.cs @@ -777,4 +777,31 @@ public HarmonyArgument(int index, string name) NewName = name; } } + + /// Flags used for optionally patching members that might not exist. + /// + [Flags] + public enum OptionalFlags + { + /// Default behavior (throw and abort patching if there are no matches). + /// + None = 0, + + /// Do not throw an exception and abort the patching process if no matches are found (a warning is logged instead). + /// + AllowNoMatches = 1 << 1, + } + + /// Attribute used for optionally patching members that might not exist. + /// + [AttributeUsage(AttributeTargets.Method)] + public class HarmonyOptional : HarmonyAttribute + { + /// Default constructor + /// + public HarmonyOptional(OptionalFlags flags = OptionalFlags.AllowNoMatches) + { + info.optionalFlags = flags; + } + } } diff --git a/Harmony/Public/HarmonyMethod.cs b/Harmony/Public/HarmonyMethod.cs index dd741713..c72c9a55 100644 --- a/Harmony/Public/HarmonyMethod.cs +++ b/Harmony/Public/HarmonyMethod.cs @@ -66,6 +66,10 @@ public class HarmonyMethod /// Whether to wrap the patch itself into a try/catch. /// public bool? wrapTryCatch; + + /// Flags used for optionally patching members that might not exist. + /// + public OptionalFlags? optionalFlags; /// Default constructor /// diff --git a/Harmony/Public/PatchClassProcessor.cs b/Harmony/Public/PatchClassProcessor.cs index e5817b11..5460b67a 100644 --- a/Harmony/Public/PatchClassProcessor.cs +++ b/Harmony/Public/PatchClassProcessor.cs @@ -5,6 +5,7 @@ using System.Text; using HarmonyLib.Public.Patching; using HarmonyLib.Tools; +using MonoMod.Utils; namespace HarmonyLib { @@ -59,7 +60,7 @@ public PatchClassProcessor(Harmony instance, Type type, bool allowUnannotatedTyp containerAttributes = HarmonyMethod.Merge(harmonyAttributes); if (containerAttributes.methodType is null) // MethodType default is Normal containerAttributes.methodType = MethodType.Normal; - + this.Category = containerAttributes.category; auxilaryMethods = new Dictionary(); @@ -128,8 +129,18 @@ void ReversePatch(ref MethodBase lastOriginal) var annotatedOriginal = patchMethod.info.GetOriginalMethod(); if (annotatedOriginal is object) lastOriginal = annotatedOriginal; + if (lastOriginal is null) + { + if (IsPatchOptional(patchMethod)) + { + Logger.Log(Logger.LogChannel.Warn, () => $"Skipping optional reverse patch {patchMethod.info.method.FullDescription()} - target method not found"); + continue; + } + throw new ArgumentException($"Undefined target method for reverse patch method {patchMethod.info.method.FullDescription()}"); + } + var reversePatcher = instance.CreateReversePatcher(lastOriginal, patchMethod.info); lock (PatchProcessor.locker) _ = reversePatcher.Patch(); @@ -173,7 +184,15 @@ List PatchWithAttributes(ref MethodBase lastOriginal) { lastOriginal = patchMethod.info.GetOriginalMethod(); if (lastOriginal is null) + { + if (IsPatchOptional(patchMethod)) + { + Logger.Log(Logger.LogChannel.Warn, () => $"Skipping optional patch {patchMethod.info.method.FullDescription()} - target method not found"); + continue; + } + throw new ArgumentException($"Undefined target method for patch method {patchMethod.info.method.FullDescription()}"); + } var job = jobs.GetJob(lastOriginal); job.AddPatch(patchMethod); @@ -186,6 +205,13 @@ List PatchWithAttributes(ref MethodBase lastOriginal) return jobs.GetReplacements(); } + static bool IsPatchOptional(AttributePatch patchMethod) + { + var optionalFlags = patchMethod.info.optionalFlags; + var isOptional = optionalFlags != null && optionalFlags.Value.Has(OptionalFlags.AllowNoMatches); + return isOptional; + } + void ProcessPatchJob(PatchJobs.Job job) { MethodInfo replacement = default; diff --git a/HarmonyTests/Patching/Assets/Specials.cs b/HarmonyTests/Patching/Assets/Specials.cs index e8e821f2..4eb1a0a6 100644 --- a/HarmonyTests/Patching/Assets/Specials.cs +++ b/HarmonyTests/Patching/Assets/Specials.cs @@ -127,6 +127,40 @@ public static void ReplaceGetValue(ref bool __result) } } + public class OptionalPatch + { + [HarmonyPrefix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), "missing_method")] + public static void Test0() => throw new InvalidOperationException(); + + [HarmonyReversePatch, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), "missing_method")] + public static void Test1() => throw new InvalidOperationException(); + + [HarmonyPostfix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), MethodType.Constructor, typeof(string))] + public static void Test2() => throw new InvalidOperationException(); + + [HarmonyTranspiler, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), "missing_method", MethodType.Getter)] + public static void Test3() => throw new InvalidOperationException(); + + [HarmonyPostfix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Enumerator)] + public static void Test4() => throw new InvalidOperationException(); + + [HarmonyPostfix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Async)] + public static void Test5() => throw new InvalidOperationException(); + + private void NotEnumerator() => throw new InvalidOperationException(); + } + + public static class OptionalPatchNone + { + [HarmonyPrefix] + [HarmonyOptional(OptionalFlags.None)] + [HarmonyPatch(typeof(OptionalPatch), "missing_method")] + public static void Prefix() + { + throw new InvalidOperationException(); + } + } + public static class SafeWrapPatch { public static bool called = false; diff --git a/HarmonyTests/Patching/Specials.cs b/HarmonyTests/Patching/Specials.cs index 7800c3ad..917657d5 100644 --- a/HarmonyTests/Patching/Specials.cs +++ b/HarmonyTests/Patching/Specials.cs @@ -42,6 +42,17 @@ public void Test_HttpWebRequestGetResponse() } */ + [Test] + public void Test_Optional_Patch() + { + var instance = new Harmony("special-case-optional-patch"); + Assert.NotNull(instance); + + Assert.DoesNotThrow(() => instance.PatchAll(typeof(OptionalPatch))); + + Assert.Throws(() => instance.PatchAll(typeof(OptionalPatchNone))); + } + [Test] public void Test_Wrap_Patch() { @@ -185,7 +196,7 @@ public void Test_Enumerator_Patch() Assert.AreEqual("MoveNext", EnumeratorPatch.patchTarget.Name); var testObject = new EnumeratorCode(); - Assert.AreEqual(new []{ 1, 2, 3, 4, 5 }, testObject.NumberEnumerator().ToArray()); + Assert.AreEqual(new[] { 1, 2, 3, 4, 5 }, testObject.NumberEnumerator().ToArray()); Assert.AreEqual(6, EnumeratorPatch.runTimes); } From c3b8070ce0eb1e3ae502e8eb2b77b4d224c630f9 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Thu, 8 Feb 2024 00:13:36 +0100 Subject: [PATCH 4/7] Switch from using flags to a bool --- Harmony/Public/Attributes.cs | 21 ++++----------------- Harmony/Public/HarmonyMethod.cs | 4 ++-- Harmony/Public/PatchClassProcessor.cs | 11 ++--------- HarmonyTests/Patching/Assets/Specials.cs | 16 +++++++++------- 4 files changed, 17 insertions(+), 35 deletions(-) diff --git a/Harmony/Public/Attributes.cs b/Harmony/Public/Attributes.cs index ac37b59d..7fe50292 100644 --- a/Harmony/Public/Attributes.cs +++ b/Harmony/Public/Attributes.cs @@ -778,30 +778,17 @@ public HarmonyArgument(int index, string name) } } - /// Flags used for optionally patching members that might not exist. - /// - [Flags] - public enum OptionalFlags - { - /// Default behavior (throw and abort patching if there are no matches). - /// - None = 0, - - /// Do not throw an exception and abort the patching process if no matches are found (a warning is logged instead). - /// - AllowNoMatches = 1 << 1, - } - - /// Attribute used for optionally patching members that might not exist. + /// Attribute used for optionally patching members that might not exist. + /// Harmony patches with this attribute will not throw an exception and abort the patching process if the target member is not found (a warning is logged instead). /// [AttributeUsage(AttributeTargets.Method)] public class HarmonyOptional : HarmonyAttribute { /// Default constructor /// - public HarmonyOptional(OptionalFlags flags = OptionalFlags.AllowNoMatches) + public HarmonyOptional() { - info.optionalFlags = flags; + info.optional = true; } } } diff --git a/Harmony/Public/HarmonyMethod.cs b/Harmony/Public/HarmonyMethod.cs index c72c9a55..28f8c4b2 100644 --- a/Harmony/Public/HarmonyMethod.cs +++ b/Harmony/Public/HarmonyMethod.cs @@ -67,9 +67,9 @@ public class HarmonyMethod /// public bool? wrapTryCatch; - /// Flags used for optionally patching members that might not exist. + /// Whether to not throw/abort when trying to patch members that do not exist (skip instead). /// - public OptionalFlags? optionalFlags; + public bool? optional; /// Default constructor /// diff --git a/Harmony/Public/PatchClassProcessor.cs b/Harmony/Public/PatchClassProcessor.cs index 5460b67a..48d5f4d3 100644 --- a/Harmony/Public/PatchClassProcessor.cs +++ b/Harmony/Public/PatchClassProcessor.cs @@ -132,7 +132,7 @@ void ReversePatch(ref MethodBase lastOriginal) if (lastOriginal is null) { - if (IsPatchOptional(patchMethod)) + if (patchMethod.info.optional == true) { Logger.Log(Logger.LogChannel.Warn, () => $"Skipping optional reverse patch {patchMethod.info.method.FullDescription()} - target method not found"); continue; @@ -185,7 +185,7 @@ List PatchWithAttributes(ref MethodBase lastOriginal) lastOriginal = patchMethod.info.GetOriginalMethod(); if (lastOriginal is null) { - if (IsPatchOptional(patchMethod)) + if (patchMethod.info.optional == true) { Logger.Log(Logger.LogChannel.Warn, () => $"Skipping optional patch {patchMethod.info.method.FullDescription()} - target method not found"); continue; @@ -205,13 +205,6 @@ List PatchWithAttributes(ref MethodBase lastOriginal) return jobs.GetReplacements(); } - static bool IsPatchOptional(AttributePatch patchMethod) - { - var optionalFlags = patchMethod.info.optionalFlags; - var isOptional = optionalFlags != null && optionalFlags.Value.Has(OptionalFlags.AllowNoMatches); - return isOptional; - } - void ProcessPatchJob(PatchJobs.Job job) { MethodInfo replacement = default; diff --git a/HarmonyTests/Patching/Assets/Specials.cs b/HarmonyTests/Patching/Assets/Specials.cs index 4eb1a0a6..68d786f1 100644 --- a/HarmonyTests/Patching/Assets/Specials.cs +++ b/HarmonyTests/Patching/Assets/Specials.cs @@ -147,18 +147,20 @@ public class OptionalPatch [HarmonyPostfix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Async)] public static void Test5() => throw new InvalidOperationException(); + [HarmonyPostfix] + [HarmonyOptional] + [HarmonyPatch(typeof(OptionalPatch), "missing_method1")] + [HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Normal)] + [HarmonyPatch(typeof(OptionalPatch), "missing_method2")] + public static void Test6() => throw new InvalidOperationException(); + private void NotEnumerator() => throw new InvalidOperationException(); } public static class OptionalPatchNone { - [HarmonyPrefix] - [HarmonyOptional(OptionalFlags.None)] - [HarmonyPatch(typeof(OptionalPatch), "missing_method")] - public static void Prefix() - { - throw new InvalidOperationException(); - } + [HarmonyPrefix, HarmonyPatch(typeof(OptionalPatch), "missing_method")] + public static void Prefix() => throw new InvalidOperationException(); } public static class SafeWrapPatch From dfd65c3725011f691b9c697a07e0c21340ae30c4 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Thu, 8 Feb 2024 00:17:43 +0100 Subject: [PATCH 5/7] Unused using --- Harmony/Public/PatchClassProcessor.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Harmony/Public/PatchClassProcessor.cs b/Harmony/Public/PatchClassProcessor.cs index 48d5f4d3..6baac2c6 100644 --- a/Harmony/Public/PatchClassProcessor.cs +++ b/Harmony/Public/PatchClassProcessor.cs @@ -5,7 +5,6 @@ using System.Text; using HarmonyLib.Public.Patching; using HarmonyLib.Tools; -using MonoMod.Utils; namespace HarmonyLib { From 5d294353f2a4afb9197ef4a3c8bbe280e0dd8e10 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Thu, 8 Feb 2024 16:11:52 +0100 Subject: [PATCH 6/7] Improve test coverage --- HarmonyTests/Patching/Assets/Specials.cs | 16 +++++++++++----- HarmonyTests/Patching/Specials.cs | 4 ++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/HarmonyTests/Patching/Assets/Specials.cs b/HarmonyTests/Patching/Assets/Specials.cs index 68d786f1..901dfad9 100644 --- a/HarmonyTests/Patching/Assets/Specials.cs +++ b/HarmonyTests/Patching/Assets/Specials.cs @@ -147,20 +147,26 @@ public class OptionalPatch [HarmonyPostfix, HarmonyOptional, HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Async)] public static void Test5() => throw new InvalidOperationException(); - [HarmonyPostfix] + [HarmonyPrefix] [HarmonyOptional] [HarmonyPatch(typeof(OptionalPatch), "missing_method1")] - [HarmonyPatch(typeof(OptionalPatch), nameof(NotEnumerator), MethodType.Normal)] + [HarmonyPatch(typeof(OptionalPatch), nameof(Thrower), MethodType.Normal)] [HarmonyPatch(typeof(OptionalPatch), "missing_method2")] - public static void Test6() => throw new InvalidOperationException(); + public static bool Test6() => false; private void NotEnumerator() => throw new InvalidOperationException(); + public static void Thrower() => throw new InvalidOperationException(); } public static class OptionalPatchNone { - [HarmonyPrefix, HarmonyPatch(typeof(OptionalPatch), "missing_method")] - public static void Prefix() => throw new InvalidOperationException(); + [HarmonyPrefix] + [HarmonyPatch(typeof(OptionalPatch), "missing_method1")] + [HarmonyPatch(typeof(OptionalPatch), nameof(Thrower), MethodType.Normal)] + [HarmonyPatch(typeof(OptionalPatch), "missing_method2")] + public static bool Test6() => false; + + public static void Thrower() => throw new InvalidOperationException(); } public static class SafeWrapPatch diff --git a/HarmonyTests/Patching/Specials.cs b/HarmonyTests/Patching/Specials.cs index 917657d5..c6e09168 100644 --- a/HarmonyTests/Patching/Specials.cs +++ b/HarmonyTests/Patching/Specials.cs @@ -48,9 +48,13 @@ public void Test_Optional_Patch() var instance = new Harmony("special-case-optional-patch"); Assert.NotNull(instance); + Assert.Throws(OptionalPatch.Thrower); Assert.DoesNotThrow(() => instance.PatchAll(typeof(OptionalPatch))); + Assert.DoesNotThrow(OptionalPatch.Thrower); + Assert.Throws(OptionalPatchNone.Thrower); Assert.Throws(() => instance.PatchAll(typeof(OptionalPatchNone))); + Assert.Throws(OptionalPatchNone.Thrower); } [Test] From d6f8432c055741679fd73840a9639f8b059027e4 Mon Sep 17 00:00:00 2001 From: ManlyMarco <39247311+ManlyMarco@users.noreply.github.com> Date: Thu, 8 Feb 2024 16:14:28 +0100 Subject: [PATCH 7/7] Oops --- HarmonyTests/Patching/Assets/Specials.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/HarmonyTests/Patching/Assets/Specials.cs b/HarmonyTests/Patching/Assets/Specials.cs index 901dfad9..3947bcd2 100644 --- a/HarmonyTests/Patching/Assets/Specials.cs +++ b/HarmonyTests/Patching/Assets/Specials.cs @@ -162,7 +162,7 @@ public static class OptionalPatchNone { [HarmonyPrefix] [HarmonyPatch(typeof(OptionalPatch), "missing_method1")] - [HarmonyPatch(typeof(OptionalPatch), nameof(Thrower), MethodType.Normal)] + [HarmonyPatch(typeof(OptionalPatchNone), nameof(Thrower), MethodType.Normal)] [HarmonyPatch(typeof(OptionalPatch), "missing_method2")] public static bool Test6() => false;