Harmony 3

[pardeike]

👋 Welcome!

This Discussions page acts as a place to define what Harmony 3 will look like. We hope that you:

• Ask questions you’re wondering about.
• Share ideas.
• Engage with other community members.
• Welcome others and are open-minded. Remember that Harmony is a community project that we build together 💪.

To get started, comment below with an introduction of yourself and tell us your thoughts and ideas for the future of Harmony.

[TonyValenti]

@pardeike I wanted to let you know that Harmony is really freaking amazing! I just found this library today and I love it!

[TonyValenti]

Hi @pardeike - I wanted to give you some of my thoughts about how Harmony3 could be even better:

Today, it seems like there is a lot of nuance around what the method signatures should be when overriding methods. I would like to suggest that this be standardized and well-documented.

I would recommend following a programming model similar to what PostSharp or MetaLama do when calling into base methods. For example, when creating an override, I feel like I should be able to do something like the following:

Overriding an Instance Method

            typeof(InstanceClass)
                .InstanceMethods(typeof(Func<string, string>)
                .WithName(nameof(StaticClass.GetStringValue))
                .OverrideWith(typeof(InstanceOverride))
                ;

    public class InstanceOverride : Override<Func<string, string>, InstanceClass> {

        protected string Handler(string Input) {
            var ret = This.GetType().Name + Base(Input);
            return ret;
        }

    }

Overriding a Static Method

            typeof(StaticClass)
                .StaticMethods(typeof(Func<Task<string>>)
                .WithName(nameof(StaticClass.GetStringValueAsync))
                .OverrideWith(typeof(StaticTaskOverride))
                ;

   public class StaticTaskOverride: Override<Func<Task<string>>> {
        
        protected async Task<string> Handler() {
            await Task.CompletedTask;
            return "Goodbye World Async";
        }

    }

Overriding a Generic Method

typeof(GenericClass<,>)
                .InstanceMethods(typeof(Func<,>))
                .WithName(nameof(GenericClass.GetValue))
                .OverrideWith(typeof(GenericOverride))
                ;

    public class GenericOverride<TKey, TValue> : Override<Func<TKey, TValue>, GenericClass<TKey, TValue>> {
        protected TValue Handler(TKey Key) {
            var ret = Base(Key);

            return ret;
        }
    }

In the above, there are a few things that I'd like to point out:

1. There is a base class called Override that represents the replacement method. There should be two definitions over the Override class:

   //Used for Overriding an Instance method
    public abstract class Override<TDelegate, TThis> : Override<TDelegate> where TDelegate : Delegate {
        protected TThis This { get; } = default!;
    }

   //Used for Overriding a Static method
    public abstract class Override<TDelegate> where TDelegate : Delegate {
        protected TDelegate Base { get; } = default!;
    }

By convention, all classes that inherit from Override must have a method named Handler that matches the signature of TDelegate. This is the new method that will override the existing method.

2. When an overridden method is invoked, this is what should happen in the background:
   A. A new copy of the Override class is created.
   B. The This property is set to the current instance of the object that is being invoked.
   C. The Base property is set to a delegate that represents the next method in the invocation chain. This would either be the next Override or the original method.

3. This makes wrapping (or even excluding) the next method in the chain much easier.

4. This is also Async friendly as you don't need to do weird stuff with ref parameters.

[TonyValenti]

Hi @pardeike -
I wanted to pass along a few other things I'd like to request/suggest:

1. I really like your reflection helpers. It might be nice if these existed in an alternate assembly/nuget package like "Harmony.Reflection" or something like that so it can be used independently of the harmony patching.
2. I've thought a bit about the above and it might be better to just have an injectable HarmonyContext<TDelegate> and HarmonyContext<TDelegate, TThis> class. If you do that, the TDelegate should be a Func/Action the represents the "base" method that was overridden and the TThis should represent the instance of the class the method is being called on.
3. I think that the SEO for the package should be upgraded and include words and phrases like "override method in third party assemblies" and "intercept calls in other objects".

[TonyValenti]

It would also be great if Harmony had some prebuilt MethodInfos for common cases. For example:

Harmony.Handlers.AlwaysTrue(); //MethodInfo that always returns true
Harmony.Handlers.AlwaysFalse(); //MethodInfo that always returns false
Harmony.Handlers.AlwaysNull<T>(); //MethodInfo that always returns null
Harmony.Handlers.DoNothing(); //MethodInfo that does nothing.  Ie.  static void DoNothing() { }

[alexms2001]

Hi Andreas, I tried to apply Harmony to the following problem: I want to set all CommandTimeout for all SqlCommand's to 0 (the idea is to be able to have lunch while sitting on a breakpoint in debug mode without timed-out commands in other threads). Turns out, all is well for .NET48 I have to use in 64-bit build, with just one fix:
if (parameters is null)
result = type.GetMethod(name, allDeclared);
in ...\Harmony\Tools\AccessTools.cs@380 I replaced by
if (parameters is null)
result = type.GetMethod(name, allDeclared, null, Type.EmptyTypes, modifiers);

Here's why: SqlCommand has several ctors, and poor GetMethod gets confused and throws; on the other hand, I cannot put Type.EmptyTypes in [HarmonyLib.HarmonyPatch(typeof(SqlCommand), HarmonyLib.MethodType.Constructor)] attribute b/c Type.EmptyTypes is NOT a const. Maybe things are more relaxed in .NET 6, but this trick works in all cases.
All is well in 64 bit case, as I said.

However, in 32 bit case I get an infinite loop in protected override unsafe IntPtr GetFunctionPointer(MethodBase method, RuntimeMethodHandle handle) in ...\MonoMod.Common\RuntimeDetour\Platforms\Runtime\DetourRuntimeNETPlatform.cs@106-@260 - the code returns to ReloadFuncPtr: label infinitely.
I have a fully independent solution that I can provide as reference, if you want, 16MB when zipped. Just let me know where I can put it. It directly referenses both Harmony's and MonoMod.Common sources so it is 100% self-contained.
Let me know if you are interested.

[Windows10CE]

I'm confused by what you mean by "not a const", even though type arrays are not const, they are absolutely allowed in attribute constructors, see this sharplab example: https://sharplab.io/#v2:D4AQTAjAsAUCDMACciDCiDetE8QbQEEAXIgCgDsBTAd0QBUBPAB0rwAYBdASg+1wWQAWRAFlSXTHxwBfWLJiwBKYkUQAuRCoBOASwBGAVyKUsMXMiQrSjFng6IiEjPOlA===

Your solution is also a breaking change, it would break something like this:

public class A
{
    public Method(int a) {}
}

[HarmonyPatch(typeof(A), "Method")]
public static class Patch {}

Passing Type.EmptyTypes to GetMethod as a default when no type array is supplied would cause the call GetMethod to no longer find that Method, because it would be expecting no parameters (as you told it to by passing an array of 0 types).

[alexms2001]

Just try to add anything non-const (any new is non-const, alas) and compile. Good luck. I think that your argument about "breaking change" is also incorrect: all i did is to confirm that i need to select a method w/o parameters (when there are several overloads). Note also that i use .NET 4.8, that's my requurement, did not bother to check core etc.

[alexms2001]

i looked again at your sample code: your method has a parameter, so the attribute has to mention its type. Just assume that you have two overloads: with aparam and wo it - how can poor dotnet select the right one?

[Windows10CE]

i looked again at your sample code: your method has a parameter, so the attribute has to mention its type. Just assume that you have two overloads: with aparam and wo it - how can poor dotnet select the right one?

You simply do this:

public class A
{
    public void Method() {}
    public void Method(int a) {}
}

[HarmonyPatch(typeof(A), "Method", new Type[0])]
public static class PatchNoParameters {}

[HarmonyPatch(typeof(A), "Method", new Type[] { typeof(int) })]
public static class PatchIntParameter {}

And just as evidence that this compiles fine, since you seem to think it will not:

[alexms2001]

I was wrong, you were right. Can't explain this rationally, I saw compilation error with my own eyes, but now I see that new Type[0] as an argument in an attribute does compile in .NET 4.8.
My apologies.
Note that Type.EmptyTypes, which should be an alias for new Type[0], does not work (?). A pro po, this is just one of strange things that happen around consts in .NET 4.8: neither Environment.NewLine nor string.Empty cannot be assigned to consts:

    const string aaa = Environment.NewLine; // The expression being assigned to 'SampleCode.aaa' must be constant
    const string bbb = string.Empty;               // The expression being assigned to 'SampleCode.bbb' must be constant

    const string ccc = "\r\n";                            // ok
    const string ddd = "";                                // ok

[TonyValenti]

I'm not sure about the complexity of prefixes, postfixes, and patches.
I would like it if I could basically think about a patch as an override and could basically write code like the following:

pubic class MyPatch : PatchPoint {

  protected HarmonyResult<string> Invoke([HarmonyInstance] This, [HarmonyArg("someArg")] Arg, HarmonyDelegate Base) {
    var ret = HarmonyResult.Create("My Value");
    if(false){
      ret = Base.Invoke();
    }

   return ret;
  }

}

[krisrok]

@pardeike hi, hope this is an ok place to ask this: On an issue comment you mention you're doing some work to make Harmony use MonoMod. Now there's also the HarmonyX repo which seems to do the same. So I guess I just wonder if MonoMod is on the horizon for your original repo. (For context: I'm mainly interested in the ability to patch Unity's extern methods.)

[pardeike]

Harmony uses MonoMod.Core under the hood

[snorkyware]

Hi, I do Valheim modding, which uses HarmonyX an older fork of Harmony but I've managed to use Harmony to patch some of the newer functionality into itself, which is equal parts cool and somehow hilarious.

Anyway, one community guideline for Valheim modding which has evolved is to never use a bool prefix method, never skip because you could end up skipping other prefixes and breaking other mods. Is this an issue for which a more elegant solution could exist? Could there ever be a way to skip the original but not other overrides? Is this even sane?

[pardeike]

If you use a prefix that does not change the state of the original then Harmony will not skip your prefix even if another prefix before you did return false.

So simply use static void Prefix() {} to be unskipable

[snorkyware]

Sure, thanks. I guess what I'm asking is, in a future version of Harmony, could there ever be an unskippable bool prefix, or a way to achieve the equivalent behaviour? An unskippable way to skip the original method completely, rather than just allowing the original to run and then replacing the result with a postfix.

[pardeike]

That’s an anti pattern. It only leads to other developers patching your “unskipable” patch to skip it. There is no way to guarantee that you get what you want. So we should not pretend that it exists.

[snorkyware]

Okay, thanks! This is a high level view that's taken me a while to unpack, but I think I comprehend.

My suggestion is an antipattern, just as a mod that breaks if its bool prefix is skipped is an antipattern.

I do find Harmony a joy to use. It's powerful and well-thought-out. You've given us two different flavours of prefix, and a priority system. All the tools are here for developers to rub along in a multi-plugin environment. With all that said, I don't think I support this blanket ban on bool prefixes at all.