Add global flag to disallow global UnpatchAll behavior

[ErisApps]

This PR, as the title implies, adds a global flag to disallow the global UnpatchAll behavior.

This is an opt-in flag to keep binary compatibility and not introduce unexpected behavior by default, this while still being able to disallow the default behavior by turning on the flag.

Reasoning for introducing this flag is that, in certain contexts, it is often undesirable to unpatch every single patch at once
For example, in the case of modloaders where mods can be enabled/disabled at runtime and where accidentally calling the .UnpatchAll() method without an id, could break a whole bunch of other mods.

[ghorsington]

Greetings!

Thank you for the PR! Here are a few points/questions:

• How throwing an exception is any better than the current logged message? If the goal is to notify plugin maintainers that the method should not be used, the same can be done with the current warning message. HarmonyX logs should be listened to either way by the mod loader (at least warnings and errors). In fact, this is why HarmonyX has the warning log message when using UnpatchAll with no parameters as compared to vanilla Harmony.
• I'd rather not have any more magic globals in Harmony. Harmony.DEBUG exists only for binary compatibility with vanilla Harmony. I'd instead make a separate HarmonyGlobalSettings static class which has static properties (not fields) with possible global values.

Instead, how about the following:

• Make a static Harmony.UnpatchAll() (or equivalent, since the naming might not work because of the current instance method)
  • Have the option to unpatch a specific instance and unpatch all if need be
• Mark current UnpatchAll() with [Obsolete(true)] to prevent developers from using it

I am a bit hesitant to allow throwing an exception on UnpatchAll and instead would rather use the current logging system along with the obsoletion flag.

[ManlyMarco]

I think obsoleting UnpatchAll and adding a static version that's explicit in its purpose would be the best bet. Unpatching a specific ID would be better done with a separate method like UnpatchID to make its actions clearer.

[ErisApps]

Heya,

I'll try my best to answer your questions, but if anything is unclear or you disagree with something, feel free to let me know.

• The goal isn't only to notify the authors/maintainers that it should not be used, the intention is to strictly disallow it altogether.
  I've had quite the discussion about this before creating the PR, because previously, we were simply Harmony patching the UnpatchAll method to only allow unpatching when an Id was specified and force the UnpatchAll behavior to use its internal id nonetheless when it wasn't specified. We (modders from Beat Saber Modding Group) felt that for our use cases, there was not a single good reason why UnpatchAll should be allowed without specifying an Id in the first place. Therefor, throwing an exception seemed to be the best course of action to strictly disallow said behavior while not breaking binary compat.
• I'm totally fine with introducing a separate static HarmonyGlobalSettings, I simply added the global flag on the Harmony class itself because it made the most sense to add it there.

In regard to the suggestions:

• Introducing 2 new methods Harmony.UnpatchAll() (without arguments) and Harmony.UnpatchId(string id) is certainly possible, but I'd still love to see the UnpatchAll method to be gated behind an optional "disallow flag" because more often than not, it is used accidentally by less experienced developers/modders and as it still achieves its end results, they often overlook the warnings (we've had it happening sadly more than once in the past). Marking the old method as Obsolete is totally fine to me as well.
• One thing I'm not sure about is how to actually make those new methods actually static because the current remark on the UnpatchAll method states:

This method could be static if it wasn't for the fact that unpatching creates a new replacement method that contains your harmony ID

I'll gladly accept any help I can get for this though 🙂

[ghorsington]

the intention is to strictly disallow it altogether.

How do you handle error reporting and logging? What information does a user see if a plugin throws an exception? Does it prevent other plugins from being loaded? After all, exceptions cause stack unwinding, which can cause other game logic not to run correctly. In that sense, instead of throwing an exception, the flag could log a warning like UnpatchAll does now and then use return, no? At least, in that case, you don't risk breaking game logic if UnpatchAll happens in some critical place.

A long-term solution for preventing modders from using the method would be to mark UnpatchAll as Obsolete with error set to true. This approach prevents plugins from being compiled at all if the method is used.

Marking the old method as Obsolete is totally fine to me as well.

I think that's probably the best way to go. To summarise:

• UnpatchAll would be marked with [Obsolete("Use UnpatchSelf to unpatch the current instance", true)]. This attribute prevents new plugins from compiling with the old UnpatchAll.
• Add static Harmony.UnpatchAll() and Harmony.UnpatchId(string). Feel free to adjust the names if you think of a better idea.
• Add a flag to HarmonyGlobalSettings that skips old UnpatchAll if no ID was given. I suggest doing a return instead of throw unless you have a good argument for throwing an exception.

Feel free to comment on the suggestion.

One thing I'm not sure about is how to actually make those new methods actually static because the current remark on the UnpatchAll method states

That remark is probably old and should be removed. Right now, unpatching works by creating a PatchProcessor and calling Unpatch on it. While PatchProcessor's constructor takes Harmony instance as a parameter, it's not actually used when unpatching. As such, it is possible to construct a new PatchProcessor(null, original) and call Unpatch on it. I'd only probably add a null check to PatchProcessor.Patch and check that instance is not null (and give a reasonable exception, e.g. "Patching requires a Harmony instance").

It would undoubtedly require a little bit of fiddling, but you can essentially replace current Unpatch calls in UnpatchAll with creating new PatchProcessor(null, original) and calling Unpatch on it.

[ghorsington]

Looking at it, the whole obsoletion and new methods might be out of the scope of this PR.

As such, if you don't think you want/can make the new methods, I'm fine if this PR just adds the flag as you initially proposed. Although I still suggest thinking about whether the flag will cause a throw or a log message + return (I urge you to comment on this point). Once the flag PR is merged, we can add the new methods and mark UnpatchAll as obsolete. Or, if you feel like it, you can make a separate PR for the new methods.

Of course, I don't mind if you make the new methods in this PR; I just thought it might get out of scope for this PR.

[ErisApps]

The use of an exception was suggested to me, but if you rather prefer simply logging and returning, then I'll adjust it accordingly.

As for the whole obsoletion and introduction of new methods, I'm fine with trying to do that as well. But preferably, I'd like to do that in a new PR so we can at least go ahead with a version that contains this change as it might take me some time to figure things out 🙂